RFR: 8356647: C2: Excessively strict assert in PhaseIdealLoop::do_unroll

[Marc Chevalier]

On Tue, 20 May 2025 21:51:07 GMT, Dean Long <dlong at openjdk.org> wrote:

>> I'm not convinced this is relaxed enough or that it shouldn't overflow. Where we set trip_count:
>> https://github.com/openjdk/jdk/blob/e961b13cd68bc352b86af17c7e53df8537519beb/src/hotspot/share/opto/loopTransform.cpp#L133-L141
>> we have a check that trip count is `< 2^32 - 1`, but it seems to me that the value of `trip_count` there might be 2^32 or 2^32-1 (same computation as the code I'm fixing). It's fine: if it would not fit in the `uint` we don't record, fine, I guess. In the code I'm touching, `old_trip_count` is the value stored in the loop head previously. In the case where the new `trip_count` is 2^31, the old_trip_count haven't been set since construction, so it's still `2^32 - 1` but without the exact flag (not sure what it means). So in the case new trip_count is 2^31, old_trip_count is 2^32-1: the `*2` overflows and we get `adjust_min_trip == true`. Which I presume is harmless (or maybe necessary?). With the version you suggest, we would guard against the overflow and allow `trip_count == 2^31-1`, but at the cost of crashing in the case of `trip_count == 2^31`, which seems possible to me (and still have the overflow happens in product).
>
> OK, I was thinking we needed to prevent the *2 below from overflowing.  If we allow the *2 to overflow, then what's left is making sure the cast to uint doesn't change the value (overflow).  To do that, we could relax the assert above to <= max_juint, or even better, use checked_cast to convert to uint below.

Overflowing is probably not a good idea, but I don't think it's impossible, and my understanding is that it would nevertheless behave correctly. We can relax the assert to `<= max_juint` but I think that changes the intent of the assert. In my understanding, the point was just to make sure that the arithmetic seems reasonable: we start with a integer counter that can cover at most 2^32 values, so after unrolling, it can cover at most 2^31 values, if we find something else, we did something wrong. But we are bounding it by 2^31-2. Maybe it is ok to be more strict in the assert than what could happen in real life, and bound trip_count by `<= (julong)max_juint/2` (instead of `<`). We could see if we find (with stress flags) the case of `trip_count == 2^31`, and see then how it behaves, and in the meantime the bound would be tight enough not to let overflow happen. I think this case is possible, but maybe some global (possibly accidental) invariant forbid it for now, and indeed I never s
 een it happening so far.

In short, we can just replace `<` by `<=` in the original code, and see if it's enough.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25295#discussion_r2100019389