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

[Marc Chevalier]

On Mon, 19 May 2025 23:20:18 GMT, Dean Long <dlong at openjdk.org> wrote:

>> This assert seems a bit too tight. See the JBS issue to check the math: the bound of `trip_count` should be `<= 2^31`, while the current bound is ` < (julong)max_juint/2` = floor((2^32-1)/2) = (2^32-2) / 2 = 2^31-1.
>
> src/hotspot/share/opto/loopTransform.cpp line 1903:
> 
>> 1901:     jlong trip_count = (limit_con - init_con + stride_m)/new_stride_con;
>> 1902:     // New trip count should satisfy next conditions.
>> 1903:     assert(trip_count > 0 && (julong)trip_count <= (julong)1 << (sizeof(juint)*BitsPerByte-1), "sanity");
> 
> Suggestion:
> 
>     assert((julong)trip_count * 2 <= max_juint, "sanity");
> 
> This should catch negative values and any value that would make new_trip_count*2 below overflow.

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).

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

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