8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API #754

swesonga · 2022-11-22T19:01:32Z

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

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API

Reviewers

Jorn Vernee (@JornVernee - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-foreign pull/754/head:pull/754
$ git checkout pull/754

Update a local copy of the PR:
$ git checkout pull/754
$ git pull https://git.openjdk.org/panama-foreign pull/754/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 754

View PR using the GUI difftool:
$ git pr show -t 754

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-foreign/pull/754.diff

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

bridgekeeper · 2022-11-22T19:03:13Z

👋 Welcome back swesonga! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-11-22T19:08:06Z

Webrevs

02: Full (ccae2d80)
01: Full (cd0ccdd5)
00: Full (7139289a)

JornVernee

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.

test/jdk/java/foreign/callarranger/TestAarch64CallArranger.java

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

test/jdk/java/foreign/TestVarArgs.java

JornVernee · 2022-11-28T13:56:59Z

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

+                    boolean spillRegistersPartially = forVariadicFunctionArgs && spillsVariadicStructsPartially();
+                    regs = spillRegistersPartially ?
+                        storageCalculator.regAllocPartial(StorageType.INTEGER, layout) :
+                        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

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

JornVernee · 2022-12-02T20:04:09Z

@swesonga I applied some fixes to test/jdk/java/foreign/TestMatrix.java 1 that were merged into this repo today. You might want to merge the latest foreign-memaccess+abi branch into this PR branch to get those changes as well.

JornVernee

Few more comments inline.

FWIW, I think CallArranger can be cleaned up further, and I've been playing with a patch 1. But I'll file a followup PR for that.

src/java.base/share/classes/jdk/internal/foreign/abi/LinkerOptions.java

test/jdk/java/foreign/valist/VaListTest.java

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

swesonga · 2023-01-10T17:47:31Z

Few more comments inline.

FWIW, I think CallArranger can be cleaned up further, and I've been playing with a patch 1. But I'll file a followup PR for that.

@JornVernee just to clarify, do you expect more refactoring of the CallArranger in this PR or will that be subsumed by your patch?

JornVernee · 2023-01-10T18:04:58Z

@swesonga No, I think this PR is good. I'll file a followup PR myself for the other refactoring I did.

openjdk · 2023-01-10T18:07:56Z

@swesonga This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API

Reviewed-by: jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the foreign-memaccess+abi branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JornVernee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

swesonga · 2023-01-11T03:59:56Z

/integrate

openjdk · 2023-01-11T04:02:17Z

@swesonga
Your change (at version ccae2d8) is now ready to be sponsored by a Committer.

JornVernee · 2023-01-11T04:22:27Z

/sponsor

openjdk · 2023-01-11T04:23:45Z

Going to push as commit 08225e4.

openjdk · 2023-01-11T04:24:12Z

@JornVernee @swesonga Pushed as commit 08225e4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JornVernee · 2023-02-14T13:41:01Z

@swesonga Hey, just a heads up. It is about that time again where we are looking to put together a PR for the JEP to go into mainline.

Since we do not support Windows/AArch64 (e.g. we can not test it), we can not be in charge of a PR that brings this port over to mainline. So, I think it's up to you or another maintainer of Windows/AArch64.

I suggest taking the following two commits:

And making a PR against openjdk/jdk out of those (since those three are all related to this port). I can approve it again there, since I've already reviewed the changes before.

swesonga · 2023-02-27T17:06:33Z

@swesonga Hey, just a heads up. It is about that time again where we are looking to put together a PR for the JEP to go into mainline.

Since we do not support Windows/AArch64 (e.g. we can not test it), we can not be in charge of a PR that brings this port over to mainline. So, I think it's up to you or another maintainer of Windows/AArch64.

I suggest taking the following two commits:

d379ca1

08225e4

And making a PR against openjdk/jdk out of those (since those three are all related to this port). I can approve it again there, since I've already reviewed the changes before.

@JornVernee I've opened openjdk/jdk#12773

openjdk bot added the rfr Ready for review label Nov 22, 2022

JornVernee reviewed Nov 28, 2022

View reviewed changes

This was referenced Nov 29, 2022

Update Foreign Function & Memory API to work with Windows AArch64 ABI swesonga/jdk#1

Closed

Move Linux & MacOs CallArranger tests into separate files #755

Closed

swesonga added 8 commits December 6, 2022 06:33

Move Windows CallArranger tests into separate file

1c12be8

Remove unnecessary null check

3580e12

Remove toSessionImpl method

52da2e8

Move storage decisions into StorageCalculator

d882168

Simplify newly added tests in TestVarArgs

3d2f4e2

Merge branch 'foreign-memaccess+abi' into WindowsAArch64ABI

385295d

Update Windows FFM implementation to match latest Preview API

08fdbf2

Merge branch 'foreign-memaccess+abi' into WindowsAArch64ABI

cd0ccdd

JornVernee reviewed Jan 6, 2023

View reviewed changes

swesonga added 3 commits January 10, 2023 07:55

Merge branch 'foreign-memaccess+abi' into WindowsAArch64ABI

121ddc6

Delete VaList

b9ccd3b

Remove redundant check

ccae2d8

JornVernee approved these changes Jan 10, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 10, 2023

openjdk bot added the sponsor Ready to sponsor label Jan 11, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 11, 2023

openjdk bot closed this Jan 11, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Jan 11, 2023

swesonga mentioned this pull request Feb 27, 2023

8303409: Add Windows AArch64 ABI support to the Foreign Function & Memory API openjdk/jdk#12773

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API #754

8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API #754

swesonga commented Nov 22, 2022 •

edited by openjdk bot

bridgekeeper bot commented Nov 22, 2022

mlbridge bot commented Nov 22, 2022 •

edited

JornVernee left a comment

JornVernee Nov 28, 2022

JornVernee commented Dec 2, 2022

JornVernee left a comment

swesonga commented Jan 10, 2023

JornVernee commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

swesonga commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

JornVernee commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

JornVernee commented Feb 14, 2023

swesonga commented Feb 27, 2023

8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API #754

8295290: Add Windows ARM64 ABI support to the Foreign Function & Memory API #754

Conversation

swesonga commented Nov 22, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 22, 2022

mlbridge bot commented Nov 22, 2022 • edited

Webrevs

JornVernee left a comment

Choose a reason for hiding this comment

JornVernee Nov 28, 2022

Choose a reason for hiding this comment

JornVernee commented Dec 2, 2022

JornVernee left a comment

Choose a reason for hiding this comment

swesonga commented Jan 10, 2023

JornVernee commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

swesonga commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

JornVernee commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

JornVernee commented Feb 14, 2023

swesonga commented Feb 27, 2023

swesonga commented Nov 22, 2022 •

edited by openjdk bot

mlbridge bot commented Nov 22, 2022 •

edited