8288112: C2: Error: ShouldNotReachHere() in Type::typerr() #128
Conversation
/label add hotspot-compiler-dev |
👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into |
@jatin-bhateja |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I still have a problem with the error message. It seems like we could give a better error message if we detected the missing vectorization support earlier. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you for fixing it. I will run testing.
Failed:
|
Hi @dean-long , Thanks for your comments, error occurs because SLP analysis passes all the checks to enable creation of vector ReverseByte IR, but SLP backend had this case missing due to which some of the IR nodes were vectorized and were feeding into a scalar ReverseByte node, thus while doing a value computation compiler encounters a meet operation b/w vector and scalar lattice. Since its an error related to internals of JIT compiler it will not be adding any value to user and just represent incorrect control path selected by compiler. With this patch we do not hit the trap any more. |
@jatin-bhateja Right, a better error message wouldn't be a value-add to users, but if we could detect it sooner, like in SuperWord::output(), that might be useful to compiler engineers debugging issues. |
Hi @dean-long , Agree, I have changed the error message generated with -XX:+TraceLoopOpts to be more explicit like. Since patch already handles the missing scalar case, hence this message will not be generated. |
@jatin-bhateja OK, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I submitted testing.
Tests copyright header validation failed because you forgot |
Thanks, fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
@jatin-bhateja 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 19 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 |
Hi @dean-long , kindly approve if the patch version looks ok to you. |
Thanks @vnkozlov @dean-long. |
/integrate |
Going to push as commit fd89ab8.
Your commit was automatically rebased without conflicts. |
@jatin-bhateja Pushed as commit fd89ab8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
JDK-8284960 added new vector IR nodes and target specific backend support for ReverseByte vector operations.
For each scalar operation, auto-vectorizer analysis stage checks for existence of vector IR opcode and target specific backend implementation. While processing scalar IR nodes corresponding to Java SE APIs [Short/Character/Integer/Long].reverseBytes SLP analysis checks passes since relevant support already existed. This bug fix patch handles missing scalar reversebyte opcode checks in SLP backed to enable creation of corresponding vector IR nodes.
A new JBS issue JDK-8290034 is created to add the missing auto-vectorization support for bit reverse operation targeting JDK mainline.
Kindly review and share your feedback.
Best Regards,
Jatin
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk19 pull/128/head:pull/128
$ git checkout pull/128
Update a local copy of the PR:
$ git checkout pull/128
$ git pull https://git.openjdk.org/jdk19 pull/128/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 128
View PR using the GUI difftool:
$ git pr show -t 128
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/128.diff