RFR: 8308504: C2: "malformed control flow" after JDK-8303466

[Roland Westrelin]

On Thu, 22 Jun 2023 11:08:36 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This is another case where imprecise type computation leads to corrupted control flow.
>> 
>> The loop incr `AddI` does never overflow at runtime, this is guaranteed by the loop limit check (we ensure that adding the incr value to the limit would not overflow). However, the type so far did often overflow, and returns type `int`. The loop  tripcount `Phi` does not have such type overflow. So we had cases where the Phi had type `minint...7`, but the AddI had type `int`, since it adds `-3`, which makes the lower bound underflow (even though the runtime value never would). If we had known that an underflow is impossible because of the limit check, then we could have had type `minint...4` for the incr AddI.
>> 
>> Since JDK-8303466, the type of the limit can now not overflow and is more precise. In the example, the limit is `8...maxint`. The CastII after the zero-trip-guard thus concludes that the type must be `9...maxint` (we have a decrement loop).
>> 
>> Since the main-loop knows that the trip-count Phi has type `minint...7`, we get an empty range (intersection of `9...maxint` and `minint...7`), and the main-loop (in this case one of the assertion predicates) start to corrode away because of TOP inputs. This creates malformed control flow, we have an if with only one projection.
>> 
>> **Why did this arise with JDK-8303466?** We made the loop-limit type more precise, forbidding overflow. So before that change I think the loop limit just had a type would have overfown, and returned `int`. That would have not allowed the main-loop internals to detect that it could never be reached. The main-loop would not have died at all, but it would still never have been entered.
>> 
>> **Solution** Since the AddI (incr) of the tripcount Phi cannot overflow/underflow, **we should also prevent the type of the incr from overflowing/underflowing**. That means that it basically has the same type as the Phi, if not even a bit better. And this means that the type of the incr used to compare against the loop-limit is consistent with the type information in the main-loop.
>> 
>> I also had to improve the notification in IGVN, I encountered a case where a incr node was replaced via Identity, so the new incr node needed its type to be tightened (no overflow).
>> 
>> **Risk** Just as JDK-8303466, this fix here makes types more precise. That can always mean that somewhere else we also need more precise types than we currently have. It is difficult to know what such bugs are still lurking for us.
>>...
>
> It doesn't seem to be true that the loop incr never overflows in the general case. See this example:
> 
> public class TestOverflowCountedLoopIncr {
>     public static void main(String[] args) {
>         for (int i = 0; i < 20_000; i++) {
>             test(Integer.MAX_VALUE);
>         }
>     }
> 
>     private static float test(int start) {
>         float v = 1;
>         int i = start;
>         do {
>             synchronized (new Object()) {}
>             v *= 2;
>             i++;
>         } while (i < Integer.MIN_VALUE + 100);
>         return v;
>     }
> }
> 
> That one also hangs with -XX:LoopMaxUnroll=0. A bug in LSM it seems. I will file a bug for that.

> @rwestrel thanks for the idea. So you mean I should make the `Phi` type narrow enough, such that adding the `stride` to it should never lead to a type overflow, right? Then there would be no type overflow in the `incr`.

That's what I was thinking indeed.

> You want to `CastII` the limit for the exit check, I assume? And we would have to add a `CastII` again if the limit is ever changed during the unrolling, right?

Would that be required? I was hoping that the CastII on the limit before unrolling would cause the phi type to be narrow 
enough.

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

PR Comment: https://git.openjdk.org/jdk/pull/14331#issuecomment-1602453662