[foreign-abi] On invokers

Wed Oct 23 11:37:02 UTC 2019

I'll  take a closer look at the patch as well.

Jorn

On 23/10/2019 13:34, Jorn Vernee wrote:
> Wow, that was fast! Thanks for looking into this.
>
> On 23/10/2019 11:33, Nick Gasson wrote:
>> Hi Jorn,
>>
>> I tried getting this to work on Arm using your webrev.03 and the 
>> Windows ABI implementation as a base. The patch is here:
>>
>> http://cr.openjdk.java.net/~ngasson/panama/new-invoker/webrev.01/
>>
>> (I used the Skara webrev tool, seemed to work ok...). This passes all 
>> the tests in the jdk_foreign group. Just a couple of notes:
>>
>> * The upcall/downcall stubs on AArch64 are correct if we are calling 
>> from/to the standard C ABI, because the stubs use only caller-saved 
>> registers. For other potential ABIs I guess we need to preserve the 
>> set of registers that are callee-saved in the "from" ABI but 
>> caller-saved in the "to" ABI and vice-versa. I'm not sure the x86 
>> version is correct wrt. this at the moment either (?).
>
> Thanks, this still needs work (it's the most incorrect for downcalls). 
> I think it's working out so far since we are going between C++ <-> C. 
> You are correct in your assessment, but it becomes a little more 
> complicated because the stub itself also destroys some registers. 
> Those also need to be saved if they are non-volatile for the caller, 
> which is C++ for downcalls, and defined dynamically for upcalls (since 
> we can come from an arbitrary ABI).
>
> To recap, for downcalls:
> 1. Save every register destroyed by the stub, which is callee-saved 
> (i.e. non-volatile) according to the platform C++ ABI.
> 2. Save every register that is volatile in the called custom ABI and 
> which is callee-saved according to the platform C++ ABI.
>
> For upcalls:
> 1. Save every register destroyed by the stub, which is callee-saved 
> (i.e. non-volatile) according to the calling custom ABI.
> 2. Save every register that is volatile in the platform C++ ABI which 
> is callee-saved according to the calling custom ABI.
>
>> * Previously I had to make some changes to the shared code to support 
>> the "indirect result register" (r8) for by-value struct returns. This 
>> time I could make this work by just treating it as an input register.
> Nice, that's one of the goals of this exercise :)
>>
>> * foreign_globals_aarch64.[ch]pp is just cut+paste from the x86 
>> version with the x87 registers removed. Maybe we can put this in 
>> shared code to remove the duplication as the platform specific bits 
>> seem minimal? Not sure about other architectures in the future...
> The main difference seems to be between XMMRegister vs FloatRegister? 
> I think we can use VMReg(Pair) here to be more portable (though that 
> doesn't support x87 registers yet). Again, It's a place where more 
> work is to be done.
>> * Similarly CallArranger.java has some complex code in `if 
>> (returnsInMemory) { ... }' to shuffle the parameters for by-value 
>> struct returns. This is identical between AArch64 and Windows x86 so 
>> maybe we can factor it out? Not sure if it applies to SysV x86 too.
> Sounds good! Should apply to SysV as well, but SysV also needs to do 
> some additional shuffling for the number-of-float-arguments 
> meta-argument.
>> * I had a bit of trouble generating the right instructions for the 
>> binding interpreter in the following case: if a struct should be 
>> passed in multiple arguments registers but there are insufficient 
>> remaining, then the entire struct should be passed as a copy on the 
>> stack. I tried doing this by allocating a stack VMStorage with the 
>> layout size, and then generating a single Binding.Dereference to copy 
>> the whole struct. This worked in BoxBindingCalculator but not 
>> UnboxBindingCalculator where I have to dereference it eight bytes at 
>> a time (otherwise I get an exception about reading outside the memory 
>> segment). Have a look at UnboxBindingCalculator::spillStruct to see 
>> what I mean...
>
> I think your first solution is causing some undefined behaviour; The 
> argument buffer allocates number-of-stack-storages * STACK_SLOT_SIZE 
> bytes for stack arguments (See the constructor of 
> ProgrammableInvoker). So if you try to pass an argument that is bigger 
> than STACK_SLOT_SIZE with a single binding it's going to read/write 
> out of bounds of the allocated space. I think the fact that it worked 
> in some cases is coincidental.
>
> Your current solution looks good to me (cutting up the struct in to 
> STACK_SLOT_SIZEd pieces and generating a binding for each). Your idea 
> of doing a bulk copy is interesting as well, but I'd like to keep 
> breaking up the arguments into chunks, since this seems easier to 
> intrinsify based on the current JIT code for doing runtime calls.
>
> Cheers,
> Jorn
>
>> Thanks,
>> Nick
>>
>>
>> On 14/10/2019 17:53, Jorn Vernee wrote:
>>> Hi Nick,
>>>
>>> Good to hear that you got it working!
>>>
>>> FTR, I also just generated a diff separately that seems to apply 
>>> cleanly
>>> (module whitespace warnings):
>>> http://cr.openjdk.java.net/~jvernee/prog-back/diff.patch
>>>
>>> (be aware that the code review server seems to be dying a bit 
>>> currently)
>>>
>>> Cheers,
>>> Jorn
>>>
>>> On 14/10/2019 11:47, Nick Gasson wrote:
>>>> Hi Jorn,
>>>>
>>>> Thanks for the hint about git apply --recount. That works modulo the
>>>> two errors below that I fixed by just downloading the raw files.
>>>>
>>>> error: patch failed: src/hotspot/cpu/x86/foreign_globals_x86.hpp:20
>>>> error: src/hotspot/cpu/x86/foreign_globals_x86.hpp: patch does not 
>>>> apply
>>>> error: patch failed: 
>>>> src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp:194
>>>> error: src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp: patch 
>>>> does not apply
>>>> panama.git.patch:4716: new blank line at EOF.
>>>>
>>>>
>>>> Nick
>>>>
>>>> On 14/10/2019 16:56, Jorn Vernee wrote:
>>>>> Hi Nick,
>>>>>
>>>>> Thanks for trying the patch. I'm using Git, so I'm using a port of 
>>>>> the
>>>>> original webrev.ksh script. I've previously done a set of fixes to 
>>>>> the
>>>>> port, since we were having off-by-one errors before, but 
>>>>> apparently not
>>>>> all the bugs were fixed :/ (well, at least I now have a test case).
>>>>>
>>>>> I'll look into a fix, but that's probably gonna take a while. In the
>>>>> mean time it might be fastest to manually fix the line count. Git 
>>>>> also
>>>>> has an option to --recount the number of lines when applying a patch,
>>>>> but I couldn't find anything similar for HG unfortunately.
>>>>>
>>>>> Sorry,
>>>>> Jorn
>>>>>
>>>>> On 14/10/2019 08:19, Nick Gasson wrote:
>>>>>> Hi Jorn,
>>>>>>
>>>>>>> Here is an updated version that gets the build working on GCC:
>>>>>>> cr.openjdk.java.net/~jvernee/prog-back/webrev.03/
>>>>>> I'm having trouble applying the patch file from this webrev (and 
>>>>>> also
>>>>>> the one from webrev.02).
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jvernee/prog-back/webrev.03/panama.git.patch 
>>>>>>
>>>>>>
>>>>>> patching file 
>>>>>> src/hotspot/cpu/aarch64/directUpcallHandler_aarch64.cpp
>>>>>> patching file src/hotspot/cpu/x86/directUpcallHandler_x86.cpp
>>>>>> patching file src/hotspot/cpu/x86/foreign_globals_x86.cpp
>>>>>> patching file src/hotspot/cpu/x86/foreign_globals_x86.hpp
>>>>>> patch: **** malformed patch at line 697: @@ -66,6 +42,38 @@
>>>>>>
>>>>>> Looks like the added/removed line counts are off-by-one?
>>>>>>
>>>>>> Thanks,
>>>>>> Nick