-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8321278: C2: Partial peeling fails with assert "last_peel <- first_not_peeled" #18353
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 281 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
|
test/hotspot/jtreg/compiler/loopopts/TestPartialPeelingAtSingleInputRegion.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public static void main(String[] args) { | ||
for (int i = 0; i < 50_000; ++i) { |
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.
Just a minor thing: Do you really need 50000 iterations or would fewer be sufficient to trigger 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.
Good catch. It reproduces with 10000. I pushed a comment that makes that change.
src/hotspot/share/opto/domgraph.cpp
Outdated
Node* dom = tdom->_control; | ||
if (dom != C->root() && dom->is_Region() && dom->req() == 2) { | ||
remove_single_entry_region(t, tdom, dom, _igvn); | ||
} | ||
_idom[t->_control->_idx] = dom; // Set immediate dominator |
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.
Removing the regions during Dominators()
seems reasonable. I guess doing a pass of IGVN after Dominators()
is probably too much to get these regions removed?
Could you also remove the region and the phi where the unreachable loops are cleaned up and the region and phis become single-entry nodes? I.e. here:
jdk/src/hotspot/share/opto/domgraph.cpp
Lines 453 to 463 in ce7ebaa
// Kill dead input path | |
assert( !visited.test(whead->in(i)->_idx), | |
"input with no loop must be dead" ); | |
_igvn.delete_input_of(whead, i); | |
for (DUIterator_Fast jmax, j = whead->fast_outs(jmax); j < jmax; j++) { | |
Node* p = whead->fast_out(j); | |
if( p->is_Phi() ) { | |
_igvn.delete_input_of(p, i); | |
} | |
} | |
i--; // Rerun same iteration |
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 reviewing this.
Removing the regions during
Dominators()
seems reasonable. I guess doing a pass of IGVN afterDominators()
is probably too much to get these regions removed?
It does feel like a lot of overhead for such a simple corner case.
Could you also remove the region and the phi where the unreachable loops are cleaned up and the region and phis become single-entry nodes? I.e. here:
I considered it but it felt harder. The algorithm collects cfg nodes in dfs and then iterate over them several times. If we remove a region at the point you mention, it would need to be removed from the dfs node list. Or we would need to make later iterations over the dfs node list handle dead region nodes. It's not a problem when it's done later as I propose 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.
Thanks for reviewing this.
Removing the regions during
Dominators()
seems reasonable. I guess doing a pass of IGVN afterDominators()
is probably too much to get these regions removed?It does feel like a lot of overhead for such a simple corner case.
Indeed, I don't think it's worth.
Could you also remove the region and the phi where the unreachable loops are cleaned up and the region and phis become single-entry nodes? I.e. here:
I considered it but it felt harder. The algorithm collects cfg nodes in dfs and then iterate over them several times. If we remove a region at the point you mention, it would need to be removed from the dfs node list. Or we would need to make later iterations over the dfs node list handle dead region nodes. It's not a problem when it's done later as I propose here.
I see, thanks for the explanation. Then it makes sense to handle this edge-case like you proposed to keep things simple. Maybe you can add a comment accordingly why we remove the region at this point.
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 see, thanks for the explanation. Then it makes sense to handle this edge-case like you proposed to keep things simple. Maybe you can add a comment accordingly why we remove the region at this point.
Sure. I added a comment. Let me know if it looks ok to you.
…eInputRegion.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
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 too.
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, thanks for the update!
@chhagedorn @TobiHartmann thanks for the reviews! |
/integrate |
Going to push as commit af15c68.
Your commit was automatically rebased without conflicts. |
The assert fails because peeling happens at a single entry
Region
. ThatRegion
only has a single input because other inputswere found unreachable and removed by
PhaseIdealLoop::Dominators()
. The fix I propose is to havePhaseIdealLoop::Dominators()
remove theRegion
and itsPhi
sentirely in this case.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18353/head:pull/18353
$ git checkout pull/18353
Update a local copy of the PR:
$ git checkout pull/18353
$ git pull https://git.openjdk.org/jdk.git pull/18353/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18353
View PR using the GUI difftool:
$ git pr show -t 18353
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18353.diff
Webrev
Link to Webrev Comment