8320646: RISC-V: C2 VectorCastHF2F #17698

Hamlin-Li · 2024-02-04T09:38:48Z

Hi,
Can you review this patch to add instrinsics for VectorCastHF2F/VectorCastF2HF?
Thanks!

Test

test/jdk/java/lang/Float/Binary16ConversionNaN.java
test/jdk/java/lang/Float/Binary16Conversion.java

hotspot/jtreg/compiler/intrinsics/float16
hotspot/jtreg/compiler/vectorization/TestFloatConversionsVectorjava
hotspot/jtreg/compiler/vectorization/TestFloatConversionsVectorNaN.java

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-8320646: RISC-V: C2 VectorCastHF2F (Sub-task - P4)
JDK-8320647: RISC-V: C2 VectorCastF2HF (Sub-task - P4)

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17698

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-04T09:39:19Z

👋 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-04T09:39:43Z

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

Hamlin-Li · 2024-02-04T09:42:02Z

/solves JDK-8320647

openjdk · 2024-02-04T09:42:32Z

@Hamlin-Li
Adding additional issue to solves list: 8320647: RISC-V: C2 VectorCastF2HF.

mlbridge · 2024-02-04T09:57:15Z

Webrevs

src/hotspot/cpu/riscv/assembler_riscv.hpp

Hamlin-Li · 2024-02-07T15:21:54Z

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

Hamlin-Li · 2024-02-21T16:31:52Z

Hi, Can I get more reviews? Thanks

RealFYang · 2024-02-26T07:30:12Z

Seems that the spec is not in ratified status yet?

Hamlin-Li · 2024-02-26T10:22:27Z

Seems that the spec is not in ratified status yet?

Seems it is, please check https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions, or I misunderstand what you means?

luhenry

Small changes.

src/hotspot/cpu/riscv/riscv_v.ad

RealFYang

One comment after a brief look. Thanks.

RealFYang · 2024-02-27T09:50:33Z

src/hotspot/cpu/riscv/globals_riscv.hpp

@@ -114,6 +114,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, UseZvfh, false, "Use Zvfh instructions")                         \


I don't think it's a good idea to have more and more experimental / disagnostic options for new RV extensions. I think it might be better to make use of the hwprobe syscall and take a vendor-specific approach. One example is #17122 which adds instructions for RV Zcb extension.

