RFR: 8346552: C2: Add IR tests to check that Parse Predicate cloning in Loop Unswitching works as expected [v2]

[Christian Hagedorn]

On Thu, 10 Apr 2025 11:19:11 GMT, Manuel Hässig <duke at openjdk.org> wrote:

>> # Issue Summary
>> 
>> When a loop is unswitched, all parse predicates from the original loop must be cloned to the second loop that is created. Forgetting to clone a parse predicate is a common error during development on loop unswitching code that we could not catch previously. Since we have the IR-framework now, this PR introduces a test to catch this error.
>> 
>> # Changes
>> 
>> The main contribution of this PR is a test to ensure that all predicates have been cloned into an unswitched loop. This also required some relating changes:
>>  - add `OPAQUE_TEMPLATE_ASSERTION_PREDICATE_NODE` to the IR-framework,
>>  - add some missing parse predicate nodes to the IR-framework,
>>  - change the output of the labels of parse predicate nodes in the ideal graph so they can be recognized reliably by the IR-framework (the main problem was that `Loop ` is a prefix of `Loop Limit Check` that is hard to distinguish with spaces instead of underlines),
>>  - rework the regex for detecting parse predicates in the IR-framework,
>>  - add a test to ensure parse predicates are cloned into unswitched loops.
>> 
>> # Testing
>> 
>> - [Github Actions](https://github.com/mhaessig/jdk/actions/runs/14266369099)
>> - tier1 through tier3 plus Oracle internal testing
>
> Manuel Hässig has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - ir-framework: use new before/after loop opts phases
>  - Merge branch 'master' into JDK-8346552-predicate-cloning
>  - Add IR test for predicate cloning
>  - ir-framework: make the parse predicate node regex more robust
>  - ir-framework: add auto vectorization check node
>  - ir-framework: add opaque template assertion predicate node

Thanks for working on that! A few some suggestions.

test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 43:

> 41: 
> 42:     public static void main(String[] strArr) {
> 43:         TestFramework.runWithFlags("-Xcomp");

Do you really need `-Xcomp` for all compilations? If you only need to have it for the test method, you can add `@Warmup(0)`. In case you would have multiple test methods, you can also use `TestFramework.setDefaultWarmup()`.

test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 64:

> 62:     @Test
> 63:     // Check that loop unswitching the number of parse predicates inside the unswitched
> 64:     // loop have doubled.

Suggestion:

    // Check that Loop Unswitching doubled the number of Parse Predicates: We have them at the true- and false-path-loop. Note that the Loop Limit Check Parse Predicate is not cloned when we already have a counted loop.


While writing this suggestion, I think it would also be good to have the following tests:
- A test where we unswitch a `LoopNode` (i.e. non-counted) to check that the Loop Limit Check Parse Predicate is also cloned to both unswitched loop versions.
- Since you already added some verification for Assertion Predicates as well, I suggest to go a step further. We could add a test where we first apply Loop Predication and then Loop Unswitching to check if the number of Template Assertion Predicates also doubled. Here we need to be careful that we don't miscount. We only mark the old Template Assertion Predicates useless in Loop Unswitching and then clean them up in IGVN. So, we would need to check before loop opts phase `n`: `x` Template Assertion Predicates and then before loop opts phase `n + 1`: `2*x` Template Assertion Predicates. 

You could then update the issue/PR title to:
C2: Add IR tests to check that predicate cloning in Loop Unswitching works as expected.

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

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24479#pullrequestreview-2756445141
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2037222466
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2037202818