8367066: RISC-V: refine register selection in MacroAssembler:: decode_klass_not_null #27138

Hamlin-Li · 2025-09-08T08:57:15Z

Hi,
Can you help to review this patch?

Previously, the register selection in MacroAssembler:: decode_klass_not_null is misleading, better to refine it to improve the readability.

Thanks!

Running runtime test tier1/2/3...

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-8367066: RISC-V: refine register selection in MacroAssembler:: decode_klass_not_null (Bug - P4)

Reviewers

Fei Yang (@RealFYang - Reviewer)
Feilong Jiang (@feilongjiang - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27138

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-08T08:58:04Z

👋 Welcome back mli! 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-09-08T08:58:51Z

@Hamlin-Li 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:

8367066: RISC-V: refine register selection in MacroAssembler:: decode_klass_not_null

Reviewed-by: fyang, fjiang

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

134c3ef: 8367293: RISC-V: enable vectorapi test for VectorMask.laneIsSet
7fcce27: 8365956: GenShen: Adaptive tenuring threshold algorithm may raise threshold prematurely
8599657: 8365676: javac incorrectly allows calling interface static method via type variable
... and 59 more: https://git.openjdk.org/jdk/compare/051f39e12ce8845d13c7d4813dabc556a834892d...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-09-08T08:59:30Z

@Hamlin-Li 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.

mlbridge · 2025-09-08T09:02:52Z

Webrevs

01: Full - Incremental (89ed8b85)
00: Full (7598f4d0)

RealFYang

Hi, seems we can further eliminate the use of t0 replacing it with tmp. How about we go like this?

diff --git a/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp b/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
index 1436bc02113..89e615b1cbb 100644
--- a/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
+++ b/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
@@ -3402,6 +3402,8 @@ void MacroAssembler::decode_klass_not_null(Register r, Register tmp) {

 void MacroAssembler::decode_klass_not_null(Register dst, Register src, Register tmp) {
   assert(UseCompressedClassPointers, "should only be used for compressed headers");
+  assert_different_registers(src, tmp);
+  assert_different_registers(dst, tmp);

   if (CompressedKlassPointers::base() == nullptr) {
     if (CompressedKlassPointers::shift() != 0) {
@@ -3421,7 +3423,7 @@ void MacroAssembler::decode_klass_not_null(Register dst, Register src, Register
   mv(xbase, (uintptr_t)CompressedKlassPointers::base());

   if (CompressedKlassPointers::shift() != 0) {
-    Register t = src == dst ? dst : t0;
+    Register t = dst == xbase ? tmp : dst;
     assert_different_registers(t, xbase);
     shadd(dst, src, xbase, t, CompressedKlassPointers::shift());
   } else {

Hamlin-Li · 2025-09-08T12:49:31Z

Hi, seems we can further eliminate the use of t0 replacing it with tmp. How about we go like this?

diff --git a/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp b/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
index 1436bc02113..89e615b1cbb 100644
--- a/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
+++ b/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp
@@ -3402,6 +3402,8 @@ void MacroAssembler::decode_klass_not_null(Register r, Register tmp) {

 void MacroAssembler::decode_klass_not_null(Register dst, Register src, Register tmp) {
   assert(UseCompressedClassPointers, "should only be used for compressed headers");
+  assert_different_registers(src, tmp);
+  assert_different_registers(dst, tmp);

   if (CompressedKlassPointers::base() == nullptr) {
     if (CompressedKlassPointers::shift() != 0) {
@@ -3421,7 +3423,7 @@ void MacroAssembler::decode_klass_not_null(Register dst, Register src, Register
   mv(xbase, (uintptr_t)CompressedKlassPointers::base());

   if (CompressedKlassPointers::shift() != 0) {
-    Register t = src == dst ? dst : t0;
+    Register t = dst == xbase ? tmp : dst;
     assert_different_registers(t, xbase);
     shadd(dst, src, xbase, t, CompressedKlassPointers::shift());
   } else {

I applied your suggested patch, and it trigger an assert at assert_different_registers(dst, tmp);,

assert(regs[i] != regs[j]) failed: regs[0] and regs[1] are both: t2

still running tests by removing this assert only.

RealFYang · 2025-09-08T23:47:48Z

Yes, we still need one extra change to guarantee that dst != tmp, which we can do with a TEMP_DEF effect for dst.
I tried adding this extra change and it seems to work.

EDIT: My local hotspot:tier1 passed with fastdebug build on SG2042.

@@ -8936,7 +8941,7 @@ instruct encodeKlass_not_null(iRegNNoSp dst, iRegP src) %{
 instruct decodeKlass_not_null(iRegPNoSp dst, iRegN src, iRegPNoSp tmp) %{
   match(Set dst (DecodeNKlass src));

-  effect(TEMP tmp);
+  effect(TEMP_DEF dst, TEMP tmp);

   ins_cost(ALU_COST);
   format %{ "decode_klass_not_null  $dst, $src\t#@decodeKlass_not_null" %}

Hamlin-Li · 2025-09-09T14:09:34Z

Yes, we still need one extra change to guarantee that dst != tmp, which we can do with a TEMP_DEF effect for dst. I tried adding this extra change and it seems to work.

EDIT: My local hotspot:tier1 passed with fastdebug build on SG2042.
@@ -8936,7 +8941,7 @@ instruct encodeKlass_not_null(iRegNNoSp dst, iRegP src) %{
 instruct decodeKlass_not_null(iRegPNoSp dst, iRegN src, iRegPNoSp tmp) %{
   match(Set dst (DecodeNKlass src));

-  effect(TEMP tmp);
+  effect(TEMP_DEF dst, TEMP tmp);

   ins_cost(ALU_COST);
   format %{ "decode_klass_not_null  $dst, $src\t#@decodeKlass_not_null" %}

In this sense, we're not saving one register (t0), as we require extra register by effect(TEMP_DEF dst, TEMP tmp);, right?

Hamlin-Li · 2025-09-09T14:09:47Z

I think we can further simplify the code in decode_klass_not_null, pleaes check the latest commit. Test running...

RealFYang · 2025-09-10T00:24:53Z

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

-    assert_different_registers(t, xbase);
-    shadd(dst, src, xbase, t, CompressedKlassPointers::shift());
+    // dst = (src << shift) + xbase
+    shadd(dst, src, xbase, dst /* temporary, dst != xbase */, CompressedKlassPointers::shift());


Is it safer to turn the code comment into an assertion? It will trigger if people incorrectly sets xbase.

assert_different_registers(dst, xbase); // dst = (src << shift) + xbase shadd(dst, src, xbase, dst, CompressedKlassPointers::shift());

In shadd when tmp (here we use dst as tmp) is used, there is already an assert there.

RealFYang · 2025-09-10T00:30:31Z

I think we can further simplify the code in decode_klass_not_null, pleaes check the latest commit. Test running...

Yes, the code now looks more simplified. Thanks.

Hamlin-Li · 2025-09-10T08:19:46Z

I think we can further simplify the code in decode_klass_not_null, pleaes check the latest commit. Test running...

Yes, the code now looks more simplified. Thanks.

Thanks for reviewing.

feilongjiang

Thanks!

Hamlin-Li · 2025-09-11T08:05:13Z

Thanks!

Thank you for reviewing!

Hamlin-Li · 2025-09-11T08:05:21Z

/integrate

openjdk · 2025-09-11T08:07:30Z

Going to push as commit 0b3a303.
Since your change was applied there have been 67 commits pushed to the master branch:

8ba0db0: 8366951: Test runtime/logging/StressAsyncUL.java is timing out
7690a45: 8366342: Key generator and key pair generator tests skipping, but showing as passed
2826d17: 8367243: Format issues with dist dump debug output in PhaseGVN::dead_loop_check
... and 64 more: https://git.openjdk.org/jdk/compare/051f39e12ce8845d13c7d4813dabc556a834892d...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-11T08:07:43Z

@Hamlin-Li Pushed as commit 0b3a303.

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

initial commit

7598f4d

openjdk bot added hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review labels Sep 8, 2025

RealFYang reviewed Sep 8, 2025

View reviewed changes

refine

89ed8b8

RealFYang approved these changes Sep 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 10, 2025

feilongjiang approved these changes Sep 11, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 11, 2025

openjdk bot closed this Sep 11, 2025

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

8367066: RISC-V: refine register selection in MacroAssembler:: decode_klass_not_null #27138

8367066: RISC-V: refine register selection in MacroAssembler:: decode_klass_not_null #27138

Uh oh!

Conversation

Hamlin-Li commented Sep 8, 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 Sep 8, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Sep 8, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

RealFYang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hamlin-Li commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RealFYang commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hamlin-Li commented Sep 9, 2025

Uh oh!

Hamlin-Li commented Sep 9, 2025

Uh oh!

RealFYang Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hamlin-Li Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

RealFYang commented Sep 10, 2025

Uh oh!

Hamlin-Li commented Sep 10, 2025

Uh oh!

feilongjiang left a comment

Choose a reason for hiding this comment

Uh oh!

Hamlin-Li commented Sep 11, 2025

Uh oh!

Hamlin-Li commented Sep 11, 2025

Uh oh!

openjdk bot commented Sep 11, 2025

Uh oh!

openjdk bot commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Hamlin-Li commented Sep 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 8, 2025 •

edited

Loading

mlbridge bot commented Sep 8, 2025 •

edited

Loading

RealFYang left a comment •

edited

Loading

Hamlin-Li commented Sep 8, 2025 •

edited

Loading

RealFYang commented Sep 8, 2025 •

edited

Loading

RealFYang Sep 10, 2025 •

edited

Loading