We still want to give end users the ability to disable extensions if things are broken. The flags would be the only way if we rely on hwprobe + vendor specific flags (like https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_riscv/vm_version_linux_riscv.cpp#L232-L260, and which I'm surprised not more hw vendors have added their own)

We still want to give end users the ability to disable extensions if things are broken. The flags would be the only way if we rely on hwprobe + vendor specific flags (like

That make sense to me. In the long run, we should depend on hwprobe + vendor specific code to auto-enable or disable certain RV extensions on certain hardwares. But I agree on that there should be some options of DIAGNOSTIC kind to help diagnose issues and find workarounds. I am OK to make this option an EXPERIMENTAL one at the current stage. But we should turn them into the DIAGNOSTIC kind in the future when we have the hardwares and hwprobe is capable to satisfy our needs. Mind you, we are lacking one option for #17122 in that respect. Let me know if you going to add one.

https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_riscv/vm_version_linux_riscv.cpp#L232-L260, and which I'm surprised not more hw vendors have added their own)

That will depend on the vendors. Given that the current RV hardwares are not that rich in RV extensions, I guess there isn't much to tune.

Thanks for comments and discussion, made UseZvfh experimental.

That make sense to me. In the long run, we should depend on hwprobe + vendor specific code to auto-enable or disable certain RV extensions on certain hardwares. But I agree on that there should be some options of DIAGNOSTIC kind to help diagnose issues and find workarounds. I am OK to make this option an EXPERIMENTAL one at the current stage. But we should turn them into the DIAGNOSTIC kind in the future when we have the hardwares and hwprobe is capable to satisfy our needs. Mind you, we are lacking one option for #17122 in that respect. Let me know if you going to add one.

Agreed on all of that! The lack of currently and widely available hardware with more of these extensions is what makes me want to keep them EXPERIMENTAL. But once we validated that these work on commercially available hardware, I would indeed move them out of EXPERIMENTAL, and I agree DIAGNOSTIC makes for a fine change.

Another question will be about backporting the EXPERIMENTAL -> DIAGNOSTIC. We can revisit later but I think we should be fairly proactive about that when the case arises. Do you see any issues that may come from it? Possibly we expect that's a breaking change as users would need to switch from -XX:+UseExperimentalFeatures to -XX:+UseDiagnosticsFeatures.

Mind you, we are lacking one option for #17122 in that respect. Let me know if you going to add one.

@RealFYang I can do that. FYI, tested on lichipi and VF2, seems both does not support Zcb yet, tracked by https://bugs.openjdk.org/browse/JDK-8327058

Another question will be about backporting the EXPERIMENTAL -> DIAGNOSTIC. We can revisit later but I think we should be fairly proactive about that when the case arises. Do you see any issues that may come from it? Possibly we expect that's a breaking change as users would need to switch from -XX:+UseExperimentalFeatures to -XX:+UseDiagnosticsFeatures.

I didn't see a big issue as they are EXPERIMENTAL options for now which are subject to change. User are not supposed to use them casually as they use normal product VM options. I am thinking that the backporting to LTS versions should happen only after they are turned into DIAGNOSTIC in JDK mainline some day.

src/hotspot/cpu/riscv/globals_riscv.hpp

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

RealFYang

And I have several more comments. Could you please move the newly-added assember functions immediately after the scalar version, that is C2_MacroAssembler::float16_to_float and C2_MacroAssembler::float_to_float16? Then it will be more convenient for people to compare the two versions.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

…d register asserts

RealFYang

Thanks for the quick update. Several minor comments remain after another look. Looks good otherwise.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp

RealFYang · 2024-03-06T06:12:31Z

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

+  assert_different_registers(dst, src, tmp);
+
+  auto stub = C2CodeStub::make<VectorRegister, VectorRegister, VectorRegister, uint>
+              (dst, src, tmp, length, 36, float_to_float16_v_slow_path);


You might want to reduce the stub size from 36 to 28 after the stub changes you made.

Thanks for detailed review!
All comments resolved.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

RealFYang

Updated change looks good assuming it still passes the related tests.

RealFYang · 2024-03-07T02:54:43Z

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

- * @requires (os.simpleArch == "x64" & (vm.cpu.features ~= ".*avx512f.*" | vm.cpu.features ~= ".*f16c.*")) | os.arch == "aarch64"
+ * @requires (os.simpleArch == "x64" & (vm.cpu.features ~= ".*avx512f.*" | vm.cpu.features ~= ".*f16c.*")) |
+ *           os.arch == "aarch64" |
+ *           (os.arch == "riscv64" & vm.cpu.features ~= ".*zvfh.*")


A small question: I see you used following condition when doing the scalar version:
(os.arch == "riscv64" & vm.cpu.features ~= ".*zfh,.*")
There is comma there for matching cpu features. Do we really need this comma? It is not there for this vector version.

Seems not necessary currently, unless in the future there is other extension starting with zfh, e.g. zfhX, but seems that chance is low IMHO.
And, I think it might not work for the last extension in the CPU feature string if it's with a comma (as the cpu feature string is converted from a list to string, so the last one will not ending with a, ? ), so for new and long named extensions, it should be without a comma.

Thanks for testing!

Seems not necessary currently, unless in the future there is other extension starting with zfh, e.g. zfhX, but seems that chance is low IMHO. And, I think it might not work for the last extension in the CPU feature string if it's with a comma (as the cpu feature string is converted from a list to string, so the last one will not ending with a, ? ), so for new and long named extensions, it should be without a comma.

Yeah, that makes sense to me. I think we should remove this comma from conditions (os.arch == "riscv64" & vm.cpu.features ~= ".*zfh,.*") on jdk master. Can you fix that? Then we will have a more simpler rule when writing those conditions. That is, for single-character extensions like c or v, we need to add this comma. But for multi-character extensions like zfh or zvfh, we should not have it to avoid the issue you mentioned.

Sure, I created https://bugs.openjdk.org/browse/JDK-8327689 to track it.

openjdk · 2024-03-07T03:00:04Z

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

8320646: RISC-V: C2 VectorCastHF2F
8320647: RISC-V: C2 VectorCastF2HF

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

4018341: 8327379: Make TimeLinearScan a develop flag
1bd4abf: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out
ddcd6de: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
3d37b28: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
989fc3e: 8325878: Require minimum Clang version 13
13c7453: 8325880: Require minimum Open XL C/C++ version 17.1.1
4f33608: 8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677
e92ecd9: 8326983: Unused operands reported after JDK-8326135
9f70940: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases
08b03a3: 8327376: Parallel: Remove unimplemented methods in psParallelCompact.hpp
... and 398 more: https://git.openjdk.org/jdk/compare/a18b03b86fdd0eef773badbced46607a8e5a068a...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.

Hamlin-Li · 2024-03-07T12:20:49Z

Thanks @RealFYang @luhenry for your reviewing.

/integrate

openjdk · 2024-03-07T12:21:26Z

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

53c4714: 8327501: Common ForkJoinPool prevents class unloading in some cases
1261740: 8327283: RISC-V: Minimal build failed after JDK-8319716
f54e598: 8327172: C2 SuperWord: data node in loop has no input in loop: replace assert with bailout
4018341: 8327379: Make TimeLinearScan a develop flag
1bd4abf: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out
ddcd6de: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
3d37b28: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
989fc3e: 8325878: Require minimum Clang version 13
13c7453: 8325880: Require minimum Open XL C/C++ version 17.1.1
4f33608: 8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677
... and 401 more: https://git.openjdk.org/jdk/compare/a18b03b86fdd0eef773badbced46607a8e5a068a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-07T12:21:33Z

@Hamlin-Li Pushed as commit d7273ac.

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

Hamlin-Li and others added 10 commits January 2, 2024 10:00

VectorCastHF2F: Initial Commit

bca3825

merge master

58b9d99

fix stub size

3ff0668

merge master

7141c3a

fix widening vector register to even one

21a7ddd

fix issues

5ffb9d0

Initial commit: VectorCastF2HF

3c4a8fc

Refine tests

2037477

Merge branch 'master' into float16-to-float-v

7f02356

clean code

dd9605b

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 4, 2024

fix jcheck error

521c222

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

luhenry suggested changes Feb 7, 2024

View reviewed changes

src/hotspot/cpu/riscv/assembler_riscv.hpp Outdated Show resolved Hide resolved

add some comments

5964bb6

luhenry approved these changes Feb 27, 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

RealFYang reviewed Feb 27, 2024

View reviewed changes

luhenry suggested changes Feb 27, 2024

View reviewed changes

src/hotspot/cpu/riscv/globals_riscv.hpp Outdated Show resolved Hide resolved

make UseZvfh experimatal; clean code

0dce157

luhenry approved these changes Feb 29, 2024

View reviewed changes

Hamlin-Li mentioned this pull request Feb 29, 2024

8327058: RISC-V: make Zcb experimental #18070

Closed

3 tasks

RealFYang reviewed Mar 1, 2024

View reviewed changes

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

RealFYang reviewed Mar 4, 2024

View reviewed changes

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

hamlin added 5 commits March 4, 2024 10:26

move code

80961f2

refine code according to review comments

d56f9e2

refine code

08dce19

correct uimm assert in vnsra_wi; use vnsra_wi instead of vnsra_wx; ad…

370892e

…d register asserts

filter tests with zvfh

bf5c123

RealFYang suggested changes Mar 6, 2024

View reviewed changes

minor improvements

def78b1

RealFYang approved these changes Mar 7, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 7, 2024

openjdk bot added the integrated Pull request has been integrated label Mar 7, 2024

openjdk bot closed this Mar 7, 2024

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

Hamlin-Li deleted the float16-to-float-v branch March 7, 2024 12:40

Hamlin-Li mentioned this pull request Mar 8, 2024

8327689: RISC-V: adjust test filters of zfh extension #18169

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8320646: RISC-V: C2 VectorCastHF2F #17698

8320646: RISC-V: C2 VectorCastHF2F #17698

Hamlin-Li commented Feb 4, 2024 •

edited by openjdk bot

bridgekeeper bot commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

Hamlin-Li commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

mlbridge bot commented Feb 4, 2024 •

edited

Hamlin-Li commented Feb 7, 2024

Hamlin-Li commented Feb 21, 2024

RealFYang commented Feb 26, 2024

Hamlin-Li commented Feb 26, 2024

luhenry left a comment

RealFYang left a comment

RealFYang Feb 27, 2024

luhenry Feb 27, 2024

RealFYang Feb 28, 2024 •

edited

Hamlin-Li Feb 28, 2024

luhenry Feb 29, 2024 •

edited

Hamlin-Li Feb 29, 2024 •

edited

RealFYang Mar 1, 2024

RealFYang left a comment •

edited

RealFYang left a comment

RealFYang Mar 6, 2024 •

edited

Hamlin-Li Mar 6, 2024

RealFYang left a comment

RealFYang Mar 7, 2024

Hamlin-Li Mar 7, 2024

Hamlin-Li Mar 7, 2024

RealFYang Mar 8, 2024

Hamlin-Li Mar 8, 2024

openjdk bot commented Mar 7, 2024

Hamlin-Li commented Mar 7, 2024

openjdk bot commented Mar 7, 2024

openjdk bot commented Mar 7, 2024

8320646: RISC-V: C2 VectorCastHF2F #17698

8320646: RISC-V: C2 VectorCastHF2F #17698

Conversation

Hamlin-Li commented Feb 4, 2024 • edited by openjdk bot

Test

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

Hamlin-Li commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

mlbridge bot commented Feb 4, 2024 • edited

Webrevs

Hamlin-Li commented Feb 7, 2024

Hamlin-Li commented Feb 21, 2024

RealFYang commented Feb 26, 2024

Hamlin-Li commented Feb 26, 2024

luhenry left a comment

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealFYang Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luhenry Feb 29, 2024 • edited

Choose a reason for hiding this comment

Hamlin-Li Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealFYang left a comment • edited

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

RealFYang Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Mar 7, 2024

Hamlin-Li commented Mar 7, 2024

openjdk bot commented Mar 7, 2024

openjdk bot commented Mar 7, 2024

Hamlin-Li commented Feb 4, 2024 •

edited by openjdk bot

mlbridge bot commented Feb 4, 2024 •

edited

RealFYang Feb 28, 2024 •

edited

luhenry Feb 29, 2024 •

edited

Hamlin-Li Feb 29, 2024 •

edited

RealFYang left a comment •

edited

RealFYang Mar 6, 2024 •

edited