8357267: ZGC: Handle APX EGPRs spilling in ZRuntimeCallSpill #25351

jatin-bhateja · 2025-05-21T12:33:26Z

Patch spills APX EGPRs across runtime calls to slow-path barriers using PUSH2P/POP2 instructions with PPX hints.
These instructions operate over a pair of registers resulting into an smaller save/restoration JIT code, on the hind side they have hard alignment and balancing constraints, as they operate over 16-byte aligned stack address.
ZRuntimeCallSpill is agnostic to live register, thus resulting SPILL sequence should not modify the contents of the register.

Patch has been verified using Intel SDE all test under test/hotspot/jtreg/compiler/gcbarriers are green.

Kindly review and share your feedback.

Best Regards,
Jatin

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8357267: ZGC: Handle APX EGPRs spilling in ZRuntimeCallSpill (Enhancement - P4)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) Review applies to 9b5c2ac4
Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25351/head:pull/25351
$ git checkout pull/25351

Update a local copy of the PR:
$ git checkout pull/25351
$ git pull https://git.openjdk.org/jdk.git pull/25351/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25351

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25351.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-21T12:34:14Z

👋 Welcome back jbhateja! 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.

openjdk · 2025-05-21T12:35:04Z

@jatin-bhateja 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:

8357267: ZGC: Handle APX EGPRs spilling in ZRuntimeCallSpill

Reviewed-by: rcastanedalo, sviswanathan

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

85af573: 8356848: Separate Metaspace and GC printing
78d0dc7: 8357460: RISC-V: Optimize array fill stub for small size
37d04a1: 8355512: Test compiler/vectorization/TestVectorZeroCount.java times out with -XX:TieredStopAtLevel=3
... and 230 more: https://git.openjdk.org/jdk/compare/03dca0323d79ef5fb1c8ee1152667e2188fa5e01...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-05-21T12:35:35Z

@jatin-bhateja 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 pull request command.

jatin-bhateja · 2025-05-21T14:01:37Z

/label add hotspot-compiler-dev

openjdk · 2025-05-21T14:02:50Z

@jatin-bhateja
The hotspot-compiler label was successfully added.

openjdk · 2025-05-21T14:03:18Z

@jatin-bhateja Unknown command lable - for a list of valid commands use /help.

jatin-bhateja · 2025-05-21T14:03:55Z

/label add hotspot-gc-dev

openjdk · 2025-05-21T14:04:01Z

@jatin-bhateja
The hotspot-gc label was successfully added.

mlbridge · 2025-05-21T14:09:07Z

Webrevs

sviswa7 · 2025-05-21T22:17:18Z

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp

+      // Push original stack pointer along with remaining registers on 16B aligned stack.
+      // Note: For PPX to work properly, a PPX-marked PUSH2 (respectively, POP2) should always
+      // be matched with a PPX-marked POP2 (PUSH2), not with two PPX-marked POPs (PUSHs).
+      __ pushp(rcx);


This is saving old rsp on stack and restored using __ movptr(rsp, Address(rsp)) on the other end in restore(). So this should be __ push(rcx) and not __ pushp(rcx) as there is no corresponding __ popp() instruction for this pushp.

sviswa7 · 2025-05-21T22:19:46Z

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp

