[foreign-abi] On invokers
Jorn Vernee
jorn.vernee at oracle.com
Wed Oct 23 11:34:37 UTC 2019
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
More information about the panama-dev
mailing list