8311862: RISC-V: small improvements to shift immediate instructions #14823

Ilyagavrilin · 2023-07-11T09:02:39Z

Please review this small change for slli srli and srai
slli change allows to replace slli Rd, Rs, 0 with addi Rd, Rs, 0 (and no operation emited if Rd == Rs)
addi with 0 has higher chances to be just a register renaming and not utilise ALU at all
We have observed small positive effect on hifive (and no change on thead).

testing: tier1 and tier2 on hifive, also hotspot/jtreg/compiler/intrinsics/string tests on Qemu with UseZba

performance on hifive, before:

Benchmark	Mode	Cnt	Score		Error	Units
StringIndexOf.advancedWithShortSub1	avgt	25	4035.143	±	191.262	ns/op
StringIndexOf.advancedWithShortSub2	avgt	25	1145.807	±	14.610	ns/op

with patch:

Benchmark	Mode	Cnt	Score		Error	Units
StringIndexOf.advancedWithShortSub1	avgt	25	3613.943	±	178.153	ns/op
StringIndexOf.advancedWithShortSub2	avgt	25	923.169	±	47.123	ns/op

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-8311862: RISC-V: small improvements to shift immediate instructions (Enhancement - P4)

Reviewers

Ludovic Henry (@luhenry - Committer) ⚠️ Review applies to 2df17399
Feilong Jiang (@feilongjiang - Author) ⚠️ Review applies to 70bce628
Fei Yang (@RealFYang - Reviewer) ⚠️ Review applies to 6a35ea6f

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14823

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-11T09:04:02Z

👋 Welcome back Ilyagavrilin! 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 · 2023-07-11T09:05:52Z

@Ilyagavrilin 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 · 2023-07-11T09:10:16Z

Webrevs

luhenry

LGTM

feilongjiang

Looks good, with one nit:

feilongjiang · 2023-07-12T06:38:11Z

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

