[foreign] RFR: 8223808: initial port for AArch64

[Jorn Vernee]

>> This is probably similar to what happens in SystemV ABI - where biger 
>> structs are returned with an indirection pointer stored in the first 
>> integer register. For this we have CallingSequence::returnsInMemory 
>> and CalingSequence::returnInMemoryBindings. It is possible that this 
>> logic might require some generalization to work with custom registers 
>> - the current code assumes that the bindings for the return-in-memory 
>> should be created with position "-1" (return) by the builder, and 
>> given the class "INTEGER_ARGUMENT_REGISTER", see
>> 
>> http://hg.openjdk.java.net/panama/dev/file/d9ded289d4dd/src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequence.java#l70 
>> Which might or might not be enough for you.
> 
> This is really useful, thanks. Maybe it's enough to add this as
> special case in StorageCalculator::addBindings where
> forArguments==false, argumentIndex()==-1 and the argument class is
> INTEGER: then we can always allocate r8 for this argument. I'll
> experiment a bit.

I think ShuffleRecipe::make is currently dependent on the argument 
bindings for each class occurring in the order of their storage indices 
to generate skips. And CallingSequenceBuilder always adds the return 
binding first: 
http://hg.openjdk.java.net/panama/dev/file/d9ded289d4dd/src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java#l54

If r8 is the first integer argument register this would work, but this 
doesn't seem to be the case? I think a quick-fix for this is adding a 
sorting pass of the bindings by storage index to 
CallingSequenceBuilder::build.

Jorn

Nick Gasson schreef op 2019-05-14 12:15:
> Hi Maurizio,
> 
> On 14/05/2019 16:32, Maurizio Cimadamore wrote:
>> 
>> I see - yes, the Windows code does that. I think the magic happens 
>> here:
>> 
>> [...]
>> 
>> That is, if the struct has certain charateristics (e.g. smaller than 
>> 64 bits), Windows pass it in register, otherwise it always pass it by 
>> reference (see the 'else' in the ABI::unbox code which creates a 
>> pointer).
>> 
>> (actually, now that I look at it, it's not clear to me why Windows's 
>> CallingSequenceBuilder is using ArgumentClass.INTEGER instead of 
>> ArgumentClass.POINTER for these cases)
> 
> Thanks! I'd found the part where it assigns ArgumentClass.INTEGER for
> these arguments in Windows CallingSequenceBuilderImpl but missed the
> corresponding code in box/unboxValue. I think we can use the same
> approach on AArch64.
> 
>> 
>> This is probably similar to what happens in SystemV ABI - where biger 
>> structs are returned with an indirection pointer stored in the first 
>> integer register. For this we have CallingSequence::returnsInMemory 
>> and CalingSequence::returnInMemoryBindings. It is possible that this 
>> logic might require some generalization to work with custom registers 
>> - the current code assumes that the bindings for the return-in-memory 
>> should be created with position "-1" (return) by the builder, and 
>> given the class "INTEGER_ARGUMENT_REGISTER", see
>> 
>> http://hg.openjdk.java.net/panama/dev/file/d9ded289d4dd/src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequence.java#l70 
>> Which might or might not be enough for you.
> 
> This is really useful, thanks. Maybe it's enough to add this as
> special case in StorageCalculator::addBindings where
> forArguments==false, argumentIndex()==-1 and the argument class is
> INTEGER: then we can always allocate r8 for this argument. I'll
> experiment a bit.
> 
>> 
>> long double is currently only working on SystemV - and the underlying 
>> implementation heavily relies on Intel x87 registers and instructions; 
>> note that in SystemV a 'long double' maps onto extended precision IEEE 
>> format (80 bits) while in AArch64 it maps onto quad precision IEEE 
>> format (128 bits).
>> 
>> Which means classification is probably not working correctly on 
>> AArch64 and the LongDouble LayoutType/Reference implementation in the 
>> binder need some work too, since they assume 80-bit extended format.
>> 
> 
> OK, thanks again for the hints!
> 
>> 
>> Process note/question, our internal systems test Windows x64, Linux 
>> x64 and MacOS (x64) - once the patch is ready to be integrated, how do 
>> we plan to make sure that we're not accidentally introducing AArch64 
>> regressions (given that our nighties will be oblivious to that?)
>> 
> 
> We have an Arm-internal CI that we can configure to run nightly tests
> on the foreign branch (and the vectorIntrinsics branch too, as we're
> working on the AArch64 port of that). There's still a manual step of
> us posting to this list or sending a patch when something breaks
> though. The unit test for AArch64 CallingSequenceBuilder should run on
> x86 too.
> 
> 
> Nick