8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub #10095

zhengxiaolinX · 2022-08-31T08:47:50Z

Hi team,

Tests described in JDK-8293100 are failed, for callee-saved float registers used in C++ code are observed being killed after entering the Java world since callee-saved float registers are unfortunately missed to be saved in StubGenerator::generate_call_stub(). So the phenomenon presented in the failed tests seems to be weird.
This patch also fixes some typos in vmreg_riscv.cpp when debugging the same issue:
(before)

This one seems not right, either; and it also gets fixed in this patch:
(after)
Also, branch nodes for float registers in OptoAssembly didn't show their destination blocks. This patch adds a little enhancement for those.

Hotspot tier1 has passed both on my board and qemu, and vmTestbase/nsk/stress tests failed before have passed as well.
The test process on the board is slow and I am testing the remaining but I consider this as ready for review.

Best,
Xiaolin

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-8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub

Reviewers

Yadong Wang (@yadongw - Author) ⚠️ Review applies to bd3adac7
Feilong Jiang (@feilongjiang - Author)
Aleksey Shipilev (@shipilev - Reviewer)
Vladimir Kempik (@VladimirKempik - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10095

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

Using diff file

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

…enerate_call_stub

bridgekeeper · 2022-08-31T08:49:42Z

👋 Welcome back xlinzheng! 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 · 2022-08-31T08:52:34Z

@zhengxiaolinX 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 · 2022-09-01T04:17:57Z

Webrevs

01: Full - Incremental (5e044111)
00: Full (bd3adac7)

VladimirKempik · 2022-09-01T07:02:07Z

vmTestbase/nsk/stress/jni/jnistress002.java now passes with this patch
Running now full tier4:hotspot ( which includes vmTestbase), will report on results later

yadongw

lgtm(not a reviewer)

feilongjiang

Looks good, with one comment.

feilongjiang · 2022-09-01T08:54:36Z

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp

-  // -22 [ argument word 1      ]
-  // -21 [ saved x27            ] <--- sp_after_call
+  // -34 [ argument word 1      ]
+  // -33 [ saved f27            ] <--- sp_after_call


I think it's better to add some comments about saving FloatRegisters at line 124.

Thank you, always forget the comment things. Seems x9 is also missed in the comments; it is added along with f8-f9 and f18-f27 now.

shipilev

Looks fine.

openjdk · 2022-09-01T11:40:00Z

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

8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub

Reviewed-by: yadongwang, fjiang, shade, vkempik

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

3ac91b0: 8293003: Review running time of Warn5 regression test
e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
99c3ab0: 8293010: JDI ObjectReference/referringObjects/referringObjects001 fails: assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking
0fb9469: 8290126: Add a check in JavadocTester for "javadoc should not crash"
0a4d0ce: 8293121: (fs) Refactor UnixFileSystem copying into generic Unix, Linux, and BSD implementations
032be16: 8292066: Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version
e393973: 8292990: Improve test coverage for XPath Axes: parent
fa68371: 8292584: assert(cb != __null) failed: must be with -XX:-Inline
04d8069: 8230374: maxOutputSize, instead of javatest.maxOutputSize, should be used in TEST.properties
3d254d3: 8289510: Improve test coverage for XPath Axes: namespace
... and 21 more: https://git.openjdk.org/jdk/compare/9e3176bd09c7210ccb125491abb6a6a4d852b6c6...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev, @VladimirKempik) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

zhengxiaolinX · 2022-09-02T02:55:47Z

vmTestbase/nsk/stress/jni/jnistress002.java now passes with this patch Running now full tier4:hotspot ( which includes vmTestbase), will report on results later

Hi Vladimir, my hotspot tier2~4 have passed. I was wondering if all's right with your hotspot tier4?

VladimirKempik · 2022-09-02T06:38:48Z

Hello
Tier4:hotspot is good on thead.

zhengxiaolinX · 2022-09-02T06:45:01Z

Thanks!
And thank you all for reviewing as always. Let's move on then.

/integrate

openjdk · 2022-09-02T06:46:12Z

@zhengxiaolinX
Your change (at version 5e04411) is now ready to be sponsored by a Committer.

RealFYang · 2022-09-02T06:59:36Z

/sponsor

openjdk · 2022-09-02T07:01:11Z

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

98ce45f: 8292981: Unify and restructure integer printing format specifiers
3ac91b0: 8293003: Review running time of Warn5 regression test
e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
99c3ab0: 8293010: JDI ObjectReference/referringObjects/referringObjects001 fails: assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking
0fb9469: 8290126: Add a check in JavadocTester for "javadoc should not crash"
0a4d0ce: 8293121: (fs) Refactor UnixFileSystem copying into generic Unix, Linux, and BSD implementations
032be16: 8292066: Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version
e393973: 8292990: Improve test coverage for XPath Axes: parent
fa68371: 8292584: assert(cb != __null) failed: must be with -XX:-Inline
04d8069: 8230374: maxOutputSize, instead of javatest.maxOutputSize, should be used in TEST.properties
... and 22 more: https://git.openjdk.org/jdk/compare/9e3176bd09c7210ccb125491abb6a6a4d852b6c6...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-02T07:01:29Z

@RealFYang @zhengxiaolinX Pushed as commit bc5ffc8.

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

zhengxiaolinX added 3 commits August 31, 2022 16:39

riscv: Fix VMReg names for Float/Vector Registers

a65ca8a

riscv: Print dest blocks of branch instructions for OptoAssembly

e6cb14e

riscv: Forgot to save callee-saved FloatRegisters in StubGenerator::g…

bd3adac

…enerate_call_stub

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 31, 2022

zhengxiaolinX changed the title ~~8293100: riscv: Forgot to save callee-saved FloatRegisters in StubGenerator::generate_call_stub~~ 8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub Aug 31, 2022

zhengxiaolinX marked this pull request as ready for review September 1, 2022 04:11

openjdk bot added the rfr Pull request is ready for review label Sep 1, 2022

yadongw approved these changes Sep 1, 2022

View reviewed changes

feilongjiang suggested changes Sep 1, 2022

View reviewed changes

Revise comments

5e04411

feilongjiang approved these changes Sep 1, 2022

View reviewed changes

shipilev approved these changes Sep 1, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 1, 2022

VladimirKempik approved these changes Sep 2, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 2, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 2, 2022

openjdk bot closed this Sep 2, 2022

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

openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 2, 2022

zhengxiaolinX deleted the callee-saved-float-missing branch September 2, 2022 07:01

8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub #10095

8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub #10095

zhengxiaolinX commented Aug 31, 2022 •

edited by openjdk bot

bridgekeeper bot commented Aug 31, 2022

openjdk bot commented Aug 31, 2022

mlbridge bot commented Sep 1, 2022 •

edited

VladimirKempik commented Sep 1, 2022

yadongw left a comment

feilongjiang left a comment

feilongjiang Sep 1, 2022

zhengxiaolinX Sep 1, 2022 •

edited

shipilev left a comment

openjdk bot commented Sep 1, 2022 •

edited

zhengxiaolinX commented Sep 2, 2022

VladimirKempik commented Sep 2, 2022

zhengxiaolinX commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

RealFYang commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub #10095

8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub #10095

Conversation

zhengxiaolinX commented Aug 31, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 31, 2022

openjdk bot commented Aug 31, 2022

mlbridge bot commented Sep 1, 2022 • edited

Webrevs

VladimirKempik commented Sep 1, 2022

yadongw left a comment

Choose a reason for hiding this comment

feilongjiang left a comment

Choose a reason for hiding this comment

feilongjiang Sep 1, 2022

Choose a reason for hiding this comment

zhengxiaolinX Sep 1, 2022 • edited

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 1, 2022 • edited

zhengxiaolinX commented Sep 2, 2022

VladimirKempik commented Sep 2, 2022

zhengxiaolinX commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

RealFYang commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

zhengxiaolinX commented Aug 31, 2022 •

edited by openjdk bot

mlbridge bot commented Sep 1, 2022 •

edited

zhengxiaolinX Sep 1, 2022 •

edited

openjdk bot commented Sep 1, 2022 •

edited