[foreign-memaccess+abi] RFR: 8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API

[Jorn Vernee]

On Tue, 22 Nov 2022 19:01:32 GMT, Saint Wesonga <duke at openjdk.org> wrote:

> There are 2 primary differences between the Windows ARM64 ABI and the macOS/Linux ARM64 ABI: variadic floating point arguments are passed in general purpose registers on Windows (instead of the vector registers). In addition to this, up to 64 bytes of a struct being passed to a variadic function can be placed in general purpose registers. This happens regardless of the type of struct (HFA or other generic struct). This means that a struct can be split across registers and the stack when invoking a variadic function.
> 
> This change introduces tests that compute the sum of the fields of structs containing 1-4 ints, floats, and doubles to verify that each field is correctly assigned a register or stack location when invoking a variadic function (both when the struct can be passed entirely in registers as well as when the struct spills onto the stack).
> 
> For details about the Foreign Function & Memory API, see JEP 434 defined at https://openjdk.org/jeps/434
> 
> The Windows ARM64 ABI conventions are documented at https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions
> 
> This work builds on @lewurm / Bernhard's branch at https://github.com/lewurm/openjdk/commits/foreign-windows-aarch64

This looks like a really good start, but there are few things to be done still, I think.

Note also that I'm not able to test this on Windows/AArch64, though I've run `jdk_foreign` tests on all the other supported platforms. Some tests having sampling applied, so it is important to run tests with `-Dgenerator.sample.factor=1`. When running tests through `make` this can be done by using `JTREG="JAVA_OPTIONS=-Dgenerator.sample.factor=1"`.

We also have the test `java/foreign/TestMatrix.java` which runs several different configurations. It's a manual test that has to be run through `jtreg` directly. A comment at the top of that file contains instructions to do that. It's also a good idea to run that test (and it isn't run when using `make`). This test also fully runs all the sampled tests at, except for TestVarArgs (we should probably add a cases for that...).

Lastly, eventually this patch will have to be brought over to the mainline JDK repo as well. I suggest you take charge of that as well, since you can do the testing on Windows/AArch64. It might also be easier to straight up integrate things into the mainline JDK repo after the changes for the JDK 20 go into that repo [1], [2] (will likely be somewhere in the next 2 weeks). After that the code is again merged into this repo, and both repos should contain the same code any ways. (There might be some changes needed to the code as well, due to naming changes).

But, we can get started on the review here already.

[1]: https://github.com/openjdk/jdk/pull/10872
[2]: https://github.com/openjdk/jdk/pull/11019

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

> 263: 
> 264:         int requiredRegisters(MemoryLayout layout) {
> 265:             return (layout == null) ? 0 : (int)Utils.alignUp(layout.byteSize(), 8) / 8;

AFAICS `layout` should never be `null`? If it is `null`, that seems like a bug, so it would be better to fail with an NPE than silently return `0`.

i.e. I think this `null` check should be removed.

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

> 415:                     regs = spillRegistersPartially ?
> 416:                         storageCalculator.regAllocPartial(StorageType.INTEGER, layout) :
> 417:                         storageCalculator.regAlloc(StorageType.INTEGER, layout);

I think ideally this decision making should be pushed down into `StorageCalculator`/`BindingCalculator` if possible.

It seems like it should be possible to have a single `alloc` method that returns a `VMStorage[]` with a mix of register and stack storages that is used by the while loop below.

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

> 423:                             final long copy = Math.min(layout.byteSize() - offset, 8);
> 424:                             VMStorage storage = regs[regIndex++];
> 425:                             boolean useFloat = (!useIntRegsForFloatingPointArgs) && storage.type() == StorageType.VECTOR;

Same here, should be handled by `StorageCalculator::regAlloc` I think.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64VaList.java line 147:

> 145:     public void skip(MemoryLayout... layouts) {
> 146:         Objects.requireNonNull(layouts);
> 147:         MemorySessionImpl.toSessionImpl(segment.session()).checkValidState();

Calls to `toSessionImpl` can be replaced with simple cast. We do that in the JEP patch as well, but those changes haven't made their way back into this repo yet.
Suggestion:

        ((MemorySessionImpl) segment.session()).checkValidState();

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64VaList.java line 174:

> 172:     @Override
> 173:     public VaList copy() {
> 174:         MemorySessionImpl.toSessionImpl(segment.session()).checkValidState();

Suggestion:

        ((MemorySessionImpl) segment.session()).checkValidState();

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64VaList.java line 189:

> 187: 
> 188:         public Builder(MemorySession session) {
> 189:             MemorySessionImpl.toSessionImpl(session).checkValidState();

Suggestion:

            ((MemorySessionImpl) segment.session()).checkValidState();

test/jdk/java/foreign/TestVarArgs.java line 131:

> 129: 
> 130:     @Test(dataProvider = "variadicStructDescriptions")
> 131:     public void testSumVariadicHfa(StructFieldType structFieldType, int extraIntArgs, MemorySegment foreignFunctionSymbol) throws Throwable {

Instead of adding another test, I'd much rather see the existing test being extended with a few extra test cases. Note also that the structs with 1/2/3 fields are already being tested by that test, so it does not need to be tested again.

You can declare a new data provider that calls the `functions` data provider and then adds a few extra cases.

test/jdk/java/foreign/callarranger/TestAarch64CallArranger.java line 648:

> 646:     }
> 647: 
> 648:     @Test

Since there are quite a few new test cases here, I suggest moving them to a separate WindowsAArch64 specific file.

(we should probably do the same for the mac tests in the long run).

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

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