RFR: 8351949: RISC-V: Cleanup and enable store-load peephole for membars

[Fei Yang]

On Thu, 13 Mar 2025 13:49:32 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

> Hi please consider.
> 
> |RVWMO| Patched|
> | ---------- | ---------- |
> |fence iorw,iorw| fence iorw,ow|
> |sw  t4,120(t2) | sw  t4,120(t2) |
> |fence ow,ir | unnecessary_membar_volatile_rvwmo |
> | sw  t6,128(t2) // Non-volatile | sw  t6,128(t2) // Non-volatile |
> |fence iorw,ow | fence iorw,ow|
> |sw  t5,124(t2) |sw  t5,124(t2) |
> 
> |TSO                                | Patched|
> | ---------- | ---------- |
> | lw	a4,120(t2) | lw	a6,120(t2) |
> | sw	a0,124(t2) | sw	t6,124(t2) |
> | fence	iorw,iorw | unnecessary_membar_volatile_tso |
> | sw	t4,120(t2) | sw	t4,120(t2) |
> | fence	ow,ir | unnecessary_membar_volatile_tso |
> | sw	t6,128(t2) | sw	t5,128(t2) |
> | sw	t5,124(t2) // Non-volatile| sw	a1,124(t2)  // Non-volatile |
> | fence	iorw,iorw  | unnecessary_membar_volatile_tso |
> |...  | ... |
> | sw	a3,120(t2) | sw	a0,120(t2) |
> | fence	ow,ir | fence	ow,ir |
> | lw	a7,124(t2) | lw	a5,124(t2) |
> 
> For the specific rvwmo volatile store + store + volatile store is around 30% faster on VF2.
> 
> The patch do:
> - Separate ztso and rvwmo in ad by using UseZtso predicate.
> - Match all that requires the same membar.
> - Make fence/fencei protected as they shouldn't be using directly.
> - Increased cost of membars to VOLATILE_REF_COST.
> - Added a real_empty pipe.
> - Change to pipe_slow on TSO (as x86).
> 
> Note that C2-rv64 is now superior to gcc/clang regrading fencing:
> https://godbolt.org/z/6E3YTP15j
> 
> Testing jcstress, tier1 and manually reading the generated assembly.
> Doing additional testing, but RFR it now as it may need some consideration.
> 
> /Robbin

Hi, I am trying to understand this. What's the Java source code look like regarding the first table showing the difference in JIT code in PR description?

In naming, can we use names like `_rvtso` instead of `_tso` which I think maps better to `_rvwmo`? I think it's OK as I read this from the RV spec which also mentions RVTSO:

This chapter defines the "Ztso" extension for the RISC-V Total Store Ordering (RVTSO) memory
 consistency model. RVTSO is defined as a delta from RVWMO, which is defined in Section 17.1.

src/hotspot/cpu/riscv/riscv.ad line 7982:

> 7980: 
> 7981:   format %{ "membar_rvwmo_storestore\n\t"
> 7982:             "fence rw, w" %}

Shouldn't this be `"fence w, w"`?

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

PR Review: https://git.openjdk.org/jdk/pull/24035#pullrequestreview-2684509713
PR Review Comment: https://git.openjdk.org/jdk/pull/24035#discussion_r1994972391