RFR: 8282592: C2: assert(false) failed: graph should be schedulable [v2]

[Christian Hagedorn]

On Mon, 14 Mar 2022 09:16:52 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - test run failure
>  - Merge branch 'master' into JDK-8282592
>  - test & fix

I've submitted some testing again.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7758