8320995: RISC-V: C2 PopCountVI #19065

Hamlin-Li · 2024-05-02T14:55:43Z

Hi,
Can you help to review this patch?
Both auto-vect and vector api depends on this intrinsic.
Thanks!

Performance

Not performance test was done, as this depends on vcpop.v instruction in zvbb extension and the code seqeunce is rather simple than non-intrinsic version.

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-8320995: RISC-V: C2 PopCountVI (Sub-task - P4)
JDK-8320996: RISC-V: C2 PopCountVL (Sub-task - P4)

Reviewers

Ludovic Henry (@luhenry - Committer) ⚠️ Review applies to cbfde208
Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19065

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

Using diff file

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

Webrev

Link to Webrev Comment

Hamlin-Li · 2024-05-02T14:55:58Z

/solves JDK-8320996

bridgekeeper · 2024-05-02T14:56:16Z

👋 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-05-02T14:56:51Z

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

8320995: RISC-V: C2 PopCountVI
8320996: RISC-V: C2 PopCountVL

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

0eff492: 8330278: Have SSLSocketTemplate.doClientSide use loopback address
c6f611c: 8331798: Remove unused arg of checkErgonomics() in TestMaxHeapSizeTools.java
0e1dca7: 8331715: Remove virtual specifiers in ContiguousSpace
7f29904: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
2baacfc: 8331789: ubsan: deoptimization.cpp:403:29: runtime error: load of value 208, which is not a valid value for type 'bool'
7b79426: 8278353: Provide Duke as default favicon in Simple Web Server
466a21d: 8331863: DUIterator_Fast used before it is constructed
8af606f: 8331334: com/sun/net/httpserver/HttpsParametersClientAuthTest.java fails in testServerNeedClientAuth(false)
b910833: 8331864: Update Public Suffix List to 1cbd6e7
f12ed06: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
... and 67 more: https://git.openjdk.org/jdk/compare/4a78906db1ebb56a759b43c2dfa909215491d4c0...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 · 2024-05-02T14:57:10Z

@Hamlin-Li
Adding additional issue to solves list: 8320996: RISC-V: C2 PopCountVL.

openjdk · 2024-05-02T14:57:30Z

@Hamlin-Li 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 · 2024-05-02T15:00:02Z

Webrevs

luhenry · 2024-05-03T18:56:48Z

src/hotspot/cpu/riscv/globals_riscv.hpp

@@ -115,6 +115,7 @@ define_pd_global(intx, InlineSmallCode,          1000);
  product(bool, UseZtso, false, EXPERIMENTAL, "Assume Ztso memory model")        \
  product(bool, UseZihintpause, false, EXPERIMENTAL,                             \
          "Use Zihintpause instructions")                                        \
+  product(bool, UseZvbb, false, "Use Zvbb instructions")                         \


Shouldn't this be marked EXPERIMENTAL as we have no hardware to test it on?

Thanks, you're right.
Fixed.

RealFYang · 2024-05-07T14:19:10Z

src/hotspot/cpu/riscv/riscv_v.ad

+
+instruct vpopcount_mask(vReg dst_src, vRegMask_V0 v0) %{
+  match(Set dst_src (PopCountVI dst_src v0));
+  match(Set dst_src (PopCountVL dst_src v0));


Is there a reason to force input & output being the same vector register?

Seems not, I modify it to usual pattern.

RealFYang · 2024-05-07T14:20:14Z

src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp

@@ -175,6 +175,9 @@ void RiscvHwprobe::add_features_from_query_result() {
  if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBS)) {
    VM_Version::ext_Zbs.enable_feature();
  }
+  if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZVBB)) {
+    VM_Version::ext_Zvbb.enable_feature();
+  }


I don't think it's appropriate to auto-enable an experimental extension.

Thanks for catching, fixed.

RealFYang

One minor comment remains, otherwise looks good. Thanks.

RealFYang · 2024-05-08T00:06:23Z

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

@@ -26,12 +26,14 @@
 * @summary Test vectorization of popcount for Long
 * @requires vm.compiler2.enabled
 * @requires ((os.arch=="x86" | os.arch=="i386" | os.arch=="amd64" | os.arch=="x86_64") & vm.cpu.features ~= ".*avx512bw.*") |
-*           os.simpleArch == "aarch64"
+*           os.simpleArch == "aarch64" |
+*           (os.arch == "riscv64" & vm.cpu.features ~= ".*zvbb,.*")


Suggestion: (os.arch == "riscv64" & vm.cpu.features ~= ".*zvbb.*")
The comma should not be there. See: https://bugs.openjdk.org/browse/JDK-8327689

Thanks, fixed.

Hamlin-Li · 2024-05-08T09:37:11Z

