8315841: RISC-V: Check for hardware TSO support #15613

luhenry · 2023-09-07T09:00:50Z

With the Ztso extension [1], some hardware will support TSO on RISC-V. That allows us to reduce the generation of memory fences, given the stronger memory model compared to RVWMO.

[1] https://github.com/riscv/riscv-isa-manual/blob/6dcbc6da9ada01f0f57da83cda6059bdec57619f/src/ztso-st-ext.adoc#L1

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-8315841: RISC-V: Check for hardware TSO support (Task - P4)

Reviewers

Vladimir Kempik (@VladimirKempik - Committer) ⚠️ Review applies to 5f80c7ca
Robbin Ehn (@robehn - Reviewer)
Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15613

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

Using diff file

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

Webrev

Link to Webrev Comment

With the Ztso extension [1], some hardware will support TSO on RISC-V. That allows us to reduce the generation of memory fences, given the stronger memory model compared to RVWMO. [1] https://github.com/riscv/riscv-isa-manual/blob/6dcbc6da9ada01f0f57da83cda6059bdec57619f/src/ztso-st-ext.adoc#L1

bridgekeeper · 2023-09-07T09:01:46Z

👋 Welcome back luhenry! 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 · 2023-09-07T09:03:18Z

@luhenry 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 · 2023-09-07T09:07:00Z

Webrevs

VladimirKempik · 2023-09-07T10:25:23Z

Hello Ludovic, will this ( and hw support for tso) help to reduce overhead from getfield IsDone ?
it's sometimes the place where cpu spends most of time in some jmh tests:

62.78%    0x0000003fdcfb6670:   fence	ir,iorw                     ;*getfield isDone {reexecute=0 rethrow=0 return_oop=0}
                                                                      ; - org.openjdk.bench.java.lang.jmh_generated.MathBench_unsignedMultiplyHighLongLong_jmhTest::unsignedMultiplyHighLongLong_thrpt_jmhStub@30 (line 121)

luhenry · 2023-09-07T11:17:29Z

fence ir,iorw would not match (predecessor & w) && (successor & r), leading to not generating the fence. In the end, only the following fences would be generated: rw,r, rw,rw w,r, w,rw.

Some of the most common cases of fences that are going to be generated are fence w,r (used for sun.misc.Unsafe::fullFence) and fence rw,rw (generated for MacroAssembler::AnyAny).

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp

VladimirKempik · 2023-09-07T11:27:25Z

IIRC, the TSO elf files will have special bit set in elf header.
So we would have to build whole java for TSO mode, then UseZtso could be a static compile-time option
https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg281514.html

it could be

#if TARGET_ZTSO
UseZtso=true;
#fi

luhenry · 2023-09-07T11:38:33Z

So we would have to build whole java for TSO mode, then UseZtso could be a static compile-time option

We don't have to build hotspot for TSO in order to use it in code generated for Java, given the RVTSO memory model is strictly stronger than RVWMO.

We could indeed set UseZtso statically in case hotspot is compiled with TSO. ~~I don't know how to check the ELF-compiled options, would you know how we do it elsewhere?~~ (you just updated your comment with how to do it)

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp

robehn

Thanks, looks good (minus a nit).

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp

robehn · 2023-09-07T12:44:09Z

src/hotspot/cpu/riscv/vm_version_riscv.cpp

@@ -210,6 +210,14 @@ void VM_Version::initialize() {
      unaligned_access.value() == MISALIGNED_FAST);
  }

+#if defined(TARGET_ZTSO) && TARGET_ZTSO


If someone compiles with "CXXFLAGS=-marchrv64....ztso..", we need to try to parse the supplied flags, that doesn't seem like fun.
Instead I suggest we add code to read-out the elf flags, i.e:
"Flags: 0x15, RVC, double-float ABI, TSO"

And set UseZtso:
A: If this is a TSO elf.
B: If hwprobe says this TSO hardware (either directly or via vendor).
C: If someone set flag,

I guess your idea was to have a flag like --enable-tso which sets TARGET_TSO ?
If we have that or not I still like above to happen.

