8272562: C2: assert(false) failed: Bad graph detected in build_loop_late #5716
Conversation
|
|
Webrevs
|
src/hotspot/share/opto/loopopts.cpp
Outdated
| @@ -1445,7 +1445,8 @@ void PhaseIdealLoop::try_sink_out_of_loop(Node* n) { | |||
| !n->is_MergeMem() && | |||
| !n->is_CMove() && | |||
| !is_raw_to_oop_cast && // don't extend live ranges of raw oops | |||
| n->Opcode() != Op_Opaque4) { | |||
| n->Opcode() != Op_Opaque4 && | |||
| (n->Opcode() == Op_CastII && ((CastIINode*)n)->has_range_check())) { | |||
There was a problem hiding this comment.
Shouldn't the condition be inverted? This would only allow to sink range check CastII nodes.
And couldn't you use !(n->is_CastII() && n->as_CastII()->has_range_check()) instead of the explicit cast?
There was a problem hiding this comment.
Right! Thanks for catching this. Actually after thinking more about this, I changed the fix to prevent all nodes that can capture a type from being sunk to be on the safe side, for now.
There was a problem hiding this comment.
Sounds good, I guess you will undo that with your general fix later?
|
@rwestrel 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 135 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.
|
|
@chhagedorn @vnkozlov Thanks for the reviews. |
|
/integrate |
|
Going to push as commit 980c50d.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
A counted loop has an array access:
(LoadI (AddP (AddP (ConvI2L (CastII (AddI (Phi ...)))))))
that is
array[iv - 1]
The Phi is the iv Phi. The ConvI2L/CastII are from a range check
and capture type: 0..maxint-1 The loop is unrolled once, the
LoadI is cloned.
array[iv - 1]
array[iv]
The first LoadI is only used out of loop and is sunk with 2
clones. One of the clones is on the IfFalse branch of a test
for iv != 0.
(LoadI (AddP (AddP (ConvI2L (CastII (AddI (CastII ...)))))))
The second CastII pins the nodes out of the loop. The ConvI2L and
CastII are pushed thru the AddI (for -1). As a result the ConvI2L
has type:
1..maxint-1
The CastII, because it has the same input as the input for iv != 0,
becomes 0 which is not part of 1..maxint-1. The ConvI2L and all its
uses including the LoadI become top. The use of the LoadI is a Phi
that is transformed into its remaining input and the graph is broken.
The root cause is that the loop body initially contains:
if (iv - 1 >=u array.length) { // range check
trap();
}
if (iv == 0) {
// path where nodes are sunk later on
}
And obviously if iv - 1 >= 0 then iv == 0 is always false but c2 fails
to prove it. I tried to implement a simple fix for this issue but
while it fixes this bug, I couldn't convince myself that it was robust
enough.
So instead I propose following the suggestion Christian and Vladimir I.
made in:
#5199
that is to more generally exclude cast nodes from sinking as a
workaround for now.
I've been looking for a more general solution to this problem and I
have a prototype that fixes this failure but is a lot more
complicated. I'll revisit this workaround when it's ready.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5716/head:pull/5716$ git checkout pull/5716Update a local copy of the PR:
$ git checkout pull/5716$ git pull https://git.openjdk.java.net/jdk pull/5716/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5716View PR using the GUI difftool:
$ git pr show -t 5716Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5716.diff