8330094: RISC-V: Save and restore FRM in the call stub #18758

Hamlin-Li · 2024-04-12T12:16:44Z

Hi,
Can you help to review this patch?
As discussed at #17745 (comment), we should do the similar thing as JDK-8319973 on aarch64.
Thanks!

Tests running ...

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-8330094: RISC-V: Save and restore FRM in the call stub (Enhancement - P4)

Reviewers

Fei Yang (@RealFYang - Reviewer)
Ludovic Henry (@luhenry - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18758

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-12T12:17:20Z

👋 Welcome back mli! 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 · 2024-04-12T12:17:49Z

@Hamlin-Li 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:

8330094: RISC-V: Save and restore FRM in the call stub

Reviewed-by: fyang, luhenry

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

003e86f: 8324755: Enable parallelism in vmTestbase/gc/gctests/LargeObjects tests
706b421: 8330467: NoClassDefFoundError when lambda is in a hidden class
fe0227e: 8311098: Change comment in verificationType.hpp to refer to _sym
0646284: 8317376: Minor improvements to the 'this' escape analyzer
4895a15: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object
fd331ff: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
9fd7802: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
192ec38: 8329595: spurious variable "might not have been initialized" on static final field
03e8417: 8329948: Remove string template feature
ff3e76f: 8330053: JFR: Use LocalDateTime instead ZonedDateTime
... and 84 more: https://git.openjdk.org/jdk/compare/7df492627b933f48750985c26de69be3f86115cb...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 · 2024-04-12T12:18:19Z

@Hamlin-Li 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 · 2024-04-12T12:21:02Z

Webrevs

RealFYang

Looks good. Thanks.
BTW: There is another path where FP control register could got clobbered is through JNI.
See: https://bugs.openjdk.org/browse/JDK-8320892. I think we might want to fix it for riscv too.

luhenry · 2024-04-15T08:49:25Z

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

@@ -415,6 +424,10 @@ class StubGenerator: public StubCodeGenerator {

    __ ld(x9, x9_save);

+    // restore fcsr
+    __ ld(t1, fcsr_save);
+    __ csrw(CSR_FCSR, t1);


It would be better to avoid the csrw in case the CSR is already set to RoundingMode::rne on some hardware. You can have avoid it with a simple check on the current value of FCSR.

Something along the line of:

__ ld(t1, fcsr_save); __ csrr(t0, CSR_FCSR); __ beq(t1, t0, skip_csrw); __ csrw(CSR_FCSR, t1); __ bind(skip_csrw);

Some doc about it: https://riscv-optimization-guide.riseproject.dev/#_controlling_rounding_behavior_scalar

Interesting. So is it safe to claim that csrr is faster than csrw? I didn't measure the difference.

It's fair to claim that csrw has side effects which may be more detrimental to performance, side effects that csrr that doesn't have (and can't as it's only reading).

I didn't measure the difference.

I don't know how that affects current hardware, but remember that current hardware are in-order CPUs which may not be impacted by many of these performance problems.

Another thing that is worth considering is that the fflags (exception flags) contained in fcsr is very likely to change after the Java call which does floating point calculations.

Should we replace read/write/comp of fcsr with frm only?

Yeah, that make sense to me. I think fflags is safe to ignore here.
(And you will also need to update this code comment in file: src/hotspot/cpu/riscv/stubGenerator_riscv.cpp if you do that: // -34 [ saved Floating-point Control and Status Register ] <--- sp_after_call)

Thanks for discussion, all updated.

luhenry · 2024-04-15T16:07:17Z

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

+    // don't want non-IEEE rounding modes.
+    Label skip_fsrmi;
+    __ mv(t1, __ RoundingMode::rne);
+    __ beq(t0, t1, skip_fsrmi);


You can take advantage of RoundingMode::rne == 0 by doing __ beq(t0, zr, skip_fsrmi) and remove the above __ mv(t1, __ RoundingMode::rne);. Please add a guarantee(__ RoundingMode::rne == 0) as well, just to self-document.

Thanks, fixed.

RealFYang · 2024-04-17T03:52:14Z

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

+    // don't want non-IEEE rounding modes.
+    Label skip_fsrmi;
+    guarantee(__ RoundingMode::rne == 0, "must be");
+    __ beq(t0, zr, skip_fsrmi);


Suggestion: __ beqz(t0, skip_fsrmi);

fixed, thanks

RealFYang · 2024-04-17T03:55:34Z

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

@@ -296,6 +299,16 @@ class StubGenerator: public StubCodeGenerator {
    __ fsd(f26, f26_save);
    __ fsd(f27, f27_save);

+    __ frrm(t0);
+    __ sd(t0, frm_save);
+    // Set fcsr to the state we need. We do want Round to Nearest. We


I think it will be more accurate to mention frm instead of fcsr in both code comment and the JBS title?

agree, fixed

Hamlin-Li · 2024-04-18T11:12:33Z

Thanks @luhenry @RealFYang for your reviewing.

/integrate

openjdk · 2024-04-18T11:13:11Z

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

4b55fe5: 8330520: linux clang build fails in os_linux.cpp with static_assert with no message is a C++17 extension
5eb2c59: 8330475: Remove unused default value for ModRefBarrierSet::write_ref_array_pre
003e86f: 8324755: Enable parallelism in vmTestbase/gc/gctests/LargeObjects tests
706b421: 8330467: NoClassDefFoundError when lambda is in a hidden class
fe0227e: 8311098: Change comment in verificationType.hpp to refer to _sym
0646284: 8317376: Minor improvements to the 'this' escape analyzer
4895a15: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object
fd331ff: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
9fd7802: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
192ec38: 8329595: spurious variable "might not have been initialized" on static final field
... and 86 more: https://git.openjdk.org/jdk/compare/7df492627b933f48750985c26de69be3f86115cb...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-18T11:13:17Z

@Hamlin-Li Pushed as commit b049609.

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

hamlin added 2 commits April 11, 2024 13:33

Initial commit

2825dfd

remove csrwi(..., rne)

75d27fa

openjdk bot added the rfr Pull request is ready for review label Apr 12, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Apr 12, 2024

RealFYang approved these changes Apr 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 15, 2024

luhenry suggested changes Apr 15, 2024

View reviewed changes

hamlin added 2 commits April 15, 2024 13:15

skip setting frm when not needed

a9db83b

update comment accordingly

0e7b241

Hamlin-Li mentioned this pull request Apr 15, 2024

8330266: RISC-V: Restore frm to RoundingMode::rne after JNI #18785

Closed

3 tasks

luhenry suggested changes Apr 15, 2024

View reviewed changes

refine code

7da4b99

RealFYang reviewed Apr 17, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Apr 17, 2024

Hamlin-Li changed the title ~~8330094: RISC-V: Save and restore FCSR in the call stub~~ 8330094: RISC-V: Save and restore FRM in the call stub Apr 17, 2024

openjdk bot added the ready Pull request is ready to be integrated label Apr 17, 2024

fix comment; minor refinement

1152248

RealFYang approved these changes Apr 18, 2024

View reviewed changes

luhenry approved these changes Apr 18, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 18, 2024

openjdk bot closed this Apr 18, 2024

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

Hamlin-Li deleted the save-store-fcsr-in-callstub branch April 18, 2024 11:20

Hamlin-Li mentioned this pull request Apr 18, 2024

8321010: RISC-V: C2 RoundVF #17745

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8330094: RISC-V: Save and restore FRM in the call stub #18758

8330094: RISC-V: Save and restore FRM in the call stub #18758

Hamlin-Li commented Apr 12, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 12, 2024

openjdk bot commented Apr 12, 2024 •

edited

Loading

openjdk bot commented Apr 12, 2024

mlbridge bot commented Apr 12, 2024 •

edited

Loading

RealFYang left a comment •

edited

Loading

luhenry Apr 15, 2024 •

edited

Loading

RealFYang Apr 15, 2024

luhenry Apr 15, 2024 •

edited

Loading

RealFYang Apr 15, 2024

Hamlin-Li Apr 15, 2024

RealFYang Apr 15, 2024 •

edited

Loading

Hamlin-Li Apr 15, 2024

luhenry Apr 15, 2024

Hamlin-Li Apr 15, 2024

RealFYang Apr 17, 2024

Hamlin-Li Apr 17, 2024

RealFYang Apr 17, 2024 •

edited

Loading

Hamlin-Li Apr 17, 2024

Hamlin-Li commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

8330094: RISC-V: Save and restore FRM in the call stub #18758

8330094: RISC-V: Save and restore FRM in the call stub #18758

Conversation

Hamlin-Li commented Apr 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 12, 2024

openjdk bot commented Apr 12, 2024 • edited Loading

openjdk bot commented Apr 12, 2024

mlbridge bot commented Apr 12, 2024 • edited Loading

Webrevs

RealFYang left a comment • edited Loading

Choose a reason for hiding this comment

luhenry Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luhenry Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealFYang Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealFYang Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamlin-Li commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

Hamlin-Li commented Apr 12, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 12, 2024 •

edited

Loading

mlbridge bot commented Apr 12, 2024 •

edited

Loading

RealFYang left a comment •

edited

Loading

luhenry Apr 15, 2024 •

edited

Loading

luhenry Apr 15, 2024 •

edited

Loading

RealFYang Apr 15, 2024 •

edited

Loading

RealFYang Apr 17, 2024 •

edited

Loading