On Mon, 14 Sep 2026 16:02:22 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 incrementally with two additional
> commits since the last revision:
>
> - Apply code review
> - RISC-V: Use Zalasr for the ordered accesses in AtomicAccess
src/hotspot/cpu/riscv/riscv.ad line 1293:
> 1291: assert(n->is_Load(), "expecting a load");
> 1292: return n->as_Load()->is_acquire();
> 1293: }
Why not placing the assert at the beginning as a precondition for the whole
method?
I'd suggest avoiding using early-return here, so sth like:
if (UseZalasr) {
return ...
} else {
return ...
}
src/hotspot/cpu/riscv/riscv.ad line 1343:
> 1341: #endif
> 1342:
> 1343: return UseZalasr && release;
Maybe use early-return of `UseZalasr` after the precondition-assertion.
src/hotspot/cpu/riscv/riscv.ad line 1354:
> 1352: assert(n->is_Store(), "expecting a store");
> 1353: return n->as_Store()->trailing_membar() != nullptr;
> 1354: }
Can be made more balanced like `needs_acquiring_load` above.
src/hotspot/cpu/riscv/riscv.ad line 4674:
> 4672: %{
> 4673: match(Set dst (ConvI2L (LoadB mem)));
> 4674: predicate(needs_acquiring_load(n->in(1)));
Seems that both styles exist, predicate-before-match is the dominant one, so
maybe follow that one instead.
src/hotspot/cpu/riscv/vm_version_riscv.cpp line 51:
> 49: #else
> 50: #define JVM_TOOLCHAIN_USES_PSABI_ATOMICS 0
> 51: #endif
Can probably use a function:
static constexpr bool toolchain_uses_psabi_atomics() {
#if defined(__clang_major__)
return __clang_major__ >= 19;
#elif defined(__GNUC__)
return __GNUC__ > 13 ||
(__GNUC__ == 13 && __GNUC_MINOR__ >= 3);
#else
return false;
#endif
}
src/hotspot/os_cpu/linux_riscv/atomicAccess_linux_riscv.hpp line 273:
> 271: DEFINE_ZALASR_ACCESS(8, 3) // ld.aq / sd.rl
> 272:
> 273: #undef DEFINE_ZALASR_ACCESS
I wonder if we can avoid macro here, sth along the line:
constexpr int zalasr_width(size_t byte_size) {
switch (byte_size) {
case 1: return 0;
case 2: return 1;
case 4: return 2;
case 8: return 3;
default: return -1;
}
}
template<size_t byte_size>
inline uint64_t zalasr_load_acquire(const void* p) {
constexpr int width = zalasr_width(byte_size);
static_assert(width >= 0, "unsupported access size");
uint64_t data;
__asm__ __volatile__ (
".insn r 0x2f, %2, 0x1a, %0, %1, zero"
: "=r" (data)
: "r" (p), "i" (width)
: "memory");
return data;
}
template<size_t byte_size>
inline void zalasr_store_release(void* p, uint64_t v) {
constexpr int width = zalasr_width(byte_size);
static_assert(width >= 0, "unsupported access size");
__asm__ __volatile__ (
".insn r 0x2f, %2, 0x1d, zero, %0, %1"
: /* no output */
: "r" (p), "r" (v), "i" (width)
: "memory");
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4010992731
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011026034
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011029645
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011140017
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4010928720
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4010815333
