8357554: Enable vectorization of Bool -> CMove with different type size (on riscv) #28231
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 label 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 list. If you would like to change these labels, use the /label pull request command. |
|
@eme64 Could you have a look? :) Not sure who else can help to review it, feel free to help have a look if you're available. :) Thanks! |
Webrevs
|
|
Sounds like this PR should include some IR tests? |
eme64
left a comment
eme64
left a comment
There was a problem hiding this comment.
@Hamlin-Li Thanks for your continued effort on CMove!
Just a first initial comment. And yes, you'll need some IR tests. Would also be nice if we could get some aarch64 or x64 implementation, so we can test it. Maybe we can collaborate on this PR to make it work together :)
Sure, I'm happy to have you involved in this one, are you interested in enabling and implementing aarch64 or x64 part? Should I close this one and use #28230 instead? Please kindly let know! :) |
refactor `is_velt_basic_type_compatible_use_def` Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
|
@Hamlin-Li We can also just go with a risv impl for now, and then we can do x64 and aarch64 separately. Does this patch not affect the IR rules of the tests we have already in the code base? With an improvement, there is usually a chance to add IR rules to existing tests, or add new tests with new IR rules. |
| @@ -204,4 +204,9 @@ | |||
| static bool is_feat_fp16_supported() { | |||
| return (VM_Version::supports_fphp() && VM_Version::supports_asimdhp()); | |||
| } | |||
|
|
|||
| static bool supports_vector_different_use_def_size() { | |||
There was a problem hiding this comment.
This sounds extremely vague. Is this supposed to only be about CMove? Because we already have all sorts of instructions that allow different use and def types, such as conversion vectors. Those are already in use on aarch64 and x64.
| bool VectorNode::is_different_use_def_size_supported() { | ||
| return Matcher::supports_vector_different_use_def_size(); | ||
| } |
There was a problem hiding this comment.
Is this only a forwarding? What's the point of this?
| // Return true if every bit in this vector is 1, e.g. based on the comparison | ||
| // result of 2 floats, set a double result. | ||
| static bool is_different_use_def_size_supported(); |
There was a problem hiding this comment.
I'm a bit confused about your description here. It sounds like this method is looking at a specific vector, and returns results based on that. But that's not what's happening here, is it?
eme64
left a comment
eme64
left a comment
There was a problem hiding this comment.
This seems like an empty refactor, and it's not clear what it solves. It also does not seem riscv specific. It would probably be better if you actually did this together with the patch that actually ensures vectorization for riscv, including IR tests and all. That's probably what you plan to do with #28230, right?
It is difficult to review the code here, without seeing how it all goes together.
|
@Hamlin-Li Can you describe your general approach with #28230? How exactly will you deal with the type size change? Will you have a conversion of the mask, between the comparison and the blend? It may be good if you describe it in a bit of detail, so that we can allow |
|
@Hamlin-Li At a quick glance, #28230 also has some scalar backend implementations of CMove. I think you could just integrate those separately first, and only then do the vectorization. Additionally: it may be easier to first ensure that the Vector API tests work for riscv backend vector instructions. And then we can work on Auto Vectorization once the all the backend instructions are already in place and tested via the Vector API. That would be a way I usually see aarch64 and x64 engineers split up the work. Also makes it easier to get specialists for the area to review the code. What do you think? |
@eme64 Thank you for the suggestion. I'll do some investigation on vector API related things, and get back to this one later. |
3 participants
Hi,
Can you help to review this patch?
This patch enables the vectorization of statement like
op_1 bop op_2 ? res_f_d_1 : res_f_d_2in a loop, where op_x's size is different from res_f_d_x's.To assist with code review, this pr contains only the shared code change, is splitted from #28230, which enable & implement the riscv part. The similar optimization could be extended to other platforms.
Some background
Previously, it's #25336, which was blocked by unsigned comparison issue. The issue was recently resolved by #27942, so I'm re-start working on this optimization.
This pr only relaxes one of the constraints in #25336, i.e. transform CMoveF/D to vector operations no matter what's the size of comparison's operator, but remove the optimization of transform CMoveI/L to vector operations which I think need more investigation.
Test
Jtreg
in progress...
Performance
check the performance data in #25341 on riscv.
Thanks
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28231/head:pull/28231$ git checkout pull/28231Update a local copy of the PR:
$ git checkout pull/28231$ git pull https://git.openjdk.org/jdk.git pull/28231/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28231View PR using the GUI difftool:
$ git pr show -t 28231Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28231.diff
Using Webrev
Link to Webrev Comment