[lworld] RFR: 8266890: [lworld] [AArch64] add support for InlineTypePassFieldsAsArgs

[Tobias Hartmann]

On Fri, 21 May 2021 10:40:29 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

> This patch implements InlineTypePassFieldsAsArgs on AArch64 and the
> associated stack extension/repair mechanism. It mostly follows the x86
> implementation closely except for how the stack increment is stored in
> the callee frame. On x86 the sp_inc stack slot stores the total size of
> the frame which is the sum of the extension space, return address copy,
> and the original method frame size (i.e. the total bytes needed to pop
> the frame). On AArch64 we just store the size of the extension space. I
> did it this way because it simplifies the stack repair code in
> MacroAssembler::remove_frame (I've added some notes there to document
> this). I don't think this should cause any problem because only the
> MacroAssembler and platform-dependant frame walking code need to be
> aware of this.
> 
> This patch includes JDK-8266609 which is a small refactoring in mainline
> JDK to simplify how the frame size is passed around in the AArch64 C1
> backend.
> 
> There was some X86 specific code in unpack_inline_args() in
> macroAssembler_common.cpp. I've split this out into an arch-dependant
> MacroAssembler::extend_stack_for_inline_args().
> 
> MacroAssembler::pack_inline_helper() and shuffle_inline_args() now take
> a new Register val_array argument which is the register holding the
> buffered oop array for packing. Previously it assumed this was already
> loaded in RAX (x86) or x20 (AArch64) but IMO passing it in explicitly
> makes the code easier to understand.
> 
> There is one new test failure: TestNullableInlineTypes.java. The test
> seems functionally correct but there is a failure verifying the C2 IR
> graph (see below). I haven't investigated this but the test enables
> InlineTypePassFieldsAsArgs which we don't support yet on AArch64 so that
> might be causing the failure.
> 
> 
> Exception in thread "main" java.lang.RuntimeException: Graph for 'TestNullableInlineTypes::test26' contains forbidden node:
> 83  StoreL  ===  176  177  84  14  [[ 89 ]]  @compiler/valhalla/inlinetypes/MyValue3:exact+16 *, name=l, idx=6;  Memory: @rawptr:BotPTR, idx=Raw; !orig=82 !jvms: TestNullableInlineTypes::test26 @ bci:13 (line 704): expected false, was true
> 
> 
> Tested tier1, runtime/valhalla, and compiler/valhalla on x86 and AArch64. There are some "bad AD file" failures on x86 but don't seem to be related to this patch.

Sorry for the delay, this looks good to me!

>  I did it this way because it simplifies the stack repair code in MacroAssembler::remove_frame

But that only simplifies the C++ code and not the assembly code that is emitted right? I wonder if we shouldn't aim for optimal assembly because this code will be exercised a lot.

Could you please file bugs for the other issues that you were seeing?

We currently don't have testing on aarch64 enabled for Valhalla in our CI but we will do so as soon as the port is stable.

-------------

Marked as reviewed by thartmann (Committer).

PR: https://git.openjdk.java.net/valhalla/pull/420