8321010: RISC-V: C2 RoundVF #17745

Hamlin-Li · 2024-02-07T09:58:35Z

Hi,
Can you have a review on this patch to add RoundVF/RoundDF intrinsics?

Current test shows that, it bring performance gain when vlenb >= 32 (which is on bananapi), but bring regression when vlenb == 16 (which is on k230). So I only enable the intrinsic when vlenb >= 32.

For double, even vlenb == 32, there is still some regression, so in this pr I only enable it when vlenb >= 64. Although there is no hardware to verify it, I think from the trend of performance data on bananapi and k230, it's promising to bring better performance rather than regression for double when vlenb == 64+. Please compare the data of Benchmark on bananapi, +UseSuperWord and Benchmark on k230, +UseSuperWord, enable RoundVF/D when vlenb == 16.

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

Performance - with Intrinsic

on bananapi

Benchmark on bananapi, +UseSuperWord

Benchmark on bananapi, +UseSuperWord	(TESTSIZE)	Mode	Cnt	Score +intrinsic	Score -intrinsic	Error	Units	Improvement
FpRoundingBenchmark.test_round_double	2048	avgt	10	23794.153	20557.467	2899.266	ns/op	0.864
FpRoundingBenchmark.test_round_float	2048	avgt	10	11531.853	16562.431	865.779	ns/op	1.436

on k230 (enable intrinsic even when vlenb == 16)

Benchmark on k230, +UseSuperWord, enable RoundVF/D when vlenb == 16

Benchmark on k230, +UseSuperWord, enable RoundVF/D when vlenb == 16	(TESTSIZE)	Mode	Cnt	Score +intrinsic	Error	Units	Score -intrinsic	improvement
FpRoundingBenchmark.test_round_double	2048	avgt	10	47049.826	2507.496	ns/op	23300.526	0.495
FpRoundingBenchmark.test_round_float	2048	avgt	10	22353.826	1020.517	ns/op	18149.228	0.812

Performance - without Intrinsic

on bananapi, intrinsic disabled due to -UseSuperWord

Benchmark on bananapi, -UseSuperWord

Benchmark on bananapi, -UseSuperWord	(TESTSIZE)	Mode	Cnt	Score +intrinsic	Score -intrinsic	Error	Units	Improvement
FpRoundingBenchmark.test_round_double	2048	avgt	10	22651.12	22954.95	1182.26	ns/op	1.013
FpRoundingBenchmark.test_round_float	2048	avgt	10	21722.434	22082.771	735.31	ns/op	1.017

on k230, intrinsic disabled due to -UseSuperWord

Benchmark on k230, -UseSuperWord

Benchmark on k230, -UseSuperWord	(TESTSIZE)	Mode	Cnt	Score	Error	Units	Score -intrinsic	improvement
FpRoundingBenchmark.test_round_double	2048	avgt	10	23312.201	1049.356	ns/op	23350.433	1.002
FpRoundingBenchmark.test_round_float	2048	avgt	10	20174.847	1039.798	ns/op	20138.565	0.998

on k230, intrinsic disabled due to vlenb == 16

Benchmark on k230, +UseSuperWord

Benchmark on k230, +UseSuperWord	(TESTSIZE)	Mode	Cnt	Score +intrinsic	Error	Units	Score -intrinsic	improvement
FpRoundingBenchmark.test_round_double	2048	avgt	10	23353.978	1063.472	ns/op	23277.546	0.997
FpRoundingBenchmark.test_round_float	2048	avgt	10	18168.97	883.562	ns/op	18137.665	0.998

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

Issues

JDK-8321010: RISC-V: C2 RoundVF (Sub-task - P4)
JDK-8321011: RISC-V: C2 RoundVD (Sub-task - P4)

Reviewers

Ludovic Henry (@luhenry - Committer) Review applies to 24559788
Robbin Ehn (@robehn - Reviewer)

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

Link to Webrev Comment

bridgekeeper · 2024-02-07T09:59:10Z

👋 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-02-07T09:59:36Z

@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.

Hamlin-Li · 2024-02-07T10:02:01Z

/solves JDK-8321011

