jdk
jdk copied to clipboard
openjdk
8321010: RISC-V: C2 RoundVF
Hi, Can you have a review on this patch to add RoundVF/RoundDF intrinsics? Thanks!
Tests
test/hotspot/jtreg/compiler/vectorization/TestRoundVectRiscv64.java test/hotspot/jtreg/compiler/c2/cr6340864/TestFloatVect.java test/hotspot/jtreg/compiler/c2/cr6340864/TestDoubleVect.java test/hotspot/jtreg/compiler/floatingpoint/TestRound.java
test/jdk/java/lang/Math/RoundTests.java
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issues
- JDK-8321010: RISC-V: C2 RoundVF (Sub-task - P4)
- JDK-8321011: RISC-V: C2 RoundVD (Sub-task - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17745/head:pull/17745
$ git checkout pull/17745
Update a local copy of the PR:
$ git checkout pull/17745
$ git pull https://git.openjdk.org/jdk.git pull/17745/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17745
View PR using the GUI difftool:
$ git pr show -t 17745
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17745.diff
Webrev
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@Hamlin-Li The following label will be automatically applied to this pull request:
-
hotspot-compiler
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.
Webrevs
- 15: Full (c35fcddc)
- 14: Full - Incremental (24559788)
- 13: Full - Incremental (32baf7e7)
- 12: Full - Incremental (9cdc198e)
- 11: Full - Incremental (066e9b7d)
- 10: Full - Incremental (8f3af2f7)
- 09: Full - Incremental (42f582e2)
- 08: Full (708fa0d9)
- 07: Full - Incremental (ffed86b1)
- 06: Full (a9faee89)
- 05: Full (2b57205f)
- 04: Full (baca01db)
- 03: Full (b7081bc9)
- 02: Full (70c0e99d)
- 01: Full - Incremental (1695aeb4)
- 00: Full (c7c87bf5)
@Hamlin-Li
Adding additional issue to solves list: 8321011: RISC-V: C2 RoundVD.
Hello Hamlin, I recall you had licheepi board. I would be nice if you can try to measure rvv performance gain with this https://github.com/syntacore/syntaj21/tree/rvv0.7.1
This PR showed it's not always easy to win perf just by using rvv - https://github.com/openjdk/jdk/pull/17413
I understand it might not be possible, but would be nice to give it a try (I can share hsdis with support for 0.7.1 if needed)
Hey, I'll be on vacation for a while, so maybe slow in response. But please leave your comments freely as usual.
Hello Hamlin, I recall you had licheepi board. I would be nice if you can try to measure rvv performance gain with this https://github.com/syntacore/syntaj21/tree/rvv0.7.1
This PR showed it's not always easy to win perf just by using rvv - #17413
I understand it might not be possible, but would be nice to give it a try (I can share hsdis with support for 0.7.1 if needed)
Had a internal discussion about your suggestion, seems 0.7.1 is not incompatible with 1.0/2.0, and for this simple intrinsic, we think a better path is to have it first, then re-visit it when we have real hardware to measure the performance later.
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@Hamlin-Li: Thanks for the quick update. Considering that saving/restoring for FRM could be expensive, I do wonder if we could gather some performance numbers before we go. I see people are now testing on RVV-1.0 hardwares [1] and I am also trying to get one (AFAIK, more powerful RVV-1.0 hardwares are also coming later this year, SG2044, SG2380, etc.). Also from discussion on [2], I see there are also other approaches available there without flipping the FP rounding mode. But I am not sure if they make sense for our case or work better without actual testing.
[1] https://github.com/openjdk/jdk/pull/18382#issuecomment-2045145255 [2] https://github.com/openjdk/jdk/pull/8204
@Hamlin-Li: Thanks for the quick update. Considering that saving/restoring for FRM could be expensive, I do wonder if we could gather some performance numbers before we go. I see people are now testing on RVV-1.0 hardwares [1] and I am also trying to get one (AFAIK, more powerful RVV-1.0 hardwares are also coming later this year, SG2044, SG2380, etc.). Also from discussion on [2], I see there are also other approaches available there without flipping the FP rounding mode. But I am not sure if they make sense for our case or work better without actual testing.
[1] #18382 (comment) [2] #8204
Thanks for the information, let me do some investigation on the solution in https://github.com/openjdk/jdk/pull/8204.
@Hamlin-Li this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout round-F+D-v
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@Hamlin-Li This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!