(I'm not saying you should do any of this in this PR, I can file new ones)

TARGET_TSO is set by gcc directly. See https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg281514.html

Ah, it is not set by LLVM what I can see at least (running a couple of weeks old tip).

Both llvm/gcc should define __riscv_ztso if this is compiled with tso. (@luhenry already updated PR)
Which means point A look in elf would never be needed,
B and C are already done in this PR.

Thanks for the update @luhenry !

openjdk · 2023-09-07T12:49:41Z

@luhenry 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:

8315841: RISC-V: Check for hardware TSO support

Reviewed-by: vkempik, rehn, fyang

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 40 new commits pushed to the master branch:

dab1c21: 8314491: Linux: jexec launched via PATH fails to find java
9a83d55: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
68f6941: 8314452: Explicitly indicate inlining success/failure in PrintInlining
b482e6d: 8315580: Remove unused java_lang_String::set_value_raw()
9b0da48: 8315410: Undocumented exceptions in java.text.StringCharacterIterator
578ded4: 8312418: Add Elements.getEnumConstantBody
dccf670: 8306632: Add a JDK Property for specifying DTD support
a62c48b: 8315891: java/foreign/TestLinker.java failed with "error occurred while instantiating class TestLinker: null"
9559e03: 8315578: PPC builds are broken after JDK-8304913
e409d07: 8315696: SignedLoggerFinderTest.java test failed
... and 30 more: https://git.openjdk.org/jdk/compare/9887cd8adc408a71b045b1a4891cc0d5dede7e0e...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.

VladimirKempik · 2023-09-07T13:08:50Z

Maybe this way:
still needs a good comment for cases when we don't generate fence

   if (UseZtso) {
      if ((pred_succ_to_membar_mask(predecessor, successor) & StoreLoad) == 0) {
             return;
   }
      // always generate fence for RVWMO
      Assembler::fence(predecessor, successor);

robehn

LGTM !

RealFYang · 2023-09-08T06:08:14Z

src/hotspot/cpu/riscv/vm_version_riscv.cpp

@@ -210,6 +210,14 @@ void VM_Version::initialize() {
      unaligned_access.value() == MISALIGNED_FAST);
  }

+#ifdef __riscv_ztso


May I ask where is this __riscv_ztso macro defined / specified? I tried to search it in the gcc user manual [1] and gcc source code, but I found nothing about it.

[1] https://gcc.gnu.org/onlinedocs/gcc.pdf

Good question, I don't find docs in the compilers for this either, but:
https://github.com/riscv-non-isa/riscv-toolchain-conventions#cc-preprocessor-definitions

We checked for it with this simple snippet. You can see that in both cases, it returns 0 which implies TSO is enabled.

OK. I think I have found its definition in gcc source code [1]. It's strange that this is not mentioned in [2].

[1] https://github.com/llvm/llvm-project/blob/523c471250a49b5603bd907ff05535f18ef61c91/clang/lib/Basic/Targets/RISCV.cpp#L160C5-L162C77
[2] https://github.com/riscv-non-isa/riscv-toolchain-conventions#cc-preprocessor-definitions

RealFYang

LGTM. Any tests performed?

luhenry · 2023-09-11T07:58:34Z

@RealFYang I've tested with jcstress on QEMU on x86 with the change and with an obvious breakage (not emitting barriers at all). It would fail when not emitting barriers at all (makes sense, x86 is not a sequential consistency memory model) and it would succeed with this patch (also makes sense given x86 is a TSO).

It's obviously lacking testing on an actual hardware that supports TSO and RVWMO, but I'm not aware of any on the market at the moment. We (Rivos) will make sure to test as soon as we have our hardware available, and we'll report back.

luhenry · 2023-09-11T09:01:49Z

/integrate

openjdk · 2023-09-11T09:02:43Z

Going to push as commit 35bccac.
Since your change was applied there have been 41 commits pushed to the master branch:

a04c6c1: 8315609: Open source few more swing text/html tests
dab1c21: 8314491: Linux: jexec launched via PATH fails to find java
9a83d55: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
68f6941: 8314452: Explicitly indicate inlining success/failure in PrintInlining
b482e6d: 8315580: Remove unused java_lang_String::set_value_raw()
9b0da48: 8315410: Undocumented exceptions in java.text.StringCharacterIterator
578ded4: 8312418: Add Elements.getEnumConstantBody
dccf670: 8306632: Add a JDK Property for specifying DTD support
a62c48b: 8315891: java/foreign/TestLinker.java failed with "error occurred while instantiating class TestLinker: null"
9559e03: 8315578: PPC builds are broken after JDK-8304913
... and 31 more: https://git.openjdk.org/jdk/compare/9887cd8adc408a71b045b1a4891cc0d5dede7e0e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-11T09:02:51Z

@luhenry Pushed as commit 35bccac.

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

openjdk bot added the rfr Pull request is ready for review label Sep 7, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 7, 2023

VladimirKempik reviewed Sep 7, 2023

View reviewed changes

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp Outdated Show resolved Hide resolved

fixup! 8315841: RISC-V: Check for hardware TSO support

1c397ad

luhenry added 2 commits September 7, 2023 11:42

fixup! 8315841: RISC-V: Check for hardware TSO support

e9691f3

fixup! 8315841: RISC-V: Check for hardware TSO support

5f80c7c

VladimirKempik approved these changes Sep 7, 2023

View reviewed changes

theRealAph reviewed Sep 7, 2023

View reviewed changes

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp Outdated Show resolved Hide resolved

robehn approved these changes Sep 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 7, 2023

luhenry added 3 commits September 7, 2023 12:58

fixup! 8315841: RISC-V: Check for hardware TSO support

ce42f97

fixup! 8315841: RISC-V: Check for hardware TSO support

c41c3e7

fixup! 8315841: RISC-V: Check for hardware TSO support

b40a28b

luhenry added 2 commits September 7, 2023 15:46

fixup! 8315841: RISC-V: Check for hardware TSO support

ec30b9a

fixup! 8315841: RISC-V: Check for hardware TSO support

98f485e

robehn approved these changes Sep 8, 2023

View reviewed changes

RealFYang reviewed Sep 8, 2023

View reviewed changes

RealFYang approved these changes Sep 11, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 11, 2023

openjdk bot closed this Sep 11, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8315841: RISC-V: Check for hardware TSO support #15613

8315841: RISC-V: Check for hardware TSO support #15613

luhenry commented Sep 7, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 7, 2023

openjdk bot commented Sep 7, 2023

mlbridge bot commented Sep 7, 2023 •

edited

Loading

VladimirKempik commented Sep 7, 2023 •

edited

Loading

luhenry commented Sep 7, 2023

VladimirKempik commented Sep 7, 2023 •

edited

Loading

luhenry commented Sep 7, 2023

robehn left a comment

robehn Sep 7, 2023

luhenry Sep 7, 2023

robehn Sep 7, 2023

robehn Sep 8, 2023

openjdk bot commented Sep 7, 2023 •

edited

Loading

VladimirKempik commented Sep 7, 2023

robehn left a comment

RealFYang Sep 8, 2023 •

edited

Loading

robehn Sep 8, 2023

luhenry Sep 8, 2023

RealFYang Sep 9, 2023

RealFYang left a comment

luhenry commented Sep 11, 2023

luhenry commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

8315841: RISC-V: Check for hardware TSO support #15613

8315841: RISC-V: Check for hardware TSO support #15613

Conversation

luhenry commented Sep 7, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 7, 2023

openjdk bot commented Sep 7, 2023

mlbridge bot commented Sep 7, 2023 • edited Loading

Webrevs

VladimirKempik commented Sep 7, 2023 • edited Loading

luhenry commented Sep 7, 2023

VladimirKempik commented Sep 7, 2023 • edited Loading

luhenry commented Sep 7, 2023

robehn left a comment

Choose a reason for hiding this comment

robehn Sep 7, 2023

Choose a reason for hiding this comment

luhenry Sep 7, 2023

Choose a reason for hiding this comment

robehn Sep 7, 2023

Choose a reason for hiding this comment

robehn Sep 8, 2023

Choose a reason for hiding this comment

openjdk bot commented Sep 7, 2023 • edited Loading

VladimirKempik commented Sep 7, 2023

robehn left a comment

Choose a reason for hiding this comment

RealFYang Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

robehn Sep 8, 2023

Choose a reason for hiding this comment

luhenry Sep 8, 2023

Choose a reason for hiding this comment

RealFYang Sep 9, 2023

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

luhenry commented Sep 11, 2023

luhenry commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

luhenry commented Sep 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 7, 2023 •

edited

Loading

VladimirKempik commented Sep 7, 2023 •

edited

Loading

VladimirKempik commented Sep 7, 2023 •

edited

Loading

openjdk bot commented Sep 7, 2023 •

edited

Loading

RealFYang Sep 8, 2023 •

edited

Loading