mlbridge · 2024-02-07T10:02:18Z

Webrevs

openjdk · 2024-02-07T10:02:50Z

@Hamlin-Li
Adding additional issue to solves list: 8321011: RISC-V: C2 RoundVD.

VladimirKempik · 2024-02-07T14:11:05Z

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)

Hamlin-Li · 2024-02-07T15:22:30Z

@VladimirKempik Thanks for your comments, I'll consider it.

Hamlin-Li · 2024-02-07T15:22:35Z

Hey, I'll be on vacation for a while, so maybe slow in response.
But please leave your comments freely as usual.

test/hotspot/jtreg/compiler/vectorization/TestRoundVectRiscv64.java

Hamlin-Li · 2024-02-21T16:30:24Z

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.

openjdk · 2024-03-13T20:13:56Z

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

8321010: RISC-V: C2 RoundVF
8321011: RISC-V: C2 RoundVD

Reviewed-by: rehn, 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 436 new commits pushed to the master branch:

5709c37: 8340081: Test java/foreign/TestLinker.java failed failed: missing permission java.lang.foreign.native.threshold.power.fill
b88ff9c: 8339849: Enumerate opto and C1 stubs, generate enums, names, fields and generator calls
ae75ca0: 8314842: zgc/genzgc tests ignore vm flags
5e5942a: 8339794: Open source closed choice tests Azure Pipelines continuous integration (without running tests, for now) #1
81ff91e: 8339531: Improve performance of MemorySegment::mismatch
ab9b72c: 8329035: New Data Destination instructions support
7f1dae1: 8339874: Avoid duplicate checking of trailing slash in ZipFile.getZipEntry
4d65c3e: 8339876: Move constant symbol caches to Utf8EntryImpl
0765917: 8340011: Simplify jdk.internal.classfile.impl.EntryMap
1b17e0b: 8338747: hasIncubatorModules needs to be generated when module resolution required at startup
... and 426 more: https://git.openjdk.org/jdk/compare/be34730fb4e6818ac13c46b34b735c967351e5cd...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.

src/hotspot/cpu/riscv/riscv_v.ad

Hamlin-Li · 2024-08-19T07:24:22Z

Hey, Is someone avaiable to review this patch again? Thanks!

src/hotspot/cpu/riscv/riscv.ad

luhenry · 2024-09-02T08:38:12Z

@RealFYang @turbanoff could we please have another review? Thank you!

robehn

Seems alright, thanks!

Hamlin-Li · 2024-09-13T07:45:31Z

Thanks @luhenry @robehn for you reviewing!

/integrate

openjdk · 2024-09-13T07:45:41Z

@Hamlin-Li This pull request has not yet been marked as ready for integration.

Hamlin-Li · 2024-09-13T08:03:29Z

/integrate

openjdk · 2024-09-13T08:05:19Z

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

5709c37: 8340081: Test java/foreign/TestLinker.java failed failed: missing permission java.lang.foreign.native.threshold.power.fill
b88ff9c: 8339849: Enumerate opto and C1 stubs, generate enums, names, fields and generator calls
ae75ca0: 8314842: zgc/genzgc tests ignore vm flags
5e5942a: 8339794: Open source closed choice tests Azure Pipelines continuous integration (without running tests, for now) #1
81ff91e: 8339531: Improve performance of MemorySegment::mismatch
ab9b72c: 8329035: New Data Destination instructions support
7f1dae1: 8339874: Avoid duplicate checking of trailing slash in ZipFile.getZipEntry
4d65c3e: 8339876: Move constant symbol caches to Utf8EntryImpl
0765917: 8340011: Simplify jdk.internal.classfile.impl.EntryMap
1b17e0b: 8338747: hasIncubatorModules needs to be generated when module resolution required at startup
... and 426 more: https://git.openjdk.org/jdk/compare/be34730fb4e6818ac13c46b34b735c967351e5cd...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-13T08:05:24Z

@Hamlin-Li Pushed as commit bacd046.

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

Hamlin-Li and others added 7 commits December 29, 2023 19:58

RoundVF/D: Initial commit

86397fa

