RFR: 8356647: C2: Excessively strict assert in PhaseIdealLoop::do_unroll [v3]

[Marc Chevalier]

On Mon, 26 May 2025 06:53:46 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> src/hotspot/share/opto/loopTransform.cpp line 1922:
>> 
>>> 1920:     // Let's check we are in a surprise-free situation, that should be the only one reachable
>>> 1921:     // here. => old_trip_count was set, is reliable, and is small enough to be sure that `stride_con`
>>> 1922:     // will also be small enough, and no overflow risk.
>> 
>> Can't we just say that the old trip count is only set as exact in `compute_trip_count()` if it is less than `max_juint` and otherwise, it's inexact and we don't enter this path? You could also mention that adding `stride_m` is then overflow safe.
>
> We can say that, of course, but that tells what it is (for now), but doesn't really explain (or mention) that's it's actually an important property, and not just something that happens by chance. If I'm to change `compute_trip_count()` (or add another call to `set_exact_trip_count` and I read about that, I need to pay attention to still make the assert pass because I need this invariant. I should not remove it thinking "well, not true anymore, but it's fine". So I'd say saying that it's set in `compute_trip_count()` is the why this assert should pass, but not why it's important it passes.
> 
> I'll add what you suggest, of course.

I've tried a phrasing. Feel free to take a look!

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

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