RFR: 8320397: RISC-V: Avoid passing t0 as temp register to MacroAssembler:: cmpxchg_obj_header/cmpxchgptr

[Robbin Ehn]

On Wed, 29 Nov 2023 11:58:31 GMT, Gui Cao <gcao at openjdk.org> wrote:

> MacroAssembler::cmpxchg/cmpxchgptr/cmpxchg_obj_header is non-trivial on linux-riscv64 platform. Passing t0(aka x5) as temporary register to this functions can also be error prone. As a reserved scratch register, t0 is implicitly clobberred by various assembler functions. @robehn can you help review this PR?
> This issue is used to track avoid passing t0 as a temporary register in the following cases:
> 1. avoid passing t0 as temp register to MacroAssembler::cmpxchg/cmpxchgptr/cmpxchg_obj_header.
> 2. avoid passing t0 as temp register to x_load_barrier and x_load_barrier_slow_path function in x_riscv.ad
> 3. avoid passing t0 as temp register to z_store_barrier and z_color function in z_riscv.ad
> 
> Note that I didn't touch MacroAssembler::cmpxchg because it seems to me that this function is designed that it allows t0 to be used as the result register. As the result register will be set on exits, there should be no risk when using t0 for receiving the result.
> https://github.com/openjdk/jdk/blob/e44d4b24ed794957c47c140ab6f15544efa2b278/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L2910-L2925
> 
> ### Testing:
> - [x]  Run tier1-3 tests on qemu 8.1.50 with UseRVV (release)
> - [x]  Run tier1-3 tests with SiFive unmatched (release)

I think we should not use any fixed regs, just add them to the nodes, like PPC have done.

src/hotspot/cpu/riscv/gc/x/x_riscv.ad line 55:

> 53: 
> 54: // Load Pointer
> 55: instruct xLoadP(iRegPNoSp dst, memory mem, rFlagsReg cr)

Add a tmpReg here, as t1 is also used by masm.
ppc have added tempregs for all these I think we should also do that as both t0 and t1 can be used by masm.

src/hotspot/cpu/riscv/gc/x/x_riscv.ad line 142:

> 140: %}
> 141: 
> 142: instruct xCompareAndExchangeP(iRegPNoSp res, indirect mem, iRegP oldval, iRegP newval, rFlagsReg cr) %{

So we fix them like this:

diff --git a/src/hotspot/cpu/riscv/gc/x/x_riscv.ad b/src/hotspot/cpu/riscv/gc/x/x_riscv.ad
index d90539123ff..e5383a695c5 100644
--- a/src/hotspot/cpu/riscv/gc/x/x_riscv.ad
+++ b/src/hotspot/cpu/riscv/gc/x/x_riscv.ad
@@ -74 +74 @@ instruct xLoadP(iRegPNoSp dst, memory mem, rFlagsReg cr)
-instruct xCompareAndSwapP(iRegINoSp res, indirect mem, iRegP oldval, iRegP newval, rFlagsReg cr) %{
+instruct xCompareAndSwapP(iRegINoSp res, indirect mem, iRegP oldval, iRegP newval, iRegINoSp tmpreg, rFlagsReg cr) %{
@@ -78 +78 @@ instruct xCompareAndSwapP(iRegINoSp res, indirect mem, iRegP oldval, iRegP newva
-  effect(KILL cr, TEMP_DEF res);
+  effect(KILL cr, TEMP_DEF res, TEMP tmpreg);
@@ -89,2 +89,2 @@ instruct xCompareAndSwapP(iRegINoSp res, indirect mem, iRegP oldval, iRegP newva
-               Assembler::relaxed /* acquire */, Assembler::rl /* release */, t1);
-    __ sub(t0, t1, $oldval$$Register);
+               Assembler::relaxed /* acquire */, Assembler::rl /* release */, $tmpreg$$Register);
+    __ sub(t0, $tmpreg$$Register, $oldval$$Register);
@@ -95 +95 @@ instruct xCompareAndSwapP(iRegINoSp res, indirect mem, iRegP oldval, iRegP newva
-      __ andr(t0, t0, t1);
+      __ andr(t0, t0, $tmpreg$$Register);
@@ -97 +97 @@ instruct xCompareAndSwapP(iRegINoSp res, indirect mem, iRegP oldval, iRegP newva
-      x_load_barrier_slow_path(_masm, this, Address($mem$$Register), t1 /* ref */, $res$$Register /* tmp */);
+      x_load_barrier_slow_path(_masm, this, Address($mem$$Register), $tmpreg$$Register /* ref */, $res$$Register /* tmp */);

src/hotspot/cpu/riscv/gc/z/z_riscv.ad line 143:

> 141:     guarantee($mem$$disp == 0, "impossible encoding");
> 142:     Address ref_addr($mem$$Register);
> 143:     z_color(_masm, this, $oldval_tmp$$Register, $oldval$$Register, t1);

Here you can use newval_tmp instead t1, no?

Don't need to add additional regs.

-------------

PR Review: https://git.openjdk.org/jdk/pull/16880#pullrequestreview-1777113868
PR Review Comment: https://git.openjdk.org/jdk/pull/16880#discussion_r1423769200
PR Review Comment: https://git.openjdk.org/jdk/pull/16880#discussion_r1423784938
PR Review Comment: https://git.openjdk.org/jdk/pull/16880#discussion_r1423779627