[foreign] RFR: 8223808: initial port for AArch64

[Maurizio Cimadamore]

This is a solid piece of work - thanks!! Some comments inline:


On 14/05/2019 07:29, Nick Gasson wrote:
> Hi,
>
> A few months ago I asked about a port of the foreign branch for 
> AArch64, and in the last couple of weeks I had some time to do some 
> work on it. I'm sending what I have for review now as I think Dmitry 
> was also planning to start looking at this.
>
> http://cr.openjdk.java.net/~ngasson/foreign/8223808/webrev.0/
>
> With this patch 49/53 tests under test/jdk/java/foreign/ pass on 
> AArch64. The failures are:
>
> java/foreign/StructByValueTest.java
> java/foreign/UnalignedStructTest.java
> java/foreign/Upcall/StructUpcall.java
> java/foreign/LongDoubleTest.java
>
> Currently passing a >16B struct by value doesn't work, because in this 
> case we need to pass a pointer to a copy of the struct in one of the 
> integer registers, but I couldn't immediately figure out how to modify 
> CallingSequenceBuilderImpl to do this. Although I believe this is the 
> same as the Windows x64 ABI (?) so I'll check the implementation there.

I see - yes, the Windows code does that. I think the magic happens here:

http://hg.openjdk.java.net/panama/dev/file/d9ded289d4dd/src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallingSequenceBuilderImpl.java#l150

and here:

http://hg.openjdk.java.net/panama/dev/file/d9ded289d4dd/src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/Windowsx64ABI.java#l125

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)

>
> Returning a struct by value doesn't work yet as we need to pass a 
> pointer to the temporary storage in the "indirect result location 
> register" (r8).

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.

>
> Passing / returning long double doesn't work, I haven't investigated why.

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.


>
> Jextract also builds and runs, but some of the tests fail for the 
> reasons above.
>
> Because I based this patch on the existing code for x86 there's a lot 
> of duplication now, particularly in 
> universalNativeInvoker_aarch64.cpp. I think all of the Shuffle* 
> classes could be moved to some shared code, with very minimal platform 
> specific #ifdefs. Similarly for some of the code in SharedUtils.java. 
> If you're ok with this I can work on it in a separate patch?

Sure, it makes sense to address that separately. The code used to be a 
lot messier than it is now - I'm positively surprised that, following 
recent refactoring of the ABI classes the Java duplication has been 
slashed quite a bit.

The idea of SharedUtils is that it could be shared between all the x64 
abi implementations, and that worked, but now that we're introducing a 
new architecture we're seeing some growing pains, which is part of the 
game to et the implementation into a better state.

As we've done for windows, is perfectly reasonable to duplicate now and 
to clean up later.

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?)

Maurizio

>
>
> Thanks,
> Nick