-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8308892: Bad graph detected in build_loop_late after JDK-8305635 #14196
Conversation
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
@chhagedorn 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. |
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!
Windows build failure in GHA testing is unrelated.
@chhagedorn 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 112 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 |
Thanks Roberto for your review! |
So which of the 3 predicates will get cleaned up? Are some of the 3 predicates for another loop that doesn't exist anymore? |
Sorry, the explanation of the bug was not precise enough - there is a missing piece: It should have cleaned up jdk/src/hotspot/share/opto/loopnode.cpp Lines 4072 to 4093 in 78aac24
So, we keep The useless predicates were added normally for the loop while the additional |
Do I read that correctly that parsing inserts useless/duplicate predicates? |
After the first IGVN round, after parsing, they become useless because the |
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.
Thanks for the explanation. Can a test case be added? |
Thanks Roland for your review! I was able to come up with a test that fails with the same graph pattern. I've pushed an update. |
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.
Thanks Tobias for your review! I've simplified the test some more but the fix remains the same. |
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.
Still looks good.
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.
/integrate |
Going to push as commit 7dbdad5.
Your commit was automatically rebased without conflicts. |
@chhagedorn Pushed as commit 7dbdad5. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The cleanup done in JDK-8305635 wrongly identifies unrelated Parse Predicates which are not cleaned up, yet. It just walks from the entry of the loop up and tries to find each of the three Parse Predicates once but in no particular order. This order insensitive walk is wrong as seen in the following graph (from the attached replay file of this bug):
We first find
116 Parse Predicate
for Loop Predicates, then84 Parse Predicate
for Profiled Loop Predicates and then stop when finding71 Parse Predicate
for Loop Predicates because we've already found a Parse Predicate for Loop Predicates already. We then wrongly create Loop Predicates (above116 Parse Predicate
) which are below newly created Profiled Loop Predicates (above84 Parse Predicate
). This could lead to a bad graph because of data dependencies that rely on the fact that Loop Predicates are above Profiled Loop Predicates:jdk/src/hotspot/share/opto/loopPredicate.cpp
Lines 1529 to 1543 in 547a8b4
The fix is straight forward to make the assignment of Parse Predicate projections in
ParsePredicates
aware of the relative ordering constraint. Note that this class will be refactored again in JDK-8305636. But I think properly fixing this first is better than waiting for JDK-8305636 to go in.Testing: tier1-4, hs-precheckin-comp, hs-stress-comp
Thanks,
Christian
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14196/head:pull/14196
$ git checkout pull/14196
Update a local copy of the PR:
$ git checkout pull/14196
$ git pull https://git.openjdk.org/jdk.git pull/14196/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14196
View PR using the GUI difftool:
$ git pr show -t 14196
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14196.diff
Webrev
Link to Webrev Comment