On Wed, 16 Sep 2026 06:08:08 GMT, Gui Cao <[email protected]> wrote:
>> Hi, This PR emits Zalasr load-acquire/store-release instructions (ISA manual
>> Table A.7 mapping) for Java volatile accesses across the interpreter, C1 and
>> C2, guarded by the experimental UseZalasr flag.
>>
>> ### Clarifying the JDK-8358959 concern
>> JDK-8358959[1] stalled on one question: JIT code using the A.7 mapping may
>> interoperate with native code using the old Table A.6 mapping (clang <= 18),
>> and on the seq_cst StoreLoad edge that combination is broken — an A.6 store
>> carries no trailing barrier (it expects the reader to pay) and an A.7 l.aq
>> carries no leading barrier (it expects the writer's .rl annotation), so
>> nobody orders the pair (the ISA manual warns about exactly this pair between
>> the two tables [2]). Since the JVM cannot audit every user JNI library, the
>> issue looked unresolvable.
>>
>> Our key observation: this incompatibility is not introduced by Zalasr — it
>> already exists today. HotSpot's current volatile scheme is "writer pays"
>> (trailing fence w,r on volatile stores, bare volatile loads with no leading
>> fence). An old clang JNI library doing a seq_cst store is "reader pays"
>> (bare store, no trailing barrier). Cross the two and the StoreLoad edge is
>> already unpaid, with no Zalasr instruction involved.
>>
>> This is also exactly why the RISC-V psABI strengthened the C/C++ seq_cst
>> store with a trailing fence (gcc >= 13.3, clang >= 19) and deprecated the
>> old mapping as "must not be combined" (Note 3 of the psABI atomics chapter
>> [3]): the standard already ruled in favor of writer-pays, i.e. HotSpot's
>> side.
>>
>> Consequently, requiring psABI-toolchain-built native code is a pre-existing
>> correctness baseline for the JVM on RISC-V, not a new cost of Zalasr. This
>> PR therefore:
>>
>> 1. gates UseZalasr on the JVM itself being built by a psABI toolchain (gcc
>> >= 13.3 / clang >= 19), so libjvm and the bundled native libraries are
>> guaranteed compatible with the JIT's A.7 code
>> 2. keeps interpreter/C1/C2 volatile accesses mutually compatible (C1
>> volatile loads use l*.aq; interpreter volatile loads gain a leading fence
>> when C2 is active, mirroring the AArch64 JDK-8179954[4] treatment).
>>
>> [1] https://bugs.openjdk.org/browse/JDK-8358959
>> [2] https://docs.riscv.org/reference/isa/v20260120/unpriv/mm-eplan.html
>> [3]
>> https://riscv-non-isa.github.io/riscv-elf-psabi-doc/#_risc_v_atomics_mappings
>> [4] https://bugs.openjdk.org/browse/JDK-8179954
>>
>>
>>
>> ---------
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy...
>
> Gui Cao has updated the pull request with a new target base due to a merge or
> a rebase. The pull request now contains 26 commits:
>
> - Merge remote-tracking branch 'upstream/master' into JDK-8358959
> - Update for Axel code review
> - RISC-V: Gate native AtomicAccess Zalasr dispatch on a post-validated flag
> - Code format
> - RISC-V: Zalasr code review followups
> - Apply code review
> - RISC-V: Use Zalasr for the ordered accesses in AtomicAccess
> - Merge remote-tracking branch 'upstream/master' into JDK-8358959
> - RISC-V: Align C1 volatile load dispatch with AArch64
> - Merge remote-tracking branch 'upstream/master' into JDK-8358959
> - ... and 16 more: https://git.openjdk.org/jdk/compare/b0ac803f...bcc84174
Such a large patch. Thanks a lot.
Several questions/suggestions.
src/hotspot/cpu/riscv/c1_LIRAssembler_riscv.cpp line 1994:
> 1992: // address is computed into t1.
> 1993: Address addr = as_Address(from_addr);
> 1994: assert(addr.getMode() == Address::base_plus_offset, "unsupported
> addressing mode");
What about asserting `UseZalasr` is true at the beginning of the method?
src/hotspot/cpu/riscv/gc/shenandoah/shenandoahBarrierSetAssembler_riscv.cpp
line 603:
> 601: "acquire path requires address to be base-only");
> 602: __ lw_aq(dst, src.base());
> 603: __ zext(dst, dst, 32);
Is `src.offset` always 0? What about using `la` to get the address?
src/hotspot/cpu/riscv/gc/shenandoah/shenandoahBarrierSetAssembler_riscv.cpp
line 603:
> 601: "acquire path requires address to be base-only");
> 602: __ lw_aq(dst, src.base());
> 603: __ zext(dst, dst, 32);
Could we use `lwu_acquire` here? It seems we need to judge whether`UseZalasr`
is true. Or we should add an assert statement to judge that `UseZalasr` is true.
src/hotspot/cpu/riscv/gc/shenandoah/shenandoahBarrierSetAssembler_riscv.cpp
line 611:
> 609: assert(src.getMode() == Address::base_plus_offset && src.offset()
> == 0,
> 610: "acquire path requires address to be base-only");
> 611: __ ld_aq(dst, src.base());
Could we use `ld_acquire` here? Same as above.
src/hotspot/cpu/riscv/gc/shenandoah/shenandoahBarrierSetAssembler_riscv.cpp
line 640:
> 638: assert(dst.getMode() == Address::base_plus_offset && dst.offset()
> == 0,
> 639: "volatile path requires address to be base-only");
> 640: __ sw_rl(src, dst.base());
Same as `load_c2`. May use `sw_release` or assert `UseZalasr` is always true.
src/hotspot/cpu/riscv/gc/shenandoah/shenandoah_riscv.ad line 215:
> 213: /* is_volatile = */ true);
> 214: %}
> 215: ins_cost(4 * STORE_COST);
According to the costs of other instructions, should the cost here be
`VOLATILE_REF_COST` or `VOLATILE_REF_COST + STORE_COST`?
src/hotspot/cpu/riscv/gc/z/z_riscv.ad line 162:
> 160: format %{ "sd.rl $mem, $src\t# ptr, #@zStorePVolatile" %}
> 161: ins_encode %{
> 162: guarantee($mem$$disp == 0, "impossible encoding");
A problem similar to shenandoah GC: is the displacement always 0?
src/hotspot/cpu/riscv/gc/z/z_riscv.ad line 164:
> 162: guarantee($mem$$disp == 0, "impossible encoding");
> 163: z_store_barrier(masm, this, Address($mem$$Register), $src$$Register,
> $tmp1$$Register, $tmp2$$Register, false /* is_atomic */);
> 164: __ sd_rl($tmp1$$Register, $mem$$Register);
Same as above. May use `sd_release` or assert `UseZalasr` is always true.
src/hotspot/cpu/riscv/jniFastGetField_riscv.cpp line 147:
> 145: // check (LoadStore for volatile field).
> 146: __ membar(MacroAssembler::LoadLoad | MacroAssembler::LoadStore);
> 147: }
What about using `lbu_acquire`、`lhu_acquire`、`lwu_acquire` instead?
src/hotspot/cpu/riscv/riscv.ad line 5094:
> 5092: instruct loadKlass(iRegPNoSp dst, memory mem)
> 5093: %{
> 5094: predicate(!needs_acquiring_load(n));
Now, we don't have a `instruction` rule to match `needs_acquiring_load` is
true. Is it a intentional behavior?
src/hotspot/cpu/riscv/templateTable_riscv.cpp line 2649:
> 2647: __ bind(notVolatile);
> 2648: }
> 2649:
Should we always generate `s{b|h|w|d}.rl` and `l{b|h|w|d}.aq` instructions in
interpreter when `UseZalasr` is true? Then we don't need such additional check.
It could be investigated and done in another PR.
src/hotspot/cpu/riscv/vm_version_riscv.hpp line 528:
> 526: // atomics mapping. This flag is false by default and is latched to
> the final
> 527: // UseZalasr value after all validation completes.
> 528: static bool _use_zalasr_atomics;
Good caught!
-------------
PR Review: https://git.openjdk.org/jdk/pull/32309#pullrequestreview-5219179028
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023707927
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023330112
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023499841
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023527235
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023678011
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023072623
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023367716
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023549200
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023758998
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4023891862
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4024105135
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4024195583
