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

[Christian Hagedorn]

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

I had to dig a little bit deeper to fully get that code. I was missing some points before but now my improved understanding is the following:
The approach with longs only partially solved the underflow/overflow problem addressed in `PhaseIdealLoop::adjust_limit`. The goal was to clamp the limits like that:
https://github.com/openjdk/jdk/blob/94d0b0f9810bd1a8da06ec267a1c7589d6cb756b/src/hotspot/share/opto/loopTransform.cpp#L2334-L2338
We achieve that by doing a long version of the inner min/max operation with cmoves. For example, let's look at the first inner MAX: `MAX(limit, min_jint)`: When `limit` is less than `min_jint` (integer underflow), then we will use `min_jint` and avoid the underflow problem. However, this only fixes the underflow case but not the overflow case of `limit` (which is the root cause of this bug). In this bug, `limit` is greater than `max_jint`. Therefore, `MAX(limit, min_jint)` takes `limit` (long comparison) which is then converted back to int resulting in a very small negative number near `min_jint` due to integer overflow. This value is less than `old_limit`, so the outer integer based MIN operation takes that value resulting in a wrong limit. This causes some folding operations later and we end up with an assertion failure. The other case in the comment above is similar but just reverted: integer overflow is handled but underflow is not.

Based on that I found that the previous fix was only partially working and has not fixed the root cause. I reverted it and pushed a new fix for it addressing the missing overflow/underflow handling described above. I now switched to a long version of min/max for the outer operation (see comment above) with cmoves as well and only then convert back to int. AFAICT this fixes the wrong limit computations and handles both integer overflow and underflow correctly. I tried to improve the comments in `PhaseIdealLoop::adjust_limit` to make the intentions of the code more clear.

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

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