RFR: 8307619: C2 failed: Not monotonic (AndI CastII LShiftI) in TestShiftCastAndNotification.java [v2]

[Tobias Hartmann]

On Mon, 15 May 2023 11:14:49 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> **The Problem**
>> 
>> During CCP, we get to a state like that:
>> 
>> x (int:1)   Phi (int:4)
>> |           |
>> |     +-----+
>> |     |
>> LShiftI (int:16)
>>   |
>> CastII (top)       ConI (int:3)
>>   |                 |
>>   +----+  +---------+
>>        |  |
>>        AndI
>> 
>> 
>> We call `AddINode::Value` during CCP, and in `MulNode::AndIL_shift_and_mask_is_always_zero` we `uncast` both inputs, which leaves us with `LShiftI` and `ConI` as the "true" inputs. They both have non-top types, and so we determine that this `AndI-LShiftI` combination always leads to `zero`: The `Phi` has a constant type (`int:4`). So this leaves the lowest 4 bits zero after the `LShiftI`. Then and-ing that with `int:3` means we extract the lowest 3 bits that are zero. So the result is provably always zero - that is the idea.
>> 
>> Then, we have some type updates (here of `x` and `Phi` and `LShiftI`), and the graph looks like this:
>> 
>> x (int)    Phi (int:0..4)
>> |           |
>> |     +-----+
>> |     |
>> LShiftI (int)
>>   |
>> CastII (top)       ConI (int:3)
>>   |                 |
>>   +----+  +---------+
>>        |  |
>>        AndI
>> 
>> 
>> This leads to `shift2` failing to have constant type:
>> https://github.com/openjdk/jdk/blob/a6b72f56f56b4f33ac163e90b115d79b2b844999/src/hotspot/share/opto/mulnode.cpp#L1964-L1967
>> 
>> And with that, we fall back to `MulNode::Value`:
>> https://github.com/openjdk/jdk/blob/a6b72f56f56b4f33ac163e90b115d79b2b844999/src/hotspot/share/opto/mulnode.cpp#L559-L566
>> 
>> In `MulNode::Value` we detect that the `CastII` has type `top`, and return `top` for `AndI`.
>> 
>> CCP expects the types to become more wide over time, so going from `int:0` to `top` is the wrong direction.
>> 
>> **Solution**
>> 
>> The problem is with the relatively rare `CastII` still being `top` - this seems to be very rare. But the new regression test `TestShiftCastAndNotification.java` seems to create exactly that case, in combination with `-XX:StressCCP`.
>> 
>> We should guard against `top` in one of the `AndI` inputs inside `MulNode::AndIL_shift_and_mask_is_always_zero`. This will prevent it from detecting the zero-case, untill `MulNode::Value` would get a chance to compute a non-top type.
>> 
>> **Argument for Solution**
>> 
>> Is there still a threat from `MulNode::AndIL_shift_and_mask_is_always_zero` computing a zero first, and `MulNode::Value` a type that does not include zero after ward?
>> As types only widen during CCP, having a zero first means that all inputs now are non-top - in fact th...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   refactor with @chhagedorn's suggestions

Looks good to me.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13908#pullrequestreview-1430313800