-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8370200: Crash: assert(outer->outcnt() >= phis + 2 - be_loads && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis #28677
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
Conversation
|
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
|
@rwestrel 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 119 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 |
Webrevs
|
dlunde
left a comment
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.
Thanks for working on this @rwestrel!
Now, PhiNode::Identity for 94 could replace it with the bottom memory phi with same inputs 451. But it doesn't run. It last ran between 3) and 4) and there's no reason for igvn to execute it again because 4) doesn't cause 94 to change in any way.
Just to double check, does VerifyIterativeGVN identify this missed transformation? If not, we should make sure it does.
The fix I propose is to mirror the transformation from PhiNode::Identity in PhiNode::Ideal so the end result doesn't depend on what phi is modified and processed by igvn last.
Correct me if I'm wrong, but do we not achieve the same thing if we identify and add 94 to the worklist after the transformation of 93 -> 451? This possibly seems like a cleaner solution to me (see my code comment below).
src/hotspot/share/opto/cfgnode.cpp
Outdated
| // PhiNode::Identity replaces a non bottom memory phi with a bottom memory phi with same inputs if it exists | ||
| // This performs the mirror transformation: it looks for non bottom memory phis with same inputs as this bottom memory | ||
| // phi and replaces them by this phi. | ||
| // The reason for having the same transformation in 2 places is so all candidates are transformed. For instance, if | ||
| // the bottom memory phi's inputs are changed (so it can now replace the non bottom memory phi) only after the non | ||
| // bottom memory phi is processed by igvn, having the transformation in PhiNode::Identity is not sufficient | ||
| if (can_reshape && type() == Type::MEMORY && adr_type() == TypePtr::BOTTOM) { | ||
| PhaseIterGVN* igvn = phase->is_IterGVN(); | ||
| uint phi_len = req(); | ||
| Node* phi_reg = region(); | ||
| for (DUIterator_Fast imax, i = phi_reg->fast_outs(imax); i < imax; i++) { | ||
| Node* u = phi_reg->fast_out(i); | ||
| assert(!u->is_Phi() || (u->in(0) == phi_reg && u->req() == phi_len), "broken Phi/Region subgraph"); | ||
| if (u->is_Phi() && u->as_Phi()->can_be_replaced_by(this)) { | ||
| igvn->replace_node(u, this); | ||
| --i; --imax; | ||
| } | ||
| } | ||
| } | ||
|
|
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.
It seems a bit out of place to transform other Phi nodes in the Ideal call for this Phi node. Can't we just instead readd all matching other Phis that we can transform to the worklist, and let IGVN handle it in a subsequent iteration?
It doesn't because
In principle, yes. |
I'm not sure about the exact mechanism, but it would be nice if
Right, this is what I propose to fix the present issue and it seems cleaner to me (we let |
|
I just started some internal testing, will come back with results in a day or two, and hopefully also start reviewing this soon. |
test/hotspot/jtreg/compiler/loopstripmining/TestMismatchedMemoryPhis.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/loopstripmining/TestMismatchedMemoryPhis.java
Outdated
Show resolved
Hide resolved
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.
Thanks for fixing this @rwestrel!
Right, this is what I propose to fix the present issue and it seems cleaner to me (we let Identity handle the identity transformations). I doubt there'll be a measurable compilation time difference.
It seems to be slightly simpler as well and a bit more inline with what Ideal should do. That said, I'm not too sure of what the "guidelines" are. So, really I have no strong opinions either.
| } | ||
|
|
||
| bool PhiNode::can_be_replaced_by(const PhiNode* other) const { | ||
| return type() == Type::MEMORY && other->type() == Type::MEMORY && adr_type() != TypePtr::BOTTOM && |
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.
I think I might miss something but I was wondering if we strictly need to check for adr_type() != TypePtr::BOTTOM
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.
Are you suggesting we could do:
bool PhiNode::can_be_replaced_by(const PhiNode* other) const {
return type() == Type::MEMORY && other->type() == Type::MEMORY && other->adr_type() == TypePtr::BOTTOM && has_same_inputs_as(other);
}
?
If there are 2 memory Phis with same inputs and same adr_type then global value numbering should common them so that would make no difference.
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.
Yes, that's what I was thinking (to have one less operand).
Test results look good except for trivial failures in product runs of the new tests, due to missing
Like Daniel and Damon, I also have a slight preference towards enqueuing the node and letting |
…ryPhis.java Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>
…ryPhis.java Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>
Thanks @dlunde @dafedafe @robcasloz for the comments. I made that change. |
Thanks Roland, will come back with internal test results. |
dlunde
left a comment
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 @rwestrel! A few very minor suggestions.
test/hotspot/jtreg/compiler/loopstripmining/TestMismatchedMemoryPhis.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel Lundén <daniel.lunden@oracle.com>
…ryPhis.java Co-authored-by: Daniel Lundén <daniel.lunden@oracle.com>
Co-authored-by: Daniel Lundén <daniel.lunden@oracle.com>
dafedafe
left a comment
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 @rwestrel!
robcasloz
left a comment
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!
test/hotspot/jtreg/compiler/loopstripmining/TestMismatchedMemoryPhis.java
Outdated
Show resolved
Hide resolved
| } catch (NullPointerException npe) { | ||
| // Expected | ||
| } |
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.
Could this exception be avoided, and still reproduce the bug?
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.
Failure doesn't reproduce without it.
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.
So iArrFld[] must be null for this to reproduce?
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.
I tried to reproduce it without it but couldn't. I'm not sure why the exception handling code is needed but it seems it is.
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.
But the main method is not even compiled, right? Do we ever deopt, and then recompile maybe? I suppose don't worry about it too much. I just don't like random catch statements, because they could hide bugs down the line. But it is a very slim chance that this would happen anyway.
| // PhiNode::Identity replaces a non-bottom memory phi with a bottom memory phi with the same inputs, if it exists. | ||
| // If the bottom memory phi's inputs are changed (so it can now replace the non-bottom memory phi) or if it's created | ||
| // only after the non-bottom memory phi is processed by igvn, PhiNode::Identity doesn't run and the transformation | ||
| // doesn't happen. | ||
| // Look for non-bottom Phis that should be transformed and enqueue them for igvn so that PhiNode::Identity executes for | ||
| // them. | ||
| if (can_reshape && type() == Type::MEMORY && adr_type() == TypePtr::BOTTOM) { | ||
| PhaseIterGVN* igvn = phase->is_IterGVN(); | ||
| uint phi_len = req(); | ||
| Node* phi_reg = region(); | ||
| for (DUIterator_Fast imax, i = phi_reg->fast_outs(imax); i < imax; i++) { | ||
| Node* u = phi_reg->fast_out(i); | ||
| assert(!u->is_Phi() || (u->in(0) == phi_reg && u->req() == phi_len), "broken Phi/Region subgraph"); | ||
| if (u->is_Phi() && u->as_Phi()->can_be_replaced_by(this)) { | ||
| igvn->_worklist.push(u); | ||
| } | ||
| } | ||
| } |
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.
Another drive-by question:
You are refactoring / fixing existing optimizations:
Are there IR tests that cover the original optimization? How do we avoid that we lose optimizations here?
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.
None was integrated with the initial change. I added one.
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.
And how confident are you that this one test ensures there won't be a regression?
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.
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.
@dlunde @robcasloz @rwestrel I leave this to you all. If you are very sure that the change is trivial, and that no additional IR tests are helpful, then leave it. But I've seen it happen multiple times that seemingly "trivial" changes have suddenly disabled older optimizations, and nobody noticed in the review. That's why I'm cautious in these cases.
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.
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.
In this case, the only thing we do is igvn->_worklist.push(u);, which should be harmless and really only enable further optimizations.
If you are sure about that. I have not looked at it in depth. But what I see is also that code was moved from Identity to Ideal, and refactored.
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.
Code was refactored but not moved. It is fairly similar to bugs fixed by adding logic to PhaseIterGVN::add_users_of_use_to_worklist().
…ryPhis.java Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
|
What should I do with this change? Should I go ahead and integrate? |
I'm happy with the current version of this changeset, please give me one or two days to re-run it through or CI test system. |
robcasloz
left a comment
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.
Test results look good.
|
@robcasloz @dlunde @dafedafe thanks for the reviews and testing |
|
/integrate |
|
Going to push as commit 2ba423d.
Your commit was automatically rebased without conflicts. |
The crash occurs because verification code expects the inner and outer
loop of a loop strip mining nest to have the same number of phis but,
in this case, the inner loop has one more memory phis than the outer
loop.
OuterStripMinedLoopNode::adjust_strip_mined_loop, inner andouter loops have the same number of phis, as expected.
PhiNode::Idealruns for 429 and pushed theMergeMem309through the outer loop phi:
PhiNode::Identityruns for 430 and finds that it can be replaceby 429: the non bottom memory phi 430 can be replaced by the bottom
memory 429 that has the same inputs.
PhiNode::Idealruns for 93 and pushed theMergeMemthrough thatPhi:Now,
PhiNode::Identityfor 94 could replace it with the bottommemory phi with same inputs 451. But it doesn't run. It last ran
between 3) and 4) and there's no reason for igvn to execute it again
because 4) doesn't cause 94 to change in any way.
The fix I propose is to mirror the transformation from
PhiNode::IdentityinPhiNode::Idealso the end result doesn'tdepend on what phi is modified and processed by igvn last.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28677/head:pull/28677$ git checkout pull/28677Update a local copy of the PR:
$ git checkout pull/28677$ git pull https://git.openjdk.org/jdk.git pull/28677/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28677View PR using the GUI difftool:
$ git pr show -t 28677Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28677.diff
Using Webrev
Link to Webrev Comment