-    __ push(r11);
+    if (VM_Version::supports_apx_f()) {
+      __ push(rax);
+      __ push(rcx);


This could be __ pushp(rcx).

sviswa7 · 2025-05-21T22:20:32Z

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp

+      __ popp(rdx);
+      // Re-instantiate original stack pointer.
+      __ movptr(rsp, Address(rsp));
+      __ pop(rcx);


This could be __ popp(rcx).

sviswa7 · 2025-05-21T22:26:38Z

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp

-    __ push(r10);
-    __ push(r11);
+    if (VM_Version::supports_apx_f()) {
+      __ push(rax);


if _result is not equal to rax this also could be pushp rax here and popp rax in restore().

sviswa7 · 2025-05-21T23:33:47Z

@xmas92 Could you please also take a look at this? Intel APX add additional GPR registers (R16 - R31). Our understanding is that these also need to be saved and restored as part of ZRuntimeCallSpill. Is that correct?

xmas92 · 2025-05-22T06:51:47Z

Intel APX add additional GPR registers (R16 - R31). Our understanding is that these also need to be saved and restored as part of ZRuntimeCallSpill. Is that correct?

ZRuntimeCallSpill is used when doing calls into libjvm from contexts where we do not track the liveness of the registers. So all caller saved registers must be saved and restored. If all APX registers are caller saved, then yes this is correct.

robcasloz · 2025-05-22T09:57:47Z

Patch has been verified using Intel SDE all test under test/hotspot/jtreg/compiler/gcbarriers are green.

Have you checked that these tests exercise ZRuntimeCallSpill significantly? Most tests in that directory seem to exercise C2's generated ZGC barriers, which use other spilling/restoring logic across runtime calls (SaveLiveRegisters). Also, I expect the register pressure in these test cases to be minimal, so it could be good to randomize register assignment to improve the testing effectiveness. Finally, ZRuntimeCallSpill is typically used in slow paths, which are rarely exercised in short-lived test cases. Have you considered altering the users of ZRuntimeCallSpill so that they are forced to always, or at least more often, enter the slow path, for testing purposes? This PR did something similar in the context of C2 ZGC barriers.

jatin-bhateja · 2025-05-22T13:30:28Z

Patch has been verified using Intel SDE all test under test/hotspot/jtreg/compiler/gcbarriers are green.

Have you checked that these tests exercise ZRuntimeCallSpill significantly? Most tests in that directory seem to exercise C2's generated ZGC barriers, which use other spilling/restoring logic across runtime calls (SaveLiveRegisters). Also, I expect the register pressure in these test cases to be minimal, so it could be good to randomize register assignment to improve the testing effectiveness. Finally, ZRuntimeCallSpill is typically used in slow paths, which are rarely exercised in short-lived test cases. Have you considered altering the users of ZRuntimeCallSpill so that they are forced to always, or at least more often, enter the slow path, for testing purposes? This PR did something similar in the context of C2 ZGC barriers.

Intel SDE allows us to collect execution traces with -itrace_execute_emulate and we found quite a lot of register save/ restorations around native method, there is already an existing test point for it
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/gcbarriers/UnsafeIntrinsicsTest.java

sviswa7

Looks good to me.

robcasloz · 2025-05-23T13:53:23Z

Patch has been verified using Intel SDE all test under test/hotspot/jtreg/compiler/gcbarriers are green.

Have you checked that these tests exercise ZRuntimeCallSpill significantly? Most tests in that directory seem to exercise C2's generated ZGC barriers, which use other spilling/restoring logic across runtime calls (SaveLiveRegisters). Also, I expect the register pressure in these test cases to be minimal, so it could be good to randomize register assignment to improve the testing effectiveness. Finally, ZRuntimeCallSpill is typically used in slow paths, which are rarely exercised in short-lived test cases. Have you considered altering the users of ZRuntimeCallSpill so that they are forced to always, or at least more often, enter the slow path, for testing purposes? This PR did something similar in the context of C2 ZGC barriers.

Intel SDE allows us to collect execution traces with -itrace_execute_emulate and we found quite a lot of register save/ restorations around native method, there is already an existing test point for it https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/gcbarriers/UnsafeIntrinsicsTest.java

OK, thanks for checking Jatin!

Have you also checked whether, at least in some of the cases, some of the APX EGPRs are live across the runtime call (i.e. are defined before the call and used after the call), and whether the called runtime routine typically clobbers these registers? Knowing that this case is exercised in the test runs would be good to be confident about the correctness of the patch.

jatin-bhateja · 2025-05-26T12:00:38Z

Patch has been verified using Intel SDE all test under test/hotspot/jtreg/compiler/gcbarriers are green.

Have you checked that these tests exercise ZRuntimeCallSpill significantly? Most tests in that directory seem to exercise C2's generated ZGC barriers, which use other spilling/restoring logic across runtime calls (SaveLiveRegisters). Also, I expect the register pressure in these test cases to be minimal, so it could be good to randomize register assignment to improve the testing effectiveness. Finally, ZRuntimeCallSpill is typically used in slow paths, which are rarely exercised in short-lived test cases. Have you considered altering the users of ZRuntimeCallSpill so that they are forced to always, or at least more often, enter the slow path, for testing purposes? This PR did something similar in the context of C2 ZGC barriers.

Intel SDE allows us to collect execution traces with -itrace_execute_emulate and we found quite a lot of register save/ restorations around native method, there is already an existing test point for it https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/gcbarriers/UnsafeIntrinsicsTest.java

OK, thanks for checking Jatin!

Have you also checked whether, at least in some of the cases, some of the APX EGPRs are live across the runtime call (i.e. are defined before the call and used after the call), and whether the called runtime routine typically clobbers these registers? Knowing that this case is exercised in the test runs would be good to be confident about the correctness of the patch.

Hi @robcasloz,
The patch uses new push2/pop2 instructions, which reduces dynamic instruction count needed to save and restore all the caller-saved registers. New instruction sequence based on push2/pop2 not only saves EGPRs but also existing GPRs with shorter JIT sequence. We verified our fix using the following standalone gtest with the Intel Software Development Emulator.

test_ZRuntimeCallSpill_cpp.txt

Given that gtests is a build-time validation and the JVM itself is built with with minimum feature set, hence am hesitant to add this along with the patch. BTW, ZRuntimeCallSpill is called as part of the slow path barrier for native methods, which can modify EGPRs.

Let me know if you think it's good to land in.

xmas92

Seems fine. Eventually it would be nice if we could generalise this and have the logic in the MacroAssembler.

Just a small comment about the conditional rax push and pop.

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp

robcasloz · 2025-05-26T14:05:42Z

Hi @robcasloz, The patch uses new push2/pop2 instructions, which reduces dynamic instruction count needed to save and restore all the caller-saved registers. New instruction sequence based on push2/pop2 not only saves EGPRs but also existing GPRs with shorter JIT sequence. We verified our fix using the following standalone gtest with the Intel Software Development Emulator.

test_ZRuntimeCallSpill_cpp.txt

Given that gtests is a build-time validation and the JVM itself is built with with minimum feature set, hence am hesitant to add this along with the patch. BTW, ZRuntimeCallSpill is called as part of the slow path barrier for native methods, which can modify EGPRs.

Let me know if you think it's good to land in.

Thanks for the details! Let me run some internal testing, since the PR affects spilling of non-extended registers too (due to special handling of _result == rax). Will come back with the results within a day or two.

robcasloz

Test results look good!

jatin-bhateja · 2025-05-27T08:27:45Z

Thanks @robcasloz , @xmas92 and @sviswa7 for your reviews and approvals.

jatin-bhateja · 2025-05-27T08:27:53Z

/integrate

openjdk · 2025-05-27T08:28:56Z

Going to push as commit 5924c2d.
Since your change was applied there have been 234 commits pushed to the master branch:

7bc315f: 8355585: Aarch64: Add aarch64 backend for Float16 vector operations
85af573: 8356848: Separate Metaspace and GC printing
78d0dc7: 8357460: RISC-V: Optimize array fill stub for small size
... and 231 more: https://git.openjdk.org/jdk/compare/03dca0323d79ef5fb1c8ee1152667e2188fa5e01...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-27T08:29:03Z

@jatin-bhateja Pushed as commit 5924c2d.

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

jatin-bhateja marked this pull request as draft May 21, 2025 12:34

openjdk bot added the hotspot [email protected] label May 21, 2025

8357267: ZGC: Handle APX EGPRs spilling in ZRuntimeCallSpill

79d7778

jatin-bhateja force-pushed the JDK-8357267 branch from 89fbf49 to 79d7778 Compare May 21, 2025 14:01

openjdk bot added the hotspot-compiler [email protected] label May 21, 2025

openjdk bot added the hotspot-gc [email protected] label May 21, 2025

jatin-bhateja marked this pull request as ready for review May 21, 2025 14:04

openjdk bot added the rfr Pull request is ready for review label May 21, 2025

sviswa7 reviewed May 21, 2025

View reviewed changes

Review comments resolutions

efc4f01

Review comments resolution

9b5c2ac

sviswa7 approved these changes May 22, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 22, 2025

Axel's comments incorporated

bd8b9c5

xmas92 suggested changes May 26, 2025

View reviewed changes

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp Outdated Show resolved Hide resolved

openjdk bot removed the ready Pull request is ready to be integrated label May 26, 2025

robcasloz approved these changes May 27, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 27, 2025

openjdk bot added the integrated Pull request has been integrated label May 27, 2025

openjdk bot closed this May 27, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 27, 2025

jatin-bhateja mentioned this pull request Jun 30, 2025

8360775: Fix Shenandoah GC test failures when APX is enabled #26009

Closed

3 tasks

8357267: ZGC: Handle APX EGPRs spilling in ZRuntimeCallSpill #25351

8357267: ZGC: Handle APX EGPRs spilling in ZRuntimeCallSpill #25351

Uh oh!

Conversation

jatin-bhateja commented May 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

jatin-bhateja commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

jatin-bhateja commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

mlbridge bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

sviswa7 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

sviswa7 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

sviswa7 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

sviswa7 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

sviswa7 commented May 21, 2025

Uh oh!

xmas92 commented May 22, 2025

Uh oh!

robcasloz commented May 22, 2025

Uh oh!

jatin-bhateja commented May 22, 2025

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

robcasloz commented May 23, 2025

Uh oh!

jatin-bhateja commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xmas92 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robcasloz commented May 26, 2025

Uh oh!

robcasloz left a comment

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja commented May 27, 2025

Uh oh!

jatin-bhateja commented May 27, 2025

Uh oh!

openjdk bot commented May 27, 2025

Uh oh!

openjdk bot commented May 27, 2025

Uh oh!

Uh oh!

jatin-bhateja commented May 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 21, 2025 •

edited

Loading

mlbridge bot commented May 21, 2025 •

edited

Loading

jatin-bhateja commented May 26, 2025 •

edited

Loading