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

robehn · 2025-03-13T13:49:32Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8351949: RISC-V: Cleanup and enable store-load peephole for membars (Enhancement - P4)

Reviewers

Fei Yang (@RealFYang - Reviewer)
Feilong Jiang (@feilongjiang - Committer)
Hamlin Li (@Hamlin-Li - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24035/head:pull/24035
$ git checkout pull/24035

Update a local copy of the PR:
$ git checkout pull/24035
$ git pull https://git.openjdk.org/jdk.git pull/24035/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24035

View PR using the GUI difftool:
$ git pr show -t 24035

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24035.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-13T13:50:24Z

👋 Welcome back rehn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-13T13:51:24Z

@robehn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: fyang, fjiang, mli

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 12 new commits pushed to the master branch:

db08726: 8352966: Opensource Several Font related tests - Batch 2
6b7b324: 8351431: Type annotations on new class creation expressions can't be retrieved
64b691a: 8271870: G1: Add objArray splitting when scanning object with evacuation failure
b428cda: 8349686: [s390x] C1: Improve Class.isInstance intrinsic
70e3250: 8352419: Test tools/jpackage/share/ErrorTest.java#id0 and #id1 fail
296d9d6: 8353345: C2 asserts because maskShiftAmount modifies node without deleting the hash
3ceabf0: 8353359: C2: Or(I|L)Node::Ideal is missing AddNode::Ideal call
b263292: 8353484: JFR: Simplify EventConfiguration
ffca4f2: 8353264: ZGC: Windows heap unreserving is broken
f7a94fe: 8352585: Add special case handling for Float16.max/min x86 backend
... and 2 more: https://git.openjdk.org/jdk/compare/bd74922157230c866802b4c5269da81e872525aa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-03-13T13:51:59Z

@robehn The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-03-13T15:56:16Z

Webrevs

RealFYang

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

robehn · 2025-03-14T07:49:45Z

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?

Thank you for having a look.

Do this help ?

volatile int v_a;
volatile int v_b;
int plain_c;

int temp_a = v_a;
v_b = 77;
plain_c = 66; // Non-volatile store between two volatile stores.
v_a = 88;
int temp_b = v_b;

The peephole is very powerful on tso, as we only care about store-load.
But it do improve some cases on rvwmo also, hence I enabled it on both.

robehn · 2025-03-14T07:54:27Z

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:

Yes, sure.

Also please run some jcstress on any machine you have!

RealFYang

Hi, Thanks for the update. I have checked the changes, seems fine to me modulo several minor comments about naming. And I having been running jcstress on two of my OoO machines over the weekend. So far so good. I will let the test continue for some more time to see.

src/hotspot/cpu/riscv/riscv.ad

RealFYang

Looks good. My local tests are still good.

src/hotspot/cpu/riscv/riscv.ad

RealFYang

Looks great to me now! Thanks for the updates!

robehn · 2025-03-19T09:34:54Z

Looks great to me now! Thanks for the updates!

Yet another, thank you! :)

feilongjiang

Looks good!

robehn · 2025-03-29T15:00:41Z

Looks good!

Thank you!

Hamlin-Li

Thanks for the patience and discussion.
Looks good to me, just some minor comments.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

src/hotspot/cpu/riscv/riscv.ad

robehn · 2025-04-07T07:08:45Z

Thanks @Hamlin-Li !

robehn · 2025-04-07T07:09:16Z

/integrate

openjdk · 2025-04-07T07:09:54Z

Going to push as commit 6d9ece7.
Since your change was applied there have been 38 commits pushed to the master branch:

97ed536: 8346989: C2: deoptimization and re-execution cycle with Math.*Exact in case of frequent overflow
660b17a: 8350852: Implement JMH benchmark for sparse CodeCache
6d37e63: 8353753: Remove unnecessary forward declaration in oop.hpp
9bb804b: 8338554: Fix inconsistencies in javadoc/doclet/testLinkOption/TestRedirectLinks.java
e17c399: 8353201: Open source Swing Tooltip tests - Set 2
9fcb06f: 8352997: Open source several Swing JTabbedPane tests
74c2d8f: 8353320: Open source more Swing text tests
31a6de2: 8353309: Open source several Swing text tests
3cc43b3: 8349075: Once again allow -compilejdk in JAVA_OPTIONS
9fb3609: 8350774: Generated test- targets broken after JDK-8348998
... and 28 more: https://git.openjdk.org/jdk/compare/bd74922157230c866802b4c5269da81e872525aa...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-07T07:10:12Z

@robehn Pushed as commit 6d9ece7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

robehn added 3 commits March 13, 2025 10:11

UseNewCode

7205674

Fixed comment

e2c2dae

Revert NC

413317a

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 13, 2025

Fixed ws

0fdda55

robehn marked this pull request as ready for review March 13, 2025 15:50

openjdk bot added the rfr Pull request is ready for review label Mar 13, 2025

RealFYang reviewed Mar 14, 2025

View reviewed changes

src/hotspot/cpu/riscv/riscv.ad Outdated Show resolved Hide resolved

robehn added 2 commits March 14, 2025 12:35

Review comments

4279f9f

Merge branch 'master' into tso-merge

a451ade

RealFYang reviewed Mar 17, 2025

View reviewed changes

src/hotspot/cpu/riscv/riscv.ad Outdated Show resolved Hide resolved

src/hotspot/cpu/riscv/riscv.ad Outdated Show resolved Hide resolved

src/hotspot/cpu/riscv/riscv.ad Outdated Show resolved Hide resolved

src/hotspot/cpu/riscv/riscv.ad Outdated Show resolved Hide resolved

Review comments

566c0a7

RealFYang approved these changes Mar 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 18, 2025

robehn added 2 commits March 19, 2025 07:48

Merge branch 'master' into tso-merge

e0e4fff

format comment

cb18420

openjdk bot removed the ready Pull request is ready to be integrated label Mar 19, 2025

RealFYang approved these changes Mar 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 19, 2025

robehn added 2 commits March 26, 2025 08:43

Merge branch 'master' into tso-merge

5eac847

Merge branch 'master' into tso-merge

c2688a6

feilongjiang approved these changes Mar 29, 2025

View reviewed changes

Merge branch 'master' into tso-merge

f09ea73

Hamlin-Li reviewed Apr 2, 2025

View reviewed changes

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp Show resolved Hide resolved

src/hotspot/cpu/riscv/riscv.ad Show resolved Hide resolved

Merge branch 'master' into tso-merge

2044cf5

Hamlin-Li approved these changes Apr 3, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 7, 2025

openjdk bot closed this Apr 7, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 7, 2025

robehn deleted the tso-merge branch May 9, 2025 16:43

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

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

Uh oh!

Conversation

robehn commented Mar 13, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

mlbridge bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

RealFYang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robehn commented Mar 14, 2025

Uh oh!

robehn commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

robehn commented Mar 19, 2025

Uh oh!

feilongjiang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robehn commented Mar 29, 2025

Uh oh!

Hamlin-Li left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robehn commented Apr 7, 2025

Uh oh!

robehn commented Apr 7, 2025

Uh oh!

openjdk bot commented Apr 7, 2025

Uh oh!

openjdk bot commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

robehn commented Mar 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 13, 2025 •

edited

Loading

mlbridge bot commented Mar 13, 2025 •

edited

Loading

RealFYang left a comment •

edited

Loading

robehn commented Mar 14, 2025 •

edited

Loading

feilongjiang left a comment •

edited

Loading