Thanks @luhenry @RealFYang for your reviewing.

/integrate

openjdk · 2024-05-08T09:37:43Z

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

0eff492: 8330278: Have SSLSocketTemplate.doClientSide use loopback address
c6f611c: 8331798: Remove unused arg of checkErgonomics() in TestMaxHeapSizeTools.java
0e1dca7: 8331715: Remove virtual specifiers in ContiguousSpace
7f29904: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
2baacfc: 8331789: ubsan: deoptimization.cpp:403:29: runtime error: load of value 208, which is not a valid value for type 'bool'
7b79426: 8278353: Provide Duke as default favicon in Simple Web Server
466a21d: 8331863: DUIterator_Fast used before it is constructed
8af606f: 8331334: com/sun/net/httpserver/HttpsParametersClientAuthTest.java fails in testServerNeedClientAuth(false)
b910833: 8331864: Update Public Suffix List to 1cbd6e7
f12ed06: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
... and 67 more: https://git.openjdk.org/jdk/compare/4a78906db1ebb56a759b43c2dfa909215491d4c0...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-08T09:37:48Z

@Hamlin-Li Pushed as commit 1aebab7.

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

Hamlin-Li added 9 commits December 28, 2023 18:10

PopCountV: Initial commit

c8d881c

merge master

2eae5f2

minor fix

7e6faa6

merge master; fix issues

b7480c3

Merge branch 'master' into popcount-v

d9186b8

fix issue for mask version

fa5a96a

remove unexpected local files

f16c32e

fix Zvbb flag and misc

9ea7e9d

move vcpop_v

87797f3

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

openjdk bot added the hotspot hotspot-dev@openjdk.org label May 2, 2024

fix minor flag

74692e2

luhenry reviewed May 3, 2024

View reviewed changes

mark UseZvbb experimenal

f316f66

RealFYang reviewed May 7, 2024

View reviewed changes

minor fix

cbfde20

luhenry approved these changes May 7, 2024

View reviewed changes

luhenry mentioned this pull request May 7, 2024

8322753: RISC-V: C2 ReverseBytesV #19120

Closed

3 tasks

RealFYang suggested changes May 8, 2024

View reviewed changes

minor fix

73abb9f

RealFYang approved these changes May 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 8, 2024

openjdk bot added the integrated Pull request has been integrated label May 8, 2024

openjdk bot closed this May 8, 2024

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

Hamlin-Li deleted the popcount-v branch May 8, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8320995: RISC-V: C2 PopCountVI #19065

8320995: RISC-V: C2 PopCountVI #19065

Hamlin-Li commented May 2, 2024 •

edited by openjdk bot

Loading

Hamlin-Li commented May 2, 2024

bridgekeeper bot commented May 2, 2024

openjdk bot commented May 2, 2024 •

edited

Loading

openjdk bot commented May 2, 2024

openjdk bot commented May 2, 2024

mlbridge bot commented May 2, 2024 •

edited

Loading

luhenry May 3, 2024

Hamlin-Li May 7, 2024

RealFYang May 7, 2024

Hamlin-Li May 7, 2024

RealFYang May 7, 2024

Hamlin-Li May 7, 2024

RealFYang left a comment

RealFYang May 8, 2024 •

edited

Loading

Hamlin-Li May 8, 2024

Hamlin-Li commented May 8, 2024

openjdk bot commented May 8, 2024

openjdk bot commented May 8, 2024

8320995: RISC-V: C2 PopCountVI #19065

8320995: RISC-V: C2 PopCountVI #19065

Conversation

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

Performance

Progress

Issues

Reviewers

Reviewing

Webrev

Hamlin-Li commented May 2, 2024

bridgekeeper bot commented May 2, 2024

openjdk bot commented May 2, 2024 • edited Loading

openjdk bot commented May 2, 2024

openjdk bot commented May 2, 2024

mlbridge bot commented May 2, 2024 • edited Loading

Webrevs

luhenry May 3, 2024

Choose a reason for hiding this comment

Hamlin-Li May 7, 2024

Choose a reason for hiding this comment

RealFYang May 7, 2024

Choose a reason for hiding this comment

Hamlin-Li May 7, 2024

Choose a reason for hiding this comment

RealFYang May 7, 2024

Choose a reason for hiding this comment

Hamlin-Li May 7, 2024

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

RealFYang May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Hamlin-Li May 8, 2024

Choose a reason for hiding this comment

Hamlin-Li commented May 8, 2024

openjdk bot commented May 8, 2024

openjdk bot commented May 8, 2024

Hamlin-Li commented May 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 2, 2024 •

edited

Loading

mlbridge bot commented May 2, 2024 •

edited

Loading

RealFYang May 8, 2024 •

edited

Loading