8365570: C2 fails assert(false) failed: Unexpected node in SuperWord truncation: CastII #26827
Conversation
|
👋 Welcome back jkarthikeyan! A progress list of the required criteria for merging this PR into |
|
@jaskarth 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 90 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 |
openjdk
bot
added
hotspot-compiler
Webrevs
|
src/hotspot/share/opto/superword.cpp
Outdated
| // Vector nodes should not truncate. | ||
| if (type->isa_vect() != nullptr || type->isa_vectmask() != nullptr || in->is_Reduction()) { | ||
| // Vector nodes and casts should not truncate. | ||
| if (type->isa_vect() != nullptr || type->isa_vectmask() != nullptr || in->is_Reduction() || in->is_ConstraintCast()) { |
There was a problem hiding this comment.
Why should we not truncate a CastII? What can go wrong?
There was a problem hiding this comment.
My thinking was that since casts specifically change the type of a node, they may not be safe to truncate. In practice it might not matter because after the CastII pack is created, it's discarded because there is no backend implementation for vectorized CastII. I've opted to mark them as non-truncating to stay on the safer side.
There was a problem hiding this comment.
I see. Ok. Can you add a comment to the code for that?
Because imagine we come along later and actually implement a backend vectorized version of CastII (no-op?). Maybe because we implement if-conversion. Then it would be nice to know if this was just a "to be on the safe side" check, or if it would run into issues when removed.
There was a problem hiding this comment.
Because the current comment says "should not truncate". That sounds more strong than "to be on the safe side".
There was a problem hiding this comment.
I think this is fair, I've pushed a commit that changes the comment wording. Let me know what you think!
There was a problem hiding this comment.
We have CastVV which should be the packed version of CastII. The thing that is I think the most difficult is to properly wire it. In the simplest situation of all elements in the pack having the same control input can be easily handled, though.
There was a problem hiding this comment.
Right. I don't think there is currently any case where CastII would actually be useful to pack in superword. They usually originate from some control-flow. For example a comparison on a variable if (x < 5) and then we can CastII the variable x on the branches. But that only becomes relevant with if-conversion.
Or do you see a case that could happen without if-conversion where CastII needs to be handled?
|
@jaskarth Thanks for the fix, it looks good to me now :) |
|
Thanks a lot for the testing @eme64! I think I need another review to merge it. |
Hi all,
This is a quick patch for the assert failure in superword truncation with CastII. I've added a check for all constraint cast nodes, and attached a reduced version of the fuzzer test. Thanks!
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26827/head:pull/26827$ git checkout pull/26827Update a local copy of the PR:
$ git checkout pull/26827$ git pull https://git.openjdk.org/jdk.git pull/26827/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26827View PR using the GUI difftool:
$ git pr show -t 26827Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26827.diff
Using Webrev
Link to Webrev Comment