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

[Dean Long]

On Tue, 20 May 2025 08:27:35 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

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

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.

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

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