8359435: AArch64: add support for SB instruction to MacroAssembler::spin_wait #25801
Conversation
| 👋 Welcome back eastigeevich! A progress list of the required criteria for merging this PR into |
| @eastig 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: 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 11 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
| Hi @theRealAph , |
Webrevs
|
| I think this can wait until we have a use for |
| The case is to use it for spin pauses instead of
There are discussions regarding using it for spin pauses:
Do you think it is better to have a PR combining this PR and use of SB for spin pauses? |
| BTW Arm published a post in their blog about different implementations of spin pauses: https://community.arm.com/arm-community-blogs/b/architectures-and-processors-blog/posts/multi-threaded-applications-arm |
| FWIW, I don't mind the SB assembler support to go under this, separate PR. We sometimes do it to split the work in the series of atomic commits, where the commit like this should certainly be non-regressing. The actual use of SB (spin-pauses) can then come under separate RFE, and would require much more work (and have associated risk). So, it would be tad less confusing if we had a dependent RFE for using SB in spin pauses, so it was obvious why do we need it. |
Yes, definitely, otherwise we're pushing dead code. Thanks. |
Huh? The least confusing is when the SB support goes in the PR where it is used. That really is obvious, without any dependency chain. |
I am flexible to have it either way. One of the drawbacks of piling up the instruction support and the feature that uses these instructions: if there is ever a second feature that depends on the same instruction support, we would effectively bind two commits (commit A: instruction support + feature A; commit B: feature B) together with an accidental dependency. Which gets extra funky if you ever go with bisects, backouts, backports. Atomic commits rule, and I personally strive to do them, even if there is a window when some code appears dead momentarily. But as I said, I would not quibble here. SB looks like something that we would solely use for spin-wait hints. |
eastig changed the title 
| OutputAnalyzer analyzer = new OutputAnalyzer(pb.start()); | ||
| | ||
| analyzer.shouldHaveExitValue(0); | ||
| if (analyzer.getExitValue() != 0 && "sb".equals(spinWaitInst) && analyzer.contains("CPU does not support SB")) { |
There was a problem hiding this comment.
The logic here is a bit off. Suppose we do have non-zero exit code for, say, isb. This would not fail the test now. Do it something like this instead?
if ("sb".equals(spinWaitInst) && analyzer.contains("CPU does not support SB")) { System.out.println("Skipping the test. The current CPU does not support SB instruction."); return; } analyzer.shouldHaveExitValue(0); There was a problem hiding this comment.
Thank you, Aleksey for finding this. I accidentally removed analyzer.shouldHaveExitValue(0)
| * @run driver compiler.onSpinWait.TestOnSpinWaitAArch64 c2 nop 7 | ||
| * @run driver compiler.onSpinWait.TestOnSpinWaitAArch64 c2 isb 3 | ||
| * @run driver compiler.onSpinWait.TestOnSpinWaitAArch64 c2 yield 1 | ||
| * @run driver compiler.onSpinWait.TestOnSpinWaitAArch64 c2 sb |
There was a problem hiding this comment.
Since we are touching up the test: maybe just say sb 1 explicitly, and then read spinWaitInstCount from args[2] unconditionally?
| Also, merge from mainline to get windows-aarch64 build fix, so that we test things there too. |
…rovided in test cmd options
I'm still waiting for a use for this thing. Then we'll be able to see it in action. |
Do you mean we need real-life workloads relying on |
The other project reports Evgeny linked in PR body look pretty convincing, as well as |
Yes. Again, my apologies. |
| Thank you, Andrew. |
| /integrate |
| Going to push as commit ecd2d83.
Your commit was automatically rebased without conflicts. |
openjdk bot removed ready 
| The modified test is now failing in our CI: Filed: JDK-8360936 |
4 participants
There is data SB-based spin pauses are less disruptive then ISB-based one on them, so performance is better:
There are discussions regarding using it for spin pauses:
Instruction support: https://developer.arm.com/documentation/109697/2025_03/Feature-descriptions/The-Armv8-5-architecture-extension
CPUs supporting it:
Tests:
test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitAArch64.javapassed.test/hotspot/jtreg/compiler/onSpinWait/TestOnSpinWaitNoneAArch64.javapassed.Micro-benchmarks (Graviton 4, c8g.16xlarge (64 CPU), Neoverse-V2):
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25801/head:pull/25801$ git checkout pull/25801Update a local copy of the PR:
$ git checkout pull/25801$ git pull https://git.openjdk.org/jdk.git pull/25801/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25801View PR using the GUI difftool:
$ git pr show -t 25801Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25801.diff
Using Webrev
Link to Webrev Comment