[8u, 9] RFR(S): 8177095: Range check dependent CastII/ConvI2L is prematurely eliminated

[Tobias Hartmann]

Hi Vladimir,

On 22.03.2017 16:43, Vladimir Ivanov wrote:
> Looks good.

Thanks for looking at this!

> PS: after finishing JDK-8154831 I thought about (but never finished, unfortunately) implementing verification logic for range check dependent cast nodes to ensure they disappear in lockstep with the range check they depend on. It would make diagnosing such problems much easier.

Yes, I thought about this as well but verification code is non-trivial. Even existing verification code like -XX:+VerifyLoopOptimizations [1] and -XX:+VerifyOpto [2] is broken. Maybe we have time to do this for JDK 10.

Best regards,
Tobias

[1] https://bugs.openjdk.java.net/browse/JDK-6359849
[2] https://bugs.openjdk.java.net/browse/JDK-6394013

> On 3/22/17 3:31 PM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8177095
>>
>> JDK 9:
>> http://cr.openjdk.java.net/~thartmann/8177095/webrev.00/
>> JDK 8u:
>> http://cr.openjdk.java.net/~thartmann/8177095_8u/webrev.00/
>>
>> A CastII (or a ConvI2L before the fix for JDK-6675699 [1]) with a narrow type that represents the index of an array access is eliminated because its type range does not intersect with the input range. This happens because the node is pushed upwards through an AddI. However, the control path to the array access is not eliminated leaving the graph in a corrupted state. We either crash during loop optimizations because we use a value from a non-dominating region or we crash in the register allocator because a Phi has a TOP input (and therefore an undefined live range).
>>
>> For more details see changes in TestLoopPeeling.java:
>> Load1 and the corresponding range check are moved out of the loop and both are used after the old loop and the peeled iteration exit. For the peeled iteration, storeIndex is always Integer.MIN_VALUE and for the old loop it is 0. Hence, the merging phi has type int:<=0. Load1 reads the array at index ConvI2L(CastII(AddI(storeIndex, -1))) where the CastII is range check dependent and has type int:>=0. The CastII gets pushed through the AddI and its type is changed to int:>=1 which does not overlap with the input type of storeIndex (int:<=0). The CastII is replaced by TOP causing a cascade of other eliminations. Since the control path through the range check CmpU(AddI(storeIndex, -1)) is not eliminated, the graph is in a corrupted state. We fail once we merge with the result of Load2 because we get data from a non-dominating region.
>>
>> I attempted to fix this issue with JDK-6675699 but missed these cases. The problem is similar to JDK-8154831 [2] and affects the latest JDK 6, 7, 8 and 9.
>>
>> I disabled narrowing of range check dependent CastIIs (either through the CastII(AddI) optimization or through CastIINode::Ideal).
>>
>> Tested with customer provided reproducer, regression test and RBT (running).
>>
>> Thanks,
>> Tobias
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-6675699
>> [2] https://bugs.openjdk.java.net/browse/JDK-8154831
>>