Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266890: [lworld] [AArch64] add support for InlineTypePassFieldsAsArgs #420

Closed
wants to merge 2 commits into from

Conversation

nick-arm
Copy link
Member

@nick-arm nick-arm commented May 21, 2021

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.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8266890: [lworld] [AArch64] add support for InlineTypePassFieldsAsArgs

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/420/head:pull/420
$ git checkout pull/420

Update a local copy of the PR:
$ git checkout pull/420
$ git pull https://git.openjdk.java.net/valhalla pull/420/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 420

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/valhalla/pull/420.diff

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
@bridgekeeper
Copy link

bridgekeeper bot commented May 21, 2021

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

@openjdk
Copy link

openjdk bot commented May 21, 2021

@nick-arm 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:

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

Reviewed-by: thartmann

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 6 new commits pushed to the lworld branch:

  • 25983c7: 8267698: [lworld] [lw3] runtime\exceptionMsgs\NullPointerException\NullPointerExceptionTest.java test fails because of new error message
  • 340ecec: 8267559: [lworld] [lw3] NPE thrown when attempting to write null to a null-free array has incorrect error message
  • 7e2f202: 8267672: [lworld] A couple of cleanups to Unified class file generation scheme
  • e150bd9: 8267542: [lworld] [lqagain] javac emits useless checkcast when accessing fields through the reference projection
  • f9806a4: 8266466: [lworld] Enhance javac to consume unified primitive class files generated under the option -XDunifiedValRefClass
  • ae4ce74: 8267503: [lworld] [lw3] NPE message thrown by checkcast is incorrect

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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 (@TobiHartmann) 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).

@mlbridge
Copy link

mlbridge bot commented May 21, 2021

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

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.

@nick-arm
Copy link
Member Author

nick-arm commented May 27, 2021

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.

Yes. I thought about it a bit more and I've modified it slightly so the sp_inc stack slot stores total_frame_size - 2*wordSize which is more like x86 and also saves one instruction when removing the frame (see the last commit
d9fa572).

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

Sure. Do you have any ideas about this one? https://bugs.openjdk.java.net/browse/JDK-8264340

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me.

Sure. Do you have any ideas about this one? https://bugs.openjdk.java.net/browse/JDK-8264340

That doesn't ring a bell. Is it reproducible?

@nick-arm
Copy link
Member Author

nick-arm commented May 31, 2021

Sure. Do you have any ideas about this one? https://bugs.openjdk.java.net/browse/JDK-8264340

That doesn't ring a bell. Is it reproducible?

Yes, it always fails in that way on AArch64. It seems to be related to scheduling: I just ran it on x86 with -XX:+UseOptoScheduling and it also fails.

@nick-arm
Copy link
Member Author

nick-arm commented May 31, 2021

/integrate

@openjdk openjdk bot added the sponsor label May 31, 2021
@openjdk
Copy link

openjdk bot commented May 31, 2021

@nick-arm
Your change (at version d9fa572) is now ready to be sponsored by a Committer.

@TobiHartmann
Copy link
Member

TobiHartmann commented May 31, 2021

/sponsor

@TobiHartmann
Copy link
Member

TobiHartmann commented May 31, 2021

Yes, it always fails in that way on AArch64. It seems to be related to scheduling: I just ran it on x86 with -XX:+UseOptoScheduling and it also fails.

Okay, thanks for the details!

@openjdk
Copy link

openjdk bot commented May 31, 2021

@TobiHartmann @nick-arm Since your change was applied there have been 7 commits pushed to the lworld branch:

  • 0f1c33c: 8267710: [lworld][lw3] Hook AlwaysAtomicAccesses to primitive classes atomicity rules
  • 25983c7: 8267698: [lworld] [lw3] runtime\exceptionMsgs\NullPointerException\NullPointerExceptionTest.java test fails because of new error message
  • 340ecec: 8267559: [lworld] [lw3] NPE thrown when attempting to write null to a null-free array has incorrect error message
  • 7e2f202: 8267672: [lworld] A couple of cleanups to Unified class file generation scheme
  • e150bd9: 8267542: [lworld] [lqagain] javac emits useless checkcast when accessing fields through the reference projection
  • f9806a4: 8266466: [lworld] Enhance javac to consume unified primitive class files generated under the option -XDunifiedValRefClass
  • ae4ce74: 8267503: [lworld] [lw3] NPE message thrown by checkcast is incorrect

Your commit was automatically rebased without conflicts.

Pushed as commit 0871590.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants