8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB #25005

Hamlin-Li · 2025-05-02T12:19:53Z

Hi,
Can you help to review this patch to add SUADD/SADD/SUSUB/SSUB for vector api?

Thanks!

Test

data

Benchmark	(size)	Mode	Cnt	Score - master	Score - patch	improvement (master/patch)
ByteMaxVector.SADD	1024	avgt	10	23693.941	381.441	62.117
ByteMaxVector.SSUB	1024	avgt	10	24067.009	379.836	63.362
ByteMaxVector.SUADD	1024	avgt	10	24131.819	382.678	63.06
ByteMaxVector.SUSUB	1024	avgt	10	23140.494	380.768	60.773
IntMaxVector.SADD	1024	avgt	10	88526.058	1378.77	64.207
IntMaxVector.SSUB	1024	avgt	10	94204.768	1383.613	68.086
IntMaxVector.SUADD	1024	avgt	10	82470.743	1384.668	59.56
IntMaxVector.SUSUB	1024	avgt	10	84443.805	1759.69	47.988
LongMaxVector.SADD	1024	avgt	10	187690.117	3770.84	49.774
LongMaxVector.SSUB	1024	avgt	10	187334.716	3814.869	49.106
LongMaxVector.SUADD	1024	avgt	10	186891.578	2747.753	68.016
LongMaxVector.SUSUB	1024	avgt	10	186092.582	2730.588	68.151
ShortMaxVector.SADD	1024	avgt	10	43991.814	726.703	60.536
ShortMaxVector.SSUB	1024	avgt	10	40560.356	730.238	55.544
ShortMaxVector.SUADD	1024	avgt	10	43349.632	729.758	59.403
ShortMaxVector.SUSUB	1024	avgt	10	42686.701	726.059	58.792

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-8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB (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/25005/head:pull/25005
$ git checkout pull/25005

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25005

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-02T12:20:53Z

👋 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 · 2025-05-02T12:20:58Z

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

8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB

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

ddd07b1: 8356172: IdealGraphPrinter doesn't need ThreadCritical
614ba9f: 8356075: Support Shenandoah GC in JVMCI
ecfaf35: 8356083: ZGC: Duplicate ZTestEntry symbols in gtests
... and 62 more: https://git.openjdk.org/jdk/compare/0cd0afb2b32abd77f6275cf34a499b5cb31f22b5...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 · 2025-05-02T12:21:26Z

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

mlbridge · 2025-05-02T12:24:17Z

Webrevs

01: Full - Incremental (466eb06c)
00: Full (7648da04)

RealFYang

Seems OK. I only have one minor comment.

RealFYang · 2025-05-05T10:17:36Z

src/hotspot/cpu/riscv/riscv_v.ad

+  predicate(n->is_SaturatingVector() && !n->as_SaturatingVector()->is_unsigned());
+  match(Set dst_src (SaturatingAddV (Binary dst_src src1) v0));
+  ins_cost(VEC_COST);
+  format %{ "vsadd_masked $dst_src, $dst_src, $src1" %}


Nit: Seems the mask register (v0) is missing in opto asm for these masked operations.
For integrity, we always print the mask register as the last operand for other masked nodes.
format %{ "vsadd_masked $dst_src, $dst_src, $src1, $v0" %}

Fixed. Thanks!

RealFYang

Updated change looks good.
What about the vector-scalar variants (vsaddu.vx, vsaddu.vi, etc.)? Do they help in any way?
BTW: Nice JMH numbers :-)

Hamlin-Li · 2025-05-06T11:17:57Z

Updated change looks good.

Thank you!

What about the vector-scalar variants (vsaddu.vx, vsaddu.vi, etc.)? Do they help in any way?

I think so, although not sure how much benefit it will bring, as it should be able to do a vmv first, then use the instructs in this patch, so there should be some improvement, but maybe just minor one.
And for other operations, like (signed/unsigned) max/min, mulb/s/i/l/f/d, and so on, I think we can also introduce the _vx and _vi version.
Maybe we could implement these bunch of instructs in another patch together?

