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

[Fei Yang]

On Mon, 17 Mar 2025 12:35:42 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
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments

Looks good. My local tests are still good.

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

> 7977:   ins_cost(VOLATILE_REF_COST);
> 7978: 
> 7979:   format %{ "membar_volatile_rvtso\n\t"

It's a bit confusing to me to see this `membar_volatile_rvtso` followed by a `fence w, r`.
Seems better if we put a `#@` prefix making it a code comment for this `fence w, r`. I mean: `#@membar_volatile_rvtso`

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

> 8010:   ins_cost(VOLATILE_REF_COST);
> 8011: 
> 8012:   format %{ "membar_aqcuire_rvwmo\n\t"

Similar here.

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

> 8026:   ins_cost(VOLATILE_REF_COST);
> 8027: 
> 8028:   format %{ "membar_release_rvwmo\n\t"

Here.

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

> 8042:   ins_cost(VOLATILE_REF_COST);
> 8043: 
> 8044:   format %{ "membar_storestore_rvwmo\n\t"

Here.

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

> 8056:   ins_cost(VOLATILE_REF_COST);
> 8057: 
> 8058:   format %{ "membar_volatile_rvwmo\n\t"

And here.

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

Marked as reviewed by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24035#pullrequestreview-2693245825
PR Review Comment: https://git.openjdk.org/jdk/pull/24035#discussion_r2000350037
PR Review Comment: https://git.openjdk.org/jdk/pull/24035#discussion_r2000351489
PR Review Comment: https://git.openjdk.org/jdk/pull/24035#discussion_r2000351934
PR Review Comment: https://git.openjdk.org/jdk/pull/24035#discussion_r2000352205
PR Review Comment: https://git.openjdk.org/jdk/pull/24035#discussion_r2000352466