8295948: Support for Zicbop/prefetch instructions on RISC-V #10884

luhenry · 2022-10-27T15:18:02Z

The OpenJDK supports generating prefetch instructions on most platforms. RISC-V supports through the Zicbop extension the use of prefetch instructions. We want to make sure we use these instructions whenever they are available.

It passes hotspot:tier1 test suite

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-8295948: Support for Zicbop/prefetch instructions on RISC-V

Reviewers

Fei Yang (@RealFYang - Reviewer)
Yadong Wang (@yadongw - Author) ⚠️ Review applies to 0e92909e

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10884

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

Using diff file

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

The OpenJDK supports generating prefetch instructions on most platforms. RISC-V supports through the Zicbop extension the use of prefetch instructions. We want to make sure we use these instructions whenever they are available.

bridgekeeper · 2022-10-27T15:20:24Z

👋 Welcome back luhenry! 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 · 2022-10-27T15:26:27Z

@luhenry 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 · 2022-10-27T15:31:04Z

Webrevs

luhenry · 2022-10-31T11:29:36Z

@RealFYang let me know what you think. Thanks!

RealFYang · 2022-11-01T06:56:13Z

@luhenry : Sorry for late reply. I am looking this this now.

luhenry · 2022-11-01T08:37:51Z

@RealFYang perfect, thank you

src/hotspot/cpu/riscv/riscv.ad

RealFYang · 2022-11-02T09:00:24Z

src/hotspot/os_cpu/linux_riscv/prefetch_linux_riscv.inline.hpp

