RFR: 8272562: C2: assert(false) failed: Bad graph detected in build_loop_late [v2]

[Christian Hagedorn]

On Mon, 27 Sep 2021 13:57:56 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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:
>> 
>> https://github.com/openjdk/jdk/pull/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.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   conservative fix

Marked as reviewed by chagedorn (Reviewer).

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

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