[jdk17] RFR: 8268883: C2: assert(false) failed: unscheduable graph [v2]

[Vladimir Kozlov]

On Mon, 5 Jul 2021 14:02:23 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The IR graph has a diamond shaped:
>> (If (Bool (CmpI ...)))/Region/Phi.
>> 
>> One of the Phi's input is a chain of arithmetic operations. The last
>> of the chain is a CastII in one branch of the If. The input of the
>> CastII is also the input of a CmpI node that's input to the If. The
>> other input of the CmpI becomes a constant. That causes the CastII's
>> type be narrower because of logic in CastIINode::Value(). The improved
>> type percolates through the chain of arithmetic operations. The type
>> of a ConvL2I in the chain is updated to a narrower type as a
>> consequence. Now the other input of the CmpI becomes a constant and as
>> a consequence the branch of the diamond where the arithmetic
>> operations are is never taken. What would next happen, is the ConvL2I
>> becomes top (because its narrow type and its input type do not
>> overlap), one of the Phi inputs would become top and the Phi would be
>> replaced by the remaining input. The If would constant fold and only
>> one branch would be kept. But before that has a chance to happen,
>> PhiNode::Ideal() runs and is_cond_add() transforms it into a:
>> 
>> (AddI (AndI ..))
>> 
>> with one input that's the ConvL2I. When that input becomes top, the
>> AndI becomes top and the AddI that replaced the Phi too. So indirectly
>> the Phi is replaced by top which shouldn't have happened.
>> 
>> The fix I propose is to delay the is_cond_add() (and similar
>> transformations) until the CmpI has a chance to be transformed. I
>> think it's enough to fix this issue because:
>> 
>> - As long as one of the If projections has a use other than the
>>   Region/Phi (such as the CastII), is_cond_add() is not applied
>> 
>> - the CastII is the root of the problem because it's what narrows the
>>   type so much that some node becomes top. The CastII can only be
>>   removed once its input becomes constant and in that case the CmpI's
>>   input is also a constant and the CmpI is enqueued for IGVN
>
> 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 five additional commits since the last revision:
> 
>  - comment
>  - Merge branch 'master' into JDK-8268883
>  - whitespaces
>  - test
>  - fix

Good.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/201