On Tue, 15 Sep 2026 01:05:58 GMT, Albert Mingkun Yang <[email protected]> wrote:
>> 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 ...
> }
Fixed.
> 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.
Fixed.
> 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.
Fixed.
> 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.
Fixed.
> 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
> }
Fixed.
> 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");
> }
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011493842
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011493660
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011493443
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011492005
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011494028
PR Review Comment: https://git.openjdk.org/jdk/pull/32309#discussion_r4011494253
