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

8253048: AArch64: When CallLeaf, no need to preserve callee-saved registers in caller #129

Closed
wants to merge 1 commit into from

Conversation

@JoshuaZhuwj
Copy link
Member

@JoshuaZhuwj JoshuaZhuwj commented Sep 11, 2020

I noticed all Floating-Point and SIMD Registers are defined as SOC
registers in c calling convention on AArch64.
As AArch64 ABI tells, the bottom 64 bits of v8-v15 are callee-saved.

When using CallRuntime, with the help of existing flag "exclude_soe" and
function add_call_kills(), SOE registers are killed by the call
because values that could show up in the RegisterMap aren't live in
callee saved registers.
But CallLeaf and CallLeafNoFP are ok because they don't have safepoint
and debug info.

Therefore I submit this patch that aligns save-policy in c calling
convention with AArch64 ABI. It could help eliminate unnecessary SOE
registers spilling in caller across CallLeafNode.

I wrote a simple test case:
http://cr.openjdk.java.net/~jzhu/8253048/Test.java
Original OptoAssembly is:
http://cr.openjdk.java.net/~jzhu/8253048/old_OptoAssembly
With the patch, unnecessary spillings are eliminated:
http://cr.openjdk.java.net/~jzhu/8253048/new_OptoAssembly

And when a vector is alive across CallLeaf, with the help of existing
FatProjectionNode and RA, the whole vector register ( length > 64-bit )
is still spilled to stack as usual.

A test case using VectorAPI is written to verify:
http://cr.openjdk.java.net/~jzhu/8253048/TestVector.java
Test patch:
http://cr.openjdk.java.net/~jzhu/8253048/patch
OptoAssembly dump:
http://cr.openjdk.java.net/~jzhu/8253048/TestVector_OptoAssembly

I also searched all occurrences of "V8-V15" in aarch64 codes.
The stubs for sin/cos don't save v10 before usage.
Therefore I replace it with caller-save register v24.

Jtreg Testing: hotspot_all_no_apps, jdk_core and langtools:tier1

Could you please help review this change?

Best Regards,
Joshua



Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253048: AArch64: When CallLeaf, no need to preserve callee-saved registers in caller

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/129/head:pull/129
$ git checkout pull/129

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 11, 2020

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 11, 2020

@JoshuaZhuwj The following label will be automatically applied to this pull request: hotspot.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 11, 2020

Webrevs

Loading

@JoshuaZhuwj
Copy link
Member Author

@JoshuaZhuwj JoshuaZhuwj commented Sep 11, 2020

/label add hotspot-compiler

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 11, 2020

@JoshuaZhuwj
The hotspot-compiler label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 11, 2020

Mailing list message from Andrew Haley on hotspot-dev:

On 11/09/2020 13:29, Joshua Zhu wrote:

That looks right. I take it that you've checked everywhere else for
misuse of callee-saved vector registers.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 14, 2020

Mailing list message from Joshua Zhu on hotspot-dev:

That looks right. I take it that you've checked everywhere else for misuse of
callee-saved vector registers.

Andrew, Thanks a lot for your review.
Yes. I checked all occurrences of "V8-V15" in aarch64 codes.

Best Regards,
Joshua

Loading

@JoshuaZhuwj
Copy link
Member Author

@JoshuaZhuwj JoshuaZhuwj commented Sep 16, 2020

/reviewer add aph

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@JoshuaZhuwj Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

  • /reviewer credit @openjdk-bot
  • /reviewer credit duke
  • /reviewer credit @user1 @user2

Loading

@JoshuaZhuwj
Copy link
Member Author

@JoshuaZhuwj JoshuaZhuwj commented Sep 16, 2020

/reviewer credit aph

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@JoshuaZhuwj
Reviewer aph successfully credited.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 16, 2020

Mailing list message from Joshua Zhu on hotspot-dev:

That looks right. I take it that you've checked everywhere else for
misuse of callee-saved vector registers.

Andrew, Thanks a lot for your review.
Yes. I checked all occurrences of "V8-V15" in aarch64 codes.

Andrew, can I get this change approved? Thanks.

Best Regards,
Joshua

Loading

@JoshuaZhuwj
Copy link
Member Author

@JoshuaZhuwj JoshuaZhuwj commented Sep 23, 2020

Gentle ping. May I ask reviewers to approve this change?

Loading

adinn
adinn approved these changes Sep 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

@JoshuaZhuwj 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 more details.

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

8253048: AArch64: When CallLeaf, no need to preserve callee-saved registers in caller

Reviewed-by: adinn, aph

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 144 new commits pushed to the master branch:

  • 3320fc0: 8253372: [TESTBUG] update tests which require jvmti - hotspot
  • f765a7f: 8252712: move doclint to jdk.javadoc module
  • c21690b: 8253464: ARM32 Zero: atomic_copy64 is incorrect, breaking volatile stores
  • 0bc01da: 8250635: MethodArityHistogram should use Compile_lock in favour of fancy checks
  • 812b39f: 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary
  • 5f1d612: 8253397: Ensure LogTag types are sorted
  • b8ea80a: 8253457: Remove unimplemented register stack functions
  • e4d0e5a: 8253516: ZGC: Remove card table functions
  • 3fe5886: 8252696: Loop unswitching may cause out of bound array load to be executed
  • 226faa5: 8253241: Update comment on java_suspend_self_with_safepoint_check()
  • ... and 134 more: https://git.openjdk.java.net/jdk/compare/8777ded1238ad195c6b1b63f54fa51984f8c95e4...master

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 (@adinn) 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).

Loading

@openjdk openjdk bot added the ready label Sep 23, 2020
@JoshuaZhuwj
Copy link
Member Author

@JoshuaZhuwj JoshuaZhuwj commented Sep 23, 2020

/integrate

Loading

@openjdk openjdk bot added the sponsor label Sep 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

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

Loading

@JoshuaZhuwj
Copy link
Member Author

@JoshuaZhuwj JoshuaZhuwj commented Sep 23, 2020

Aph and Adinn, Thanks a lot for your review!

Loading

@nsjian
Copy link

@nsjian nsjian commented Sep 24, 2020

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2020

@nsjian @JoshuaZhuwj Since your change was applied there have been 144 commits pushed to the master branch:

  • 3320fc0: 8253372: [TESTBUG] update tests which require jvmti - hotspot
  • f765a7f: 8252712: move doclint to jdk.javadoc module
  • c21690b: 8253464: ARM32 Zero: atomic_copy64 is incorrect, breaking volatile stores
  • 0bc01da: 8250635: MethodArityHistogram should use Compile_lock in favour of fancy checks
  • 812b39f: 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary
  • 5f1d612: 8253397: Ensure LogTag types are sorted
  • b8ea80a: 8253457: Remove unimplemented register stack functions
  • e4d0e5a: 8253516: ZGC: Remove card table functions
  • 3fe5886: 8252696: Loop unswitching may cause out of bound array load to be executed
  • 226faa5: 8253241: Update comment on java_suspend_self_with_safepoint_check()
  • ... and 134 more: https://git.openjdk.java.net/jdk/compare/8777ded1238ad195c6b1b63f54fa51984f8c95e4...master

Your commit was automatically rebased without conflicts.

Pushed as commit ba174af.

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

Loading

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