On Mon, 14 Sep 2026 06:56:06 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 19 commits:
>
> - 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
> - Merge branch 'master' into JDK-8358959
>
> # Conflicts:
> # src/hotspot/cpu/riscv/riscv.ad
> - Code format
> - Merge remote-tracking branch 'upstream/master' into JDK-8358959
> - RISC-V: Use register operands for Zalasr access helpers
> - Code Format
> - Fix for merge code
> - Merge branch 'master' into JDK-8358959
>
> # Conflicts:
> # src/hotspot/cpu/riscv/downcallLinker_riscv.cpp
> # src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp
> # src/hotspot/cpu/riscv/templateInterpreterGenerator_riscv.cpp
> - ... and 9 more: https://git.openjdk.org/jdk/compare/89365a0f...479d1168
src/hotspot/cpu/riscv/assembler_riscv.hpp line 1189:
> 1187: assert_cond(UseZalasr);
> 1188:
> 1189: if (funct5 == ZALASR_LOAD_ACQUIRE) {
I wonder whether `if constexpr` can be used here.
src/hotspot/cpu/riscv/assembler_riscv.hpp line 1204:
> 1202: memory_order = (memory_order == aqrl) ? aqrl : rl;
> 1203: } else {
> 1204: ShouldNotReachHere();
Can probably use `static_assert(funct5 == ...` instead of this dynamic checking.
src/hotspot/cpu/riscv/c1_LIRAssembler_riscv.cpp line 1980:
> 1978:
> 1979: void LIR_Assembler::load_volatile(LIR_Address* from_addr, LIR_Opr dest,
> BasicType type, CodeEmitInfo* info) {
> 1980: if (!UseZalasr) {
This feels a bit unbalanced; how about:
if (UseZalasr) {
load_acquire(from_addr, dest, type, info);
} else {
load_unordered(from_addr, dest, type, /* wide */ false, info);
membar_acquire();
}
src/hotspot/cpu/riscv/jniFastGetField_riscv.cpp line 148:
> 146: __ fmv_x_d(result, f28); // d{63--0}-->x
> 147: break;
> 148: }
Preexisting: why this path can't use fall-through for float/double?
src/hotspot/cpu/riscv/riscv.ad line 1288:
> 1286:
> 1287: bool needs_acquiring_load(const Node *n) {
> 1288: if (!UseZalasr) return false;
I'd suggest breaking this into multiple lines to make the early-return more
visible.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4004709635
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4004715479
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4004955418
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4005033445
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4005052592
