RFR: 8355488: Add stress mode for C2 loop peeling [v3]

[Tobias Hartmann]

On Thu, 15 May 2025 12:12:31 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> Adding a `StressLoopPeeling` dev flag that randomize peeling.
>> 
>> ## Semantics
>> 
>> For now, the direction I've taken is to randomly take a decision in case of peeling, otherwise, rely on existing heuristics.
>> 
>> This requires to distinguish two things:
>> - not inlining because it's not legal: see for instance
>>   ```cpp
>>   assert(cl->trip_count() > 0, "peeling a fully unrolled loop");
>>   ```
>>   in `PhaseIdealLoop::do_peeling`
>> - not inlining because it doesn't seem profitable.
>> 
>> Peeling loops without a good reason (not containing an exiting `If` whose condition is not a member of the loop) but without a concrete way to forbid it should always be allowed. Let's stress it!
>> 
>> Peeling too many times is not a great idea either. It uses a lot of memory, of nodes... Also, it may prevent other optimisations from kicking in. And what about interaction with future stress flags? Let's limit peeling: we give a fixed number of opportunities to peel before we give up on peeling for good. That is not the same as limiting the amount of peeling we do. Indeed, if we bound the number of times we say "yes, please, peel" given enough requests, we will always reach the bound. If we limit the number of requests, we have a more evenly distributed amount of peeling, between 0 and the bound.
>> 
>> I've tried without the bound: I couldn't find any bug without the bound that would not reproduce with the bound. It only save some legitimate memory problems. Without a bound on the number of peeling opportunities, hotspot eats a lot of memory, but all the allocations seems reasonable: it just seems we ask too much. We could limit the number of nodes, to prevent peeling before we reach the memory limit, but that would also hinder other optimizations and (future) stress flags.
>> 
>> 
>> 
>> ## The Flag
>> 
>> The flag is very specialized, unlike a `StressLoopOpts` would be. My idea so far is "let's see". My idea is that it's good to be able to enable stress optimizations selectively, and have a flag like `StressLoopOpts` that would turn them all: we could use the general one in testing, and the finer-grain ones when debugging. A reason for that is that I don't see a real use-case for stressing some features but not others (which would make the number of combinations explode): having (for instance) `+StressLoopUnrolling +StressLoopPeeling` would sometimes behave like `+StressLoopUnrolling -StressLoopPeeling`, and so it's not very useful to test the latter.
>> 
>> But once again: let'...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Back to PRODUCT for consistency

Thanks for making these changes. Looks good to me, I just added some minor suggestions.

src/hotspot/share/opto/c2_globals.hpp line 838:

> 836:                                                                             \
> 837:   develop(bool, StressLoopPeeling, false,                                   \
> 838:           "Randomize peeling decision")                                     \

Suggestion:

          "Randomize loop peeling decision")                                \

src/hotspot/share/opto/loopTransform.cpp line 522:

> 520:       loop_head->_peeling_opportunities_count++;
> 521:       // In case of stress, let's just pick randomly...
> 522:       return phase->C->random() % 2 == 0 ? estimate : 0;

Suggestion:

      return ((phase->C->random() % 2) == 0) ? estimate : 0;

src/hotspot/share/opto/loopnode.hpp line 137:

> 135: 
> 136: #ifndef PRODUCT
> 137:   uint _peeling_opportunities_count = 0;

I think this should rather be named `_stress_peeling_attempts` or something.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25140#pullrequestreview-2843482129
PR Review Comment: https://git.openjdk.org/jdk/pull/25140#discussion_r2091156892
PR Review Comment: https://git.openjdk.org/jdk/pull/25140#discussion_r2091043445
PR Review Comment: https://git.openjdk.org/jdk/pull/25140#discussion_r2091155929