8357551: RISC-V: support CMoveF/D vectorization #28309
Conversation
|
@Hamlin-Li : Thanks for the update. I am having another look. |
RealFYang
left a comment
RealFYang
left a comment
There was a problem hiding this comment.
Latest version seems fine to me. Thanks for the update.
As we are very close to JDK 26 rampdown (2025/12/04), I suggest we postpone this to JDK 27.
|
/reviewers 2 I can help review this as well, but currently there is a lot going on with JDK26 bugs. Hopefully things settle down in a few weeks. |
|
@luhenry Thank you! |
|
/integrate |
eme64
left a comment
eme64
left a comment
There was a problem hiding this comment.
Nice work! I did not review the RISC-V specific changes, but had a look at the tests. Wow, there are a lot of them, and that's a good thing :)
I have a few comments below, to consider for improvement.
| // @IR(counts = {IRNode.CMOVE_I, ">0", IRNode.CMP_L, ">0"}, | ||
| // applyIf = {"UseVectorCmov", "false"}, | ||
| // applyIfPlatform = {"riscv64", "true"}) |
There was a problem hiding this comment.
Why are these all commented out?
| private static void init(int[] a) { | ||
| for (int i = 0; i < SIZE; i++) { | ||
| a[i] = RANDOM.nextInt(); | ||
| } | ||
| } | ||
|
|
||
| private static void init(long[] a) { | ||
| for (int i = 0; i < SIZE; i++) { | ||
| a[i] = RANDOM.nextLong(); | ||
| } | ||
| } | ||
|
|
||
| private static void init(float[] a) { | ||
| for (int i = 0; i < SIZE; i++) { | ||
| a[i] = switch(RANDOM.nextInt() % 20) { | ||
| case 0 -> Float.NaN; | ||
| case 1 -> 0; | ||
| case 2 -> 1; | ||
| case 3 -> Float.POSITIVE_INFINITY; | ||
| case 4 -> Float.NEGATIVE_INFINITY; | ||
| case 5 -> Float.MAX_VALUE; | ||
| case 6 -> Float.MIN_VALUE; | ||
| case 7, 8, 9 -> RANDOM.nextFloat(); | ||
| default -> Float.intBitsToFloat(RANDOM.nextInt()); | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| private static void init(double[] a) { | ||
| for (int i = 0; i < SIZE; i++) { | ||
| a[i] = switch(RANDOM.nextInt() % 20) { | ||
| case 0 -> Double.NaN; | ||
| case 1 -> 0; | ||
| case 2 -> 1; | ||
| case 3 -> Double.POSITIVE_INFINITY; | ||
| case 4 -> Double.NEGATIVE_INFINITY; | ||
| case 5 -> Double.MAX_VALUE; | ||
| case 6 -> Double.MIN_VALUE; | ||
| case 7, 8, 9 -> RANDOM.nextDouble(); | ||
| default -> Double.longBitsToDouble(RANDOM.nextLong()); | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
You could use Generators.java, it creates "interesting" distributions, and makes sure we use sufficient special case values.
test/hotspot/jtreg/compiler/c2/irTests/TestScalarConditionalMoveCmpObj.java
Show resolved
Hide resolved
|
Going to push as commit 6700baa.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@Hamlin-Li Pushed as commit 6700baa. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
@Hamlin-Li Oh bummer, I was just a few seconds too slow 😢 |
@eme64 Thanks for having a look. |
|
@Hamlin-Li Sounds good. I don't blame you, you had 2 reviewers ;) |
4 participants
Hi,
This pr add CMoveF/D on riscv, which enable vectorization of statement like:
op_1 bop op_2 ? res_f_d_1 : res_f_d_2 in a loop.This pr is also a preparation for further vectorization in #28231.
Previously it's #25341, but at that time, C2 SLP has some issue with unsigned comparison, which is now fixed, so it's good to continue the work.
Test
Jtreg
in progress...
Performance
Column names meanings:
-XX:+UseVectorCmov -XX:+UseCMoveUnconditionallyturned on-XX:+UseVectorCmov -XX:+UseCMoveUnconditionallyturned onAverage improvement
NOTE: With only this PR, it brings performance benefit in case of
CMoveF+CmpF,CMoveD+ComD,CMoveF+CmpI,CMoveD+CmpL. The data below is based on fullly implmenting the vectorization ofCMoveI/L/F/D+CmpI/L/F/D, which will be achieved by #28231.For details, check the performance data in #25341 on riscv.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28309/head:pull/28309$ git checkout pull/28309Update a local copy of the PR:
$ git checkout pull/28309$ git pull https://git.openjdk.org/jdk.git pull/28309/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28309View PR using the GUI difftool:
$ git pr show -t 28309Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28309.diff
Using Webrev
Link to Webrev Comment