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
8282592: C2: assert(false) failed: graph should be schedulable #7758
Conversation
|
Webrevs
|
Gave this a quick run through testing and
|
Thanks for running tests. I pushed a new commit that should fix the test failure. |
I've submitted some testing again. |
Testing looked 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.
This looks reasonable to me.
@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 83 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.
|
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.
@chhagedorn @TobiHartmann thanks for the reviews |
/integrate |
Going to push as commit 85628a8.
Your commit was automatically rebased without conflicts. |
This is another case of overunrolling that causes the type of the main
loop's iv to conflict with a ConvI2L of an array access. Skeleton
predicates were added to solve this family of issues. A predicate that
should catch that the main loop is unreachable is indeed added but the
predicate's condition doesn't constant fold.
What happens is that the main loop's iv phi initial value is a CastII
(added by PhaseIdealLoop::cast_incr_before_loop()). That CastII is
input to the predicates above the main loop and of the main loop's iv
phi. Once loop opts are over, the type of the CastII is updated by
CastIINode::Value() (the logic that looks at the test that guards the
CastII). The type update causes the main loop iv phi's type to be
updated and a ConvI2L to be become top. The predicate that should
catch the overunrolling has the shape:
(CmpU (AddI#1 (CastII (AddI#2 (Phi ..) ConI#1)) ConI#2) ConI#3)
In the case of the test case:
ConI#3 = 10
ConI#2 = 7
ConI#1 = 1
CastII has type = int:<=max-8
Phi has type = int:>=8
As a result:
AddI#2 has type int
AddI#1 has type int:min+7..max-1
The Phi is the pre loop Phi.
The fix I propose is that predicates when they are updated should skip
over the CastII, that is in this case:
(CmpU (AddI#1 (AddI#2 (Phi ..) ConI#1) ConI#2) ConI#3)
which is reshaped to:
(CmpU (AddI#1' (Phi ..) ConI#1') ConI#3)
In this case:
ConI#1 = 4 (because the predicate constant fold in earlier passes of
unrolling)
As a result AddI#1' still has type int But CmpUNode has special logic
to deal with the (CmpU (AddI ...)) shape and it can determine that
this one always fails.
The role of the CastII is to enforce the dependency between nodes in
the loop and the the loop entry test so it's not required for
predicates.
With that change, compiler/loopopts/TestOverunrolling.java fails
because a predicate doesn't constant fold anymore. The reason in that
case, is that PhaseIdealLoop::reorg_offsets() adds an Opaque2 between
the pre loop iv phi and the predicate that hides the type of the iv
phi. Without the change, the CastII captures the type of the iv phi
and the Opaque2 node is added between the CastII and the pre loop iv
phi. To solve that problem, I propose that
PhaseIdealLoop::reorg_offsets() always add a CastII after the Opaque2
so the type of the iv phi is not lost.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7758/head:pull/7758
$ git checkout pull/7758
Update a local copy of the PR:
$ git checkout pull/7758
$ git pull https://git.openjdk.java.net/jdk pull/7758/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7758
View PR using the GUI difftool:
$ git pr show -t 7758
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7758.diff