RFR: 8262017: C2: assert(n != __null) failed: Bad immediate dominator info. [v2]

[Roland Westrelin]

On Tue, 8 Jun 2021 15:53:38 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> The wrong dominance information is caused by a `ConvL2I` node whose input gets a wrong type which is out of the range of the `ConvL2I`. As a consequence, the node becomes top and some data nodes are folded away which eventually results in the dominance assertion failure.
>> 
>> The wrong type information can be traced back to the overflow/underflow handling in range check elimination. Originally, the code in `PhaseIdealLoop::add_constraint()` covered the following special case for the adjustment of the pre loop limit:
>> https://github.com/openjdk/jdk/blob/cd33abb136d415455090cef4fe6211d9e6940948/src/hotspot/share/opto/loopTransform.cpp#L2423-L2456
>> 
>> This code, however, was removed later by accident:
>> https://github.com/openjdk/jdk/commit/afd852ccb8f8d36c721963ba68de695ce3e0e23d#diff-6a59f91cb710d682247df87c75faf602f0ff9f87e2855ead1b80719704fbedffL2370-L2394
>> 
>> This was only revealed after enabling more precise type information for phi nodes in [JDK-8257813](https://bugs.openjdk.java.net/browse/JDK-8257813) after which the testcase starts to fail.
>> 
>> The proposed fix is straight forward to reintroduce the wrongly removed code and adapt it to long instead of int.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add case for positive stride*scale

Roughly, the problem as I understand it from your description/patch is that: new bounds are computed using long arithmetic which causes bounds to not fit in an int and to be incorrect once the bounds are casted to an int. Is that it? Can you give some more details?

So the previous patch dropped some overflow checks under the assumption, I suppose, that there was no need for them because of the move to long arithmetic and now we're finding that it's doesn't really work that way. Was long arithmetic the right move then? With the previous code added back, does it still solve problems? Or was that approach fundamentally flawed?

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

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