8357551: RISC-V: support CMoveF/D #25341
Conversation
|
👋 Welcome back mli! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@Hamlin-Li The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
|
/solves JDK-8357554 |
|
@Hamlin-Li |
|
Hi @eme64 , do you mind help to have a look at the patch? Thanks! |
|
@Hamlin-Li The table in the PR description is a little hard to read, can you find a way to improve the formatting? |
There was a problem hiding this comment.
@Hamlin-Li That looks like exciting work!
I hope to come back to CMove soon myself, there are a few things to improve there!
I left some initial comments below.
Generally, splitting is nice, especially if the patch is so large.
But we probably also don't want to just add backend instructions that are not yet used. Can the VectorAPI use those CMove instructions you are about to add?
src/hotspot/share/opto/superword.cpp
Outdated
| return type2aelembytes(use_bt) == type2aelembytes(def_bt); | ||
| return (type2aelembytes(use_bt) == type2aelembytes(def_bt)) || | ||
| (support_vectorize_cmovefd_bool_unconditionally() && use->is_CMove() && def->is_Bool()); |
There was a problem hiding this comment.
I think it also works. I'll change it.
| case Op_CMoveI: | ||
| return ((SuperWord::support_vectorize_cmovefd_bool_unconditionally() && bt == T_INT) ? Op_VectorBlend : 0); | ||
| case Op_CMoveL: | ||
| return ((SuperWord::support_vectorize_cmovefd_bool_unconditionally() && bt == T_LONG) ? Op_VectorBlend : 0); |
There was a problem hiding this comment.
Why not always return Op_VectorBlend? And then check elsewhere if that is actually implemented for the expected types?
There was a problem hiding this comment.
Let me check, sounds better if we can do so, as I don't like the current way either. : )
Let me check, it's scrollable in preview mode. Edit: I modified the data a bit, so it looks better now. |
Great!
I can do it.
These instructs can be used by a normal Edit: I might have missed the unsigned version and maybe P/N too. I'll check it later.
For this part, I'm not sure, but I'll have a look later. |
I think the unsigned ones are also used by something like |
|
@Hamlin-Li this pull request can not be integrated into git checkout cmove-fd
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
This patch enable the vectorization of statement like
fd_1 bop fd_2 ? res_1 : res_2in a loop.The current behaviour on other platforms support vecatorization of
fd_1 bop fd_2 ? res_1 : res_2in a loop only whenfdandreshave the same size, but this constraint seems not necessary at least not necessary on riscv, so I relax this constraint on riscv, maybe on other platforms it can be relaxed too, but currently I only made it work on riscv.Besides of this, I also relax the constraint on transforming Op_CMoveI/L to Op_VectorBlend on riscv, this bring some extra benefit when the
resis not float or double types.Both relaxation bring performance benefit via vectorization.
Compared with other runs (master, master with
-XX:+UseVectorCmov -XX:+UseCMoveUnconditionallyturned on, patch without flags turned on), average improvement introduced by the patch with-XX:+UseVectorCmov -XX:+UseCMoveUnconditionallyturned on is more than 2.1 times, in some cases it can bring more than 4 times improvement.When
-XX:-UseVectorCmov -XX:-UseCMoveUnconditionallyturned off, there is no regression on average.Test
Performance
Test: o.o.b.j.l.FPComparison
Column names meanings:
-XX:-UseVectorCmov -XX:-UseCMoveUnconditionallyturned on-XX:-UseVectorCmov -XX:-UseCMoveUnconditionallyturned onAverage improvement
data
Improvement
data
Progress
Warnings
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25341/head:pull/25341$ git checkout pull/25341Update a local copy of the PR:
$ git checkout pull/25341$ git pull https://git.openjdk.org/jdk.git pull/25341/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25341View PR using the GUI difftool:
$ git pr show -t 25341Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25341.diff