RFR: 8347449: C2: UseLoopPredicate off should also turn UseProfiledLoopPredicate off [v3]

[Manuel Hässig]

On Wed, 26 Mar 2025 15:04:15 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Manuel Hässig has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   ir-framework: rename new nodes to convention
>
> src/hotspot/share/opto/c2_globals.hpp line 789:
> 
>> 787:   product(bool, UseProfiledLoopPredicate, true,                             \
>> 788:           "Move predicates out of loops based on profiling data. "          \
>> 789:           "Requires UseLoopPredicate to be turned on (default).")           \
> 
> It was already a bit vague before but I suggest to be more precise that we move checks with an uncommon trap out of a loop (and the resulting check before the loop is then a predicate):
> 
> Move checks with an uncommon trap out of loops based on profiling data. Requires [...]

Implemented in [f903729](https://github.com/openjdk/jdk/pull/24248/commits/f90372927a4d7ed82740014934f4409648d42bca)

> src/hotspot/share/opto/loopnode.cpp line 4304:
> 
>> 4302:     tty->print(" profile_predicated");
>> 4303:   }
>> 4304:   if (UseLoopPredicate && predicates.loop_predicate_block()->is_non_empty()) {
> 
> Maybe you can merge these blocks:
> 
> if (UseLoopPredicate) {
>   if (UseProfiledLoopPredicate && predicates.profiled_loop_predicate_block()->is_non_empty()) {
>     tty->print(" profile_predicated");
>   }
>   if (predicates.loop_predicate_block()->is_non_empty()) {
>     tty->print(" predicated");
>   }
> }

Implemented in [1aba1c2](https://github.com/openjdk/jdk/pull/24248/commits/1aba1c23f0e90e0a6717bdf7c441451b8e9c3efc)

> test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 1524:
> 
>> 1522:     public static final String PARSE_PREDICATE_LOOP = PREFIX + "PARSE_PREDICATE_LOOP" + POSTFIX;
>> 1523:     static {
>> 1524:         parsePredicateNodes(PARSE_PREDICATE_LOOP, "Loop");
> 
> I suggest the following names found in `predicates.hpp`:
> https://github.com/openjdk/jdk/blob/79bffe2f28f90986d45f4e91efc021290b4fc00a/src/hotspot/share/opto/predicates.hpp#L48-L50

Implemented in [7f24c87](https://github.com/openjdk/jdk/pull/24248/commits/7f24c87557da33bbb96a3596222b4737a06d9d31)

> test/hotspot/jtreg/compiler/predicates/TestDisabledLoopPredicates.java line 41:
> 
>> 39:     static final int WARMUP = 10_000;
>> 40:     static final int SIZE = 100;
>> 41:     static final int min = 3;
> 
> Since `min` is also a constant, you should capitalize it.

Implemented in [7f24c87](https://github.com/openjdk/jdk/pull/24248/commits/7f24c87557da33bbb96a3596222b4737a06d9d31)

> test/hotspot/jtreg/compiler/predicates/TestDisabledLoopPredicates.java line 46:
> 
>> 44:         TestFramework.runWithFlags("-XX:+UseLoopPredicate",
>> 45:                                    "-XX:+UseProfiledLoopPredicate");
>> 46:         TestFramework.runWithFlags("-XX:-UseLoopPredicate");
> 
> You could also add a run where you only disable `-XX:-UseProfiledLoopPredicate` for completeness and add an IR rule accordingly.

Implemented in [7f24c87](https://github.com/openjdk/jdk/pull/24248/commits/7f24c87557da33bbb96a3596222b4737a06d9d31)

> test/hotspot/jtreg/compiler/predicates/TestDisabledLoopPredicates.java line 74:
> 
>> 72:     @IR(counts = { IRNode.PARSE_PREDICATE_LOOP, "=1",
>> 73:                    IRNode.PARSE_PREDICATE_PROFILED_LOOP, "1" },
>> 74:         phase = CompilePhase.AFTER_PARSING,
> 
> `phase` is not required since you've decided that `AFTER_PARSING` is the default phase where we match this node on. You only need to specify `phase` if you want to match on a different phase.

Implemented in [7f24c87](https://github.com/openjdk/jdk/pull/24248/commits/7f24c87557da33bbb96a3596222b4737a06d9d31)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2016037388
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2016040418
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2016039828
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2016039142
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2016037918
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2016038370