+    void (*stub)(const void*, intptr_t) =
+        (void (*)(const void*, intptr_t))StubRoutines::riscv::prefetch_r();
+    if (interval >= 0 && stub != NULL) {
+        stub(loc, interval);


I am not sure if it really worth it to call a stub for read / write here. It looks to me not a big issue for the case the stub tries to catch and resolve. And I see aarch64 simply plant a 'prfm' instruction for prefetching [1]. I guess we might can do the same?

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_aarch64/prefetch_linux_aarch64.inline.hpp#L34

We would need to check for UseZicbop in any case; the access to a global variable is then required.

It would be the same issue as https://github.com/openjdk/jdk/pull/10884/files/e968f7164124dcf560807c9ff7765e6f82b64cdd#diff-e3c18b8b83898e82b5a3069319df6a47468e91cc2527bf065e704a685a20f26bR5196 without the stub.

I've to admit that the interval naming here is confusing since no implementation ever uses it as an interval but alway as an offset. Also, the callers assume it to be an offset, like ContiguousSpace::prepare_for_compaction for example.

Yes, I agree that a check for UseZicbop option would be necessary. But I still don't understand why we should implement this through a stub here. It looks to me that CPP code with inline assembly would also do. At least this could help eliminate the prologue & epilogue cost of calling the stub.

We can definitely go with inline assembly here, just not sure that prefetch.r or prefetch.w would be recognized by the assembler. However, given that prefetch.* is an ori under the hood, we can simply use that.

RealFYang

Updated changes looks good except for the indentation issue.

src/hotspot/os_cpu/linux_riscv/prefetch_linux_riscv.inline.hpp

openjdk · 2022-11-04T03:27:50Z

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

8295948: Support for Zicbop/prefetch instructions on RISC-V

Reviewed-by: fyang, yadongwang

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

f2acdfd: 8296638: RISC-V: NegVI node emits wrong code when vector element basic type is T_BYTE/T_SHORT
bfc5816: 8295475: Move non-resource allocation strategies out of ResourceObj
e802b12: 8296196: Class.getEnumConstants() throws undocumented ClassCastException and NullPointerException
78a08a0: 8295430: Use cmsDoTransformLineStride instead of cmsDoTransform in the loop
f0a6e71: 8295812: Skip the "half float" support in LittleCMS during the build
79c0092: 8285635: javax/swing/JRootPane/DefaultButtonTest.java failed with Default Button not pressed for L&F: com.sun.java.swing.plaf.motif.MotifLookAndFeel
0981bfb: 8296156: [macos] Resize DMG windows and background to fit additional DMG contents
93fed9b: 8296448: RISC-V: Fix temp usages of heapbase register killed by MacroAssembler::en/decode_klass_not_null
d6e2d0d: 8296611: Problemlist several sun/security tests until JDK-8295343 is resolved
102b2b3: 8292033: Move jdk.X509Certificate event logic to JCA layer
... and 134 more: https://git.openjdk.org/jdk/compare/628820f47ef9c9ad3cc62e68db9c4dbc7e659154...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 (@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).

src/hotspot/cpu/riscv/riscv.ad

RealFYang

Still looks good. Thanks.

luhenry · 2022-11-04T12:46:16Z

/integrate

openjdk · 2022-11-04T12:47:13Z

@luhenry
Your change (at version 51725fd) is now ready to be sponsored by a Committer.

src/hotspot/cpu/riscv/riscv.ad

luhenry · 2022-11-08T07:34:52Z

/integrate

openjdk · 2022-11-08T07:36:31Z

@luhenry
Your change (at version 42a61aa) is now ready to be sponsored by a Committer.

luhenry · 2022-11-08T16:16:17Z

@RealFYang @yadongw could you please review+sponsor if it looks all good to you? Thanks!

src/hotspot/cpu/riscv/riscv.ad

yadongw

lgtm

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

RealFYang · 2022-11-10T13:02:48Z

/sponsor

openjdk · 2022-11-10T13:04:24Z

@RealFYang The PR has been updated since the change author (@luhenry) issued the integrate command - the author must perform this command again.

luhenry · 2022-11-10T13:30:20Z

/integrate

openjdk · 2022-11-10T13:32:11Z

@luhenry
Your change (at version 3f70a21) is now ready to be sponsored by a Committer.

RealFYang · 2022-11-10T13:35:22Z

/sponsor

openjdk · 2022-11-10T13:37:49Z

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

f2acdfd: 8296638: RISC-V: NegVI node emits wrong code when vector element basic type is T_BYTE/T_SHORT
bfc5816: 8295475: Move non-resource allocation strategies out of ResourceObj
e802b12: 8296196: Class.getEnumConstants() throws undocumented ClassCastException and NullPointerException
78a08a0: 8295430: Use cmsDoTransformLineStride instead of cmsDoTransform in the loop
f0a6e71: 8295812: Skip the "half float" support in LittleCMS during the build
79c0092: 8285635: javax/swing/JRootPane/DefaultButtonTest.java failed with Default Button not pressed for L&F: com.sun.java.swing.plaf.motif.MotifLookAndFeel
0981bfb: 8296156: [macos] Resize DMG windows and background to fit additional DMG contents
93fed9b: 8296448: RISC-V: Fix temp usages of heapbase register killed by MacroAssembler::en/decode_klass_not_null
d6e2d0d: 8296611: Problemlist several sun/security tests until JDK-8295343 is resolved
102b2b3: 8292033: Move jdk.X509Certificate event logic to JCA layer
... and 134 more: https://git.openjdk.org/jdk/compare/628820f47ef9c9ad3cc62e68db9c4dbc7e659154...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-10T13:38:17Z

@RealFYang @luhenry Pushed as commit 4465361.

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

openjdk bot added the rfr Pull request is ready for review label Oct 27, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 27, 2022

RealFYang reviewed Nov 2, 2022

View reviewed changes

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

Remove uncessary cache line alignement

e968f71

RealFYang reviewed Nov 2, 2022

View reviewed changes

luhenry added 3 commits November 3, 2022 16:20

review

0c8691b

remove dead code

c9d6f3f

fixup! remove dead code

48ab8f3

RealFYang approved these changes Nov 4, 2022

View reviewed changes

src/hotspot/os_cpu/linux_riscv/prefetch_linux_riscv.inline.hpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Nov 4, 2022

RealFYang reviewed Nov 4, 2022

View reviewed changes

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

luhenry added 2 commits November 4, 2022 09:58

review

6836296

fixup! review

51725fd

RealFYang approved these changes Nov 4, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 4, 2022

yadongw reviewed Nov 7, 2022

View reviewed changes

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

support large disp values

42a61aa

openjdk bot removed the sponsor Pull request is ready to be sponsored label Nov 8, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 8, 2022

yadongw reviewed Nov 9, 2022

View reviewed changes

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

review

0e92909

openjdk bot removed the sponsor Pull request is ready to be sponsored label Nov 9, 2022

yadongw approved these changes Nov 10, 2022

View reviewed changes

RealFYang reviewed Nov 10, 2022

View reviewed changes

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp Outdated Show resolved Hide resolved

review

3f70a21

RealFYang approved these changes Nov 10, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 10, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 10, 2022

openjdk bot closed this Nov 10, 2022

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 Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295948: Support for Zicbop/prefetch instructions on RISC-V #10884

8295948: Support for Zicbop/prefetch instructions on RISC-V #10884

luhenry commented Oct 27, 2022 •

edited by openjdk bot

bridgekeeper bot commented Oct 27, 2022

openjdk bot commented Oct 27, 2022

mlbridge bot commented Oct 27, 2022 •

edited

luhenry commented Oct 31, 2022

RealFYang commented Nov 1, 2022

luhenry commented Nov 1, 2022

RealFYang Nov 2, 2022 •

edited

luhenry Nov 2, 2022 •

edited

RealFYang Nov 3, 2022

luhenry Nov 3, 2022

RealFYang left a comment

openjdk bot commented Nov 4, 2022 •

edited

RealFYang left a comment

luhenry commented Nov 4, 2022

openjdk bot commented Nov 4, 2022

luhenry commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

luhenry commented Nov 8, 2022

yadongw left a comment

RealFYang commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

luhenry commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

RealFYang commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

8295948: Support for Zicbop/prefetch instructions on RISC-V #10884

8295948: Support for Zicbop/prefetch instructions on RISC-V #10884

Conversation

luhenry commented Oct 27, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 27, 2022

openjdk bot commented Oct 27, 2022

mlbridge bot commented Oct 27, 2022 • edited

Webrevs

luhenry commented Oct 31, 2022

RealFYang commented Nov 1, 2022

luhenry commented Nov 1, 2022

RealFYang Nov 2, 2022 • edited

Choose a reason for hiding this comment

luhenry Nov 2, 2022 • edited

Choose a reason for hiding this comment

RealFYang Nov 3, 2022

Choose a reason for hiding this comment

luhenry Nov 3, 2022

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 4, 2022 • edited

RealFYang left a comment

Choose a reason for hiding this comment

luhenry commented Nov 4, 2022

openjdk bot commented Nov 4, 2022

luhenry commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

luhenry commented Nov 8, 2022

yadongw left a comment

Choose a reason for hiding this comment

RealFYang commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

luhenry commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

RealFYang commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

luhenry commented Oct 27, 2022 •

edited by openjdk bot

mlbridge bot commented Oct 27, 2022 •

edited

RealFYang Nov 2, 2022 •

edited

luhenry Nov 2, 2022 •

edited

openjdk bot commented Nov 4, 2022 •

edited