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

[Christian Hagedorn]

On Wed, 26 Mar 2025 14:34:47 GMT, Manuel Hässig <duke at openjdk.org> wrote:

>> # Issue Summary
>> 
>> When running with `-XX:-UseLoopPredicate` C2 still inserts profiled loop parse predicates, despite those being a form of loop parse predicate. Further, the loop predicate code is not always consistent when to insert/expect profiled parse predicates.
>> 
>> # Change Summary
>> 
>> Following the rationale, that profiled predicates are a subset of loop predicates, this PR disables profiled predicates whenever loop predicates are disabled. They are disabled on the level of arguments. Further, before any checks for whether profiled predicates are enabled, this PR inserts a check that loop predicates are enabled such that the code is consistent in its intention.
>> 
>> Concretel, this PR
>>  - adds parse predicate nodes to the IR testing framework,
>>  - turns off `UseProfiledLoopPredicate` if `UseLoopPredicate` is turned off,
>>  - predicates all checks for `UseProfiledLoopPredicate`on `UseLoopPredicate` first for consistency,
>>  - adds a regression test.
>> 
>> 
>> # Testing
>>  
>> The changes passed the following testing:
>>  - [Github Actions](https://github.com/mhaessig/jdk/actions/runs/14078750038)
>>  - tier1 through tier3 and Oracle internal testing
>
> Manuel Hässig has updated the pull request incrementally with one additional commit since the last revision:
> 
>   ir-framework: rename new nodes to convention

A few comments but overall it looks good. Thanks for cleaning that up!

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 [...]

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");
  }
}

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

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 2763:

> 2761:         IR_NODE_MAPPINGS.put(irNodePlaceholder, new SinglePhaseRangeEntry(CompilePhase.AFTER_PARSING, regex,
> 2762:                                                                           CompilePhase.AFTER_PARSING,
> 2763:                                                                           CompilePhase.CCP1));

I think the legal last phase should be `CompilePhase.PHASEIDEALLOOP_ITERATIONS` where we could observe `ParsePredicates`:
Suggestion:

                                                                          CompilePhase.PHASEIDEALLOOP_ITERATIONS));

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.

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 completness and add an IR rule accorndingly.

test/hotspot/jtreg/compiler/predicates/TestDisabledLoopPredicates.java line 49:

> 47:     }
> 48: 
> 49:     @Run(test = { "test" })

Braces are not required here:

Suggestion:

    @Run(test = "test")

test/hotspot/jtreg/compiler/predicates/TestDisabledLoopPredicates.java line 72:

> 70: 
> 71:     @Test
> 72:     @IR(counts = { IRNode.PARSE_PREDICATE_LOOP, "=1",

The `=` is not required:
Suggestion:

    @IR(counts = { IRNode.PARSE_PREDICATE_LOOP, "1",

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.

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

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24248#pullrequestreview-2717534252
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014385700
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014346920
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014355627
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014361300
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014364335
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014371293
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014363733
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014364939
PR Review Comment: https://git.openjdk.org/jdk/pull/24248#discussion_r2014366623