[foreign-memaccess+abi] RFR: 8275584: Incorrect stack spilling in CallArranger on MacOS/AArch64 [v2]

[Jorn Vernee]

On Mon, 24 Oct 2022 13:08:54 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

>> This patch adds special handling for argument spilling on M1 Mac which does not follow the standard AArch64.  In the standard ABI arguments are first extended to the full 64-bit register width and then spilled but on macOS the arguments are spilled according to their original width and packed using theior natural.  @JornVernee  did most of the work to support this but there were a few issues remaining related to structspilling which I've fixed up in the last commit here.
>
> Nick Gasson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove sign-extension XXX

Thanks again for fixing.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 197:

> 195:             long stackSlotAlignment = subSlotPacking && !forVarArgs
> 196:                     ? alignment
> 197:                     : Math.max(alignment, STACK_SLOT_SIZE);

With spillStruct(Un)box, and `nextStorageForHFA` now also accounting for sub slot packing, I think that maybe this is not the right level to adjust the alignment (again).

I suggest removing the conditional expression here, and simply using the `alignment` passed to this method in place of `stackSlotAlignment`. The alignment can be adjusted based on `requiresSubSlotStackPacking()` in the other  `stackAlloc(MemorryLayout)` overload instead, which looks to be the only place that doesn't pass a constant boolean value. Then, `nextStorageForHFA` can also just call that overload for each member layout.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 290:

> 288:                 long copy = Math.min(layout.byteSize() - offset, STACK_SLOT_SIZE);
> 289:                 VMStorage storage =
> 290:                     storageCalculator.stackAlloc(copy, layout.byteAlignment(), false);

Ok. So it looks like only HFAs are 'packed', and other struck follow the classic approach of passing everything in stack slot sized chunks :/

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

Marked as reviewed by jvernee (Committer).

PR: https://git.openjdk.org/panama-foreign/pull/746