8371603: C2: Missing Ideal optimizations for load and store vectors on SVE#28651
8371603: C2: Missing Ideal optimizations for load and store vectors on SVE#28651XiaohongGong wants to merge 3 commits intoopenjdk:masterfrom
Conversation
…ias_idx) == load->in(1)) failed
|
👋 Welcome back xgong! A progress list of the required criteria for merging this PR into |
|
@XiaohongGong 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: 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 106 new commits pushed to the
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 |
|
/contributor add @eme64 |
|
@XiaohongGong |
|
@XiaohongGong 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. |
|
Hi @eme64 , this is the fixing for the crash issue reported on aws machine. Could you please help take a look? Thanks a lot! |
Webrevs
|
eme64
left a comment
There was a problem hiding this comment.
@XiaohongGong Thanks for taking this over from me and fixing this so quickly, much appreciated!
Thanks for adding my regression tests and for the attribution :)
I only have a minor comment below. Otherwise, the code looks good to me. But since I'm not an SVE specialist, it would be good if someone with deeper knowledge would do a deeper review of the specific SVE parts.
Once an SVE specialist gives the approval for the PR, I'l run some internal testing and approve from my side :)
Ah, and one more thing: you should change the PR title to be more descriptive of the issue. The assert that was hit is only a far removed symptom. I would suggest:
C2 SVE: missing Ideal optimizations for load and store vectors
The new title looks good to me. Thanks! |
|
Hi @eme64, I'v updated the patch based on your suggestion. Would you mind taking another look? Thanks! |
|
Hi @theRealAph , would you mind taking a look at this patch especially the AArch64 part? Thanks a lot! |
erifan
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix.
shqking
left a comment
There was a problem hiding this comment.
Thanks for your work. LGTM.
eme64
left a comment
There was a problem hiding this comment.
Nice, thanks for the updates. I think this looks cleaner now.
I have one more nit below. But I'll run some sanity testing from my side already.
test/hotspot/jtreg/compiler/vectorapi/TestVectorLoadStoreOptimization.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/vectorapi/TestVectorOperationsWithPartialSize.java
Outdated
Show resolved
Hide resolved
eme64
left a comment
There was a problem hiding this comment.
Tests pass. Thanks for the updates.
And thanks for fixing it so swiftly and for the attribution 😊
|
/integrate |
|
Going to push as commit b6732d6.
Your commit was automatically rebased without conflicts. |
|
@XiaohongGong Pushed as commit b6732d6. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
/backport jdk26 |
|
@XiaohongGong The target repository There is a branch |
|
/backport :jdk26 |
|
@XiaohongGong the backport was successfully created on the branch backport-XiaohongGong-b6732d60-jdk26 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk26, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk: |
Problem:
This issue occurs on a 256-bit SVE machine, caused by the following problematic pattern in
LoadVectorNode::Ideal():The condition
Matcher::vector_needs_partial_operations(this, vt)returns true forLoadVectorNodewith 256-bit vector size even when the vector size equals the maximum vector size on SVE. In such cases, whenVectorNode::try_to_gen_masked_vector()returnsnullptr, the method exits early without callingLoadNode::Ideal(). This results in missing crucial optimizations that would normally be applied by the superclass.This code was introduced by https://bugs.openjdk.org/browse/JDK-8286941 to generate vector masks for partial vector operations, but it failed to ensure that the superclass
Ideal()method is always invoked when no transformation is applied.Solution:
This patch addresses the issue through two changes:
Matcher::vector_needs_partial_operations()to return true only when the vector node genuinely represents a partial vector operation that requires masking.VectorNode::try_to_gen_masked_vector()to never returnnullptr, ensuring the superclassIdeal()method is always invoked when no transformation is applied.Testing:
LoadVector/StoreVectorare now applied, and 2) that mask and the correct IR patterns are generated for partial vector operations.Progress
Issue
Reviewers
Contributors
<epeter@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28651/head:pull/28651$ git checkout pull/28651Update a local copy of the PR:
$ git checkout pull/28651$ git pull https://git.openjdk.org/jdk.git pull/28651/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28651View PR using the GUI difftool:
$ git pr show -t 28651Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28651.diff
Using Webrev
Link to Webrev Comment