8282590: C2: assert(addp->is_AddP() && addp->outcnt() > 0) failed: Don't process dead nodes #7728
Conversation
|
|
Webrevs
|
| @@ -297,6 +297,16 @@ bool ArrayCopyNode::prepare_array_copy(PhaseGVN *phase, bool can_reshape, | |||
| dest_offset = Compile::conv_I2X_index(phase, dest_offset, ary_dest->size()); | |||
| if (src_offset->is_top() || dest_offset->is_top()) { | |||
| // Offset is out of bounds (the ArrayCopyNode will be removed) | |||
| // Below: make sure that we record the nodes, so that they can be deleted later (if they are dead) | |||
There was a problem hiding this comment.
What about this?
src_offset = Compile::conv_I2X_index(phase, src_offset, ary_src->size());
if (src_offset->is_top()) {
return false;
}
dest_offset = Compile::conv_I2X_index(phase, dest_offset, ary_dest->size());
if (dest_offset->is_top()) {
if (can_reshape) {
phase->is_IterGVN()->_worklist.push(src_offset);
}
return false;
}
| if (!backward_mem->is_top() && backward_mem->outcnt() == 0) { | ||
| phase->is_IterGVN()->_worklist.push(backward_mem); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think you need to check if the mem is really dead. Adding it to the worklist does not hurt. Would this work as well?
if (can_reshape) {
phase->is_IterGVN()->_worklist.push(mem):
}
| if (can_reshape) { | ||
| if (!src_offset->is_top()) { |
There was a problem hiding this comment.
And looking at this again, src_offset can't be top, right? We just checked it above.
|
@eme64 This change now passes all automated pre-integration checks. 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 158 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @chhagedorn) but any other Committer may sponsor as well.
|
|
Thanks @TobiHartmann and @chhagedorn for the help, suggestions for improvement and reviews! |
|
/sponsor |
|
Going to push as commit d29c7e7.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@TobiHartmann @eme64 Pushed as commit d29c7e7. |
Problem:
Sometimes nodes are generated, and then not properly added to any other node, nor added to the worklist, so that it could be deleted. In some cases, a node can thus survive after IGVN and enter into
ConnectionGraph::compute_escape, where no dead nodes are expected (except constants).In
ArrayCopyNode::prepare_array_copy:In one, I now add the new ConvI2LNode to the worklist if we are aborting.
In the other case, I could safely reorder the code, such new ConvI2LNode would only be generated once we know we are not going to abort.
In
SubTypeCheckNode::Ideal:Replaced
set_reqwithset_req_X, such that the input-node that is replaced (which is now potentially a dead node) is put on the worklist.In
ArrayCopyNode::Ideal:If we decide to
return NULLwe also need to add the potentially dead forward / backward memory nodes to the worklist, they may turn out to be dead.Added a regression test: the test + flag combination that first reproduced this issue.
All Tests, including this new regression test are passing. Tests rerun after small refactorings.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7728/head:pull/7728$ git checkout pull/7728Update a local copy of the PR:
$ git checkout pull/7728$ git pull https://git.openjdk.java.net/jdk pull/7728/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7728View PR using the GUI difftool:
$ git pr show -t 7728Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7728.diff