+  if (UseZba) {
+    _slli_uw(Rd, Rs, shamt);
+  } else {
+    slli(Rd, Rs, shamt+32);


Suggested change

slli(Rd, Rs, shamt+32);

slli(Rd, Rs, shamt + 32);

Thank you for your suggestion

RealFYang · 2023-07-14T03:18:31Z

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

+    _slli_uw(Rd, Rs, shamt);
+  } else {
+    slli(Rd, Rs, shamt + 32);
+    srli(Rd, Rd, 32);


I don't think this code in else block will work in the case when shamt >= 32. Note that the slli.uw instruction is the same as slli with zext.w performed on the least-significant word of Rs before shifting. So you might want to do a combination of 32-bit zero extension and slli on Rs instead.

slli.uw with shamt >= 32 will be same as add Rd, X0, X0, isn't it ?
He can just add handling for that special case then

that was wrong.

RealFYang · 2023-07-14T09:24:58Z

PS: I think we should also change the JBS title as I see this not only touches slli but also srli and srai.
Suggestion: "8311862: RISC-V: small improvements to shift immediate instructions".

RealFYang

Two nits remain. Looks good otherwise.

RealFYang · 2023-07-14T13:46:06Z

src/hotspot/cpu/riscv/assembler_riscv.hpp

+    if (shamt != 0) {                                                                        \
+      _slli(Rd, Rs1, shamt);                                                                 \
+    } else {                                                                                 \
+      if(Rd != Rs1) {                                                                        \


Nit: s/if(Rd != Rs1) {/if (Rd != Rs1) {/

RealFYang · 2023-07-14T13:46:16Z

src/hotspot/cpu/riscv/assembler_riscv.hpp

+    if (shamt != 0) {                                                                        \
+      NORMAL_NAME(Rd, Rs1, shamt);                                                           \
+    } else {                                                                                 \
+      if(Rd != Rs1) {                                                                        \


Nit: s/if(Rd != Rs1) {/if (Rd != Rs1) {/

openjdk · 2023-07-14T13:48:59Z

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

8311862: RISC-V: small improvements to shift immediate instructions

Reviewed-by: luhenry, fjiang, fyang

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

a63f865: 8311946: add support for libgraal specific jtreg tests
167d1c1: 8311986: Disable runtime/os/TestTracePageSizes.java for ShenandoahGC
7539cc0: 8303134: JFR: Missing stack trace during chunk rotation stress
d1fa1a8: 8311825: Duplicate qualified enum constants not detected
4676b40: 8312049: runtime/logging/ClassLoadUnloadTest can be improved
bbb7ce5: 8311038: Incorrect exhaustivity computation
2e12a12: 8281658: Add a security category to the java -XshowSettings option
43099a8: 8311647: Memory leak in Java_jdk_internal_org_jline_terminal_impl_jna_linux_CLibraryImpl_ttyname_1r
c7c6d47: 6355567: AdobeMarkerSegment causes failure to read valid JPEG
af7f95e: 8310070: Test: javax/net/ssl/DTLS/DTLSWontNegotiateV10.java timed out
... and 39 more: https://git.openjdk.org/jdk/compare/4b1403d06b99b91ddd89ad6e54669b0595f1f8e5...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 (@luhenry, @RealFYang) 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).

Ilyagavrilin · 2023-07-14T14:30:21Z

/integrate

openjdk · 2023-07-14T14:31:37Z

@Ilyagavrilin
Your change (at version c7b0416) is now ready to be sponsored by a Committer.

VladimirKempik · 2023-07-14T14:31:47Z

/sponsor

openjdk · 2023-07-14T14:32:42Z

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

a63f865: 8311946: add support for libgraal specific jtreg tests
167d1c1: 8311986: Disable runtime/os/TestTracePageSizes.java for ShenandoahGC
7539cc0: 8303134: JFR: Missing stack trace during chunk rotation stress
d1fa1a8: 8311825: Duplicate qualified enum constants not detected
4676b40: 8312049: runtime/logging/ClassLoadUnloadTest can be improved
bbb7ce5: 8311038: Incorrect exhaustivity computation
2e12a12: 8281658: Add a security category to the java -XshowSettings option
43099a8: 8311647: Memory leak in Java_jdk_internal_org_jline_terminal_impl_jna_linux_CLibraryImpl_ttyname_1r
c7c6d47: 6355567: AdobeMarkerSegment causes failure to read valid JPEG
af7f95e: 8310070: Test: javax/net/ssl/DTLS/DTLSWontNegotiateV10.java timed out
... and 39 more: https://git.openjdk.org/jdk/compare/4b1403d06b99b91ddd89ad6e54669b0595f1f8e5...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-14T14:32:49Z

@VladimirKempik @Ilyagavrilin Pushed as commit f3b96f6.

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

Ilyagavrilin · 2023-07-14T14:38:01Z

/backport jdk21u

Ilyagavrilin · 2023-07-14T14:38:18Z

/backport jdk17u-dev

openjdk · 2023-07-14T14:38:26Z

@Ilyagavrilin To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

openjdk · 2023-07-14T14:40:25Z

@Ilyagavrilin To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

initial commit

70bce62

openjdk bot added the rfr Pull request is ready for review label Jul 11, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jul 11, 2023

luhenry approved these changes Jul 11, 2023

View reviewed changes

feilongjiang approved these changes Jul 12, 2023

View reviewed changes

Fix typo in macroAssembler_riscv.cpp

2df1739

luhenry approved these changes Jul 13, 2023

View reviewed changes

RealFYang suggested changes Jul 14, 2023

View reviewed changes

Revert changes related with slli_uw

6a35ea6

Ilyagavrilin changed the title ~~8311862: RISC-V: small improvements to slli~~ 8311862: RISC-V: small improvements to shift immediate instructions Jul 14, 2023

RealFYang approved these changes Jul 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 14, 2023

Fix whitespaces in assembler_riscv.hpp

c7b0416

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 14, 2023

openjdk bot added the integrated Pull request has been integrated label Jul 14, 2023

openjdk bot closed this Jul 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8311862: RISC-V: small improvements to shift immediate instructions #14823

8311862: RISC-V: small improvements to shift immediate instructions #14823

Ilyagavrilin commented Jul 11, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 11, 2023

openjdk bot commented Jul 11, 2023

mlbridge bot commented Jul 11, 2023 •

edited

Loading

luhenry left a comment

feilongjiang left a comment

feilongjiang Jul 12, 2023

Ilyagavrilin Jul 12, 2023

RealFYang Jul 14, 2023 •

edited

Loading

VladimirKempik Jul 14, 2023 •

edited

Loading

RealFYang commented Jul 14, 2023

RealFYang left a comment

RealFYang Jul 14, 2023

RealFYang Jul 14, 2023

openjdk bot commented Jul 14, 2023 •

edited

Loading

Ilyagavrilin commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

VladimirKempik commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

Ilyagavrilin commented Jul 14, 2023

Ilyagavrilin commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

8311862: RISC-V: small improvements to shift immediate instructions #14823

8311862: RISC-V: small improvements to shift immediate instructions #14823

Conversation

Ilyagavrilin commented Jul 11, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 11, 2023

openjdk bot commented Jul 11, 2023

mlbridge bot commented Jul 11, 2023 • edited Loading

Webrevs

luhenry left a comment

Choose a reason for hiding this comment

feilongjiang left a comment

Choose a reason for hiding this comment

feilongjiang Jul 12, 2023

Choose a reason for hiding this comment

Ilyagavrilin Jul 12, 2023

Choose a reason for hiding this comment

RealFYang Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

VladimirKempik Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

RealFYang commented Jul 14, 2023

RealFYang left a comment

Choose a reason for hiding this comment

RealFYang Jul 14, 2023

Choose a reason for hiding this comment

RealFYang Jul 14, 2023

Choose a reason for hiding this comment

openjdk bot commented Jul 14, 2023 • edited Loading

Ilyagavrilin commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

VladimirKempik commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

Ilyagavrilin commented Jul 14, 2023

Ilyagavrilin commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

Ilyagavrilin commented Jul 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 11, 2023 •

edited

Loading

RealFYang Jul 14, 2023 •

edited

Loading

VladimirKempik Jul 14, 2023 •

edited

Loading

openjdk bot commented Jul 14, 2023 •

edited

Loading