RealFYang · 2025-05-07T01:48:17Z

What about the vector-scalar variants (vsaddu.vx, vsaddu.vi, etc.)? Do they help in any way?

I think so, although not sure how much benefit it will bring, as it should be able to do a vmv first, then use the instructs in this patch, so there should be some improvement, but maybe just minor one. And for other operations, like (signed/unsigned)

We already have some vector-scalar examples like vadd_vx, vadd_vi [1][2]. I guess it will be similar for this case as well.
Maybe just replicate the scalar src2 to get a vector in the match rule will do, like:
match(Set dst (SaturatingAddV src1 (Replicate src2)));

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/riscv_v.ad#L446
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/riscv_v.ad#L417

max/min, mulb/s/i/l/f/d, and so on, I think we can also introduce the _vx and _vi version. Maybe we could implement these bunch of instructs in another patch together?

Sure, OK.

Hamlin-Li · 2025-05-07T08:32:21Z

/integrate

openjdk · 2025-05-07T08:33:52Z

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

da004cb: 8356030: RISC-V: enable (part of) BasicDoubleOpTest.java
b5fd289: 8356310: compiler/print/TestPrintAssemblyDeoptRace.java fails with Improperly specified VM option 'DeoptimizeALot'
328715d: 8356187: TestJcmd.java may incorrectly parse podman version
... and 91 more: https://git.openjdk.org/jdk/compare/0cd0afb2b32abd77f6275cf34a499b5cb31f22b5...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-07T08:33:58Z

@Hamlin-Li Pushed as commit 1a4bbb0.

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

initial commit

7648da0

openjdk bot added the rfr Pull request is ready for review label May 2, 2025

openjdk bot added the hotspot-compiler [email protected] label May 2, 2025

RealFYang reviewed May 5, 2025

View reviewed changes

minor

466eb06

RealFYang approved these changes May 6, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 6, 2025

luhenry approved these changes May 6, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 7, 2025

openjdk bot closed this May 7, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 7, 2025

Hamlin-Li deleted the saturating-add-sub branch May 7, 2025 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB #25005

8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB #25005

Hamlin-Li commented May 2, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 2, 2025

openjdk bot commented May 2, 2025 •

edited

Loading

openjdk bot commented May 2, 2025

mlbridge bot commented May 2, 2025 •

edited

Loading

RealFYang left a comment

RealFYang May 5, 2025 •

edited

Loading

Hamlin-Li May 6, 2025

RealFYang left a comment •

edited

Loading

Hamlin-Li commented May 6, 2025

RealFYang commented May 7, 2025

Hamlin-Li commented May 7, 2025

openjdk bot commented May 7, 2025

openjdk bot commented May 7, 2025

8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB #25005

8355699: RISC-V: support SUADD/SADD/SUSUB/SSUB #25005

Conversation

Hamlin-Li commented May 2, 2025 • edited by openjdk bot Loading

Test

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 2, 2025

openjdk bot commented May 2, 2025 • edited Loading

openjdk bot commented May 2, 2025

mlbridge bot commented May 2, 2025 • edited Loading

Webrevs

RealFYang left a comment

Choose a reason for hiding this comment

RealFYang May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Hamlin-Li May 6, 2025

Choose a reason for hiding this comment

RealFYang left a comment • edited Loading

Choose a reason for hiding this comment

Hamlin-Li commented May 6, 2025

RealFYang commented May 7, 2025

Hamlin-Li commented May 7, 2025

openjdk bot commented May 7, 2025

openjdk bot commented May 7, 2025

Hamlin-Li commented May 2, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 2, 2025 •

edited

Loading

mlbridge bot commented May 2, 2025 •

edited

Loading

RealFYang May 5, 2025 •

edited

Loading

RealFYang left a comment •

edited

Loading