refine code

ad92189

Merge branch 'master' into round-F+D-v

029cfe6

Fix corner cases

d6cf04b

v2: (src + 0.5) + rdn

1b6b7e9

add test cases

ae70357

add tests

c7c87bf

openjdk bot added the rfr Pull request is ready for review label Feb 7, 2024

openjdk bot added the hotspot-compiler [email protected] label Feb 7, 2024

turbanoff reviewed Feb 14, 2024

View reviewed changes

test/hotspot/jtreg/compiler/vectorization/TestRoundVectRiscv64.java Outdated Show resolved Hide resolved

turbanoff reviewed Feb 14, 2024

View reviewed changes

test/hotspot/jtreg/compiler/vectorization/TestRoundVectRiscv64.java Outdated Show resolved Hide resolved

fix space

1695aeb

merge master

70c0e99

RealFYang suggested changes Apr 9, 2024

View reviewed changes

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

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

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

hamlin added 3 commits April 9, 2024 16:35

fix minors

ba4cd66

Merge branch 'master' into round-F+D-v

457fcfa

restore round mode back to rne

b7081bc

Hamlin-Li mentioned this pull request Apr 12, 2024

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

Closed

3 tasks

hamlin added 2 commits April 18, 2024 11:24

Merge branch 'master' into round-F+D-v

baca01d

Merge branch 'master' into round-F+D-v

2b57205

hamlin added 8 commits July 24, 2024 11:05

Merge branch 'master' into round-F+D-v

708fa0d

enable intrinsic when MaxVectorSize >= 32

42f582e

enable roundVD when MaxVectorSize >= 64

8f3af2f

add additional tests

066e9b7

minor

9cdc198

minor

32baf7e

minor

2455978

Merge branch 'master' into round-F+D-v

959c92e

luhenry approved these changes Aug 28, 2024

View reviewed changes

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

comments

c35fcdd

robehn approved these changes Sep 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 13, 2024

minor

68608fb

openjdk bot removed the ready Pull request is ready to be integrated label Sep 13, 2024

robehn approved these changes Sep 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 13, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 13, 2024

openjdk bot closed this Sep 13, 2024

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

Hamlin-Li deleted the round-F+D-v branch September 18, 2024 12:43

DingliZhang mentioned this pull request Sep 24, 2024

8320998: RISC-V: C2 RoundDoubleModeV #21164

Closed

3 tasks

Hamlin-Li mentioned this pull request Mar 13, 2025

8351876: RISC-V: enable and fix some float round tests #24015

Closed

3 tasks

8321010: RISC-V: C2 RoundVF #17745

8321010: RISC-V: C2 RoundVF #17745

Uh oh!

Conversation

Hamlin-Li commented Feb 7, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Performance - with Intrinsic

on bananapi

on k230 (enable intrinsic even when vlenb == 16)

Performance - without Intrinsic

on bananapi, intrinsic disabled due to -UseSuperWord

on k230, intrinsic disabled due to -UseSuperWord

on k230, intrinsic disabled due to vlenb == 16

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Feb 7, 2024

Uh oh!

openjdk bot commented Feb 7, 2024

Uh oh!

Hamlin-Li commented Feb 7, 2024

Uh oh!

mlbridge bot commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Feb 7, 2024

Uh oh!

VladimirKempik commented Feb 7, 2024

Uh oh!

Hamlin-Li commented Feb 7, 2024

Uh oh!

Hamlin-Li commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

Hamlin-Li commented Feb 21, 2024

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hamlin-Li commented Aug 19, 2024

Uh oh!

Uh oh!

luhenry commented Sep 2, 2024

Uh oh!

robehn left a comment

Choose a reason for hiding this comment

Uh oh!

Hamlin-Li commented Sep 13, 2024

Uh oh!

openjdk bot commented Sep 13, 2024

Uh oh!

Hamlin-Li commented Sep 13, 2024

Uh oh!

openjdk bot commented Sep 13, 2024

Uh oh!

openjdk bot commented Sep 13, 2024

Uh oh!

Uh oh!

Hamlin-Li commented Feb 7, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 7, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading