RFR: 8346552: C2: Add IR tests to check that Predicate cloning in Loop Unswitching works as expected [v4]
Christian Hagedorn
chagedorn at openjdk.org
Tue Apr 22 11:43:51 UTC 2025
On Thu, 17 Apr 2025 06:23:33 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.
>> Working on this PR revealed that Loop Limit Check Parse Predicates are erroneously cloned when unswitching counted loops. That is because we know that the loop variable increments monotonously in counted loops, so a loop limit check at the loop selector is sufficient for both unswitched loops. However, for uncounted loops we do not know anything about the behavior of the loop variables and they could behave differently in either of the unswitched loops. Hence, cloning the loop limit check is needed in that case. This PR also removes the superfluous cloning.
>>
>> All changes summarized:
>> - 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,
>> - only clone loop limit checks when unswitching uncounted loops,
>> - add a test which checks that loop limit checks are not cloned when unswitching counted loops,
>> - add a test which checks that loop limit checks are cloned when unswitching uncounted 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 pull request now contains 13 commits:
>
> - Make test random
> - Add test for predicate cloning on uncounted loops
> - Add test case for predication before unswitching
> - Test predicate cloning only before loop predication
>
> Thus, we do not see the predicates in the loop selector that cloning
> actually killed.
> - Clone loop limit predicates for uncounted loops
>
> When unswitching uncounted loops we have to clone loop limit checks
> because we do not have information on the behavior of the loop index
> - Do not clone loop limit checks in loop unswitching
> - Add suggested comment
>
> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
> - Remove -Xcomp and replace with Warmup(0)
> - ir-framework: use new before/after loop opts phases
> - Add IR test for predicate cloning
> - ... and 3 more: https://git.openjdk.org/jdk/compare/465c8e65...17b32db4
Thanks for the update and fixing the found cloning bug! A few mostly minor comments, otherwise, it looks good!
src/hotspot/share/opto/predicates.cpp line 1103:
> 1101: // Does not clone Loop Limit Check parse predicates iff a counted loop is unswitched, because a loop limit check before
> 1102: // the unswitched loop selector covers both unswitched counted loops. Otherwise, we would need to hoist the loop limit
> 1103: // checks from both loops back up to the loop selector.
I suggest the following rephrasing to highlight why cloning the Parse Predicate is not useful (note that Loop Unswitching does not clone the actual Loop Limit Check but rather the Parse Predicate which is a placeholder for a potential Loop Limit Check later).
Suggestion:
// Does not clone a Loop Limit Check Parse Predicate if a counted loop is unswitched, because it most likely will not be used anymore (it could only be used when both unswitched loop versions die and the Loop Limit Check Parse Predicate ends up at a LoopNode without Loop Limit Check Parse Predicate directly following the unswitched loop that can then be speculatively converted to a counted loop - this is rather rare).
src/hotspot/share/opto/predicates.hpp line 1179:
> 1177:
> 1178: PhaseIdealLoop* const _phase;
> 1179: bool const _is_counted_loop;
We usually have them swapped for constants:
Suggestion:
const bool _is_counted_loop;
test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 2826:
> 2824: /**
> 2825: * Apply {@code regex} on all ideal graph phases starting from {@link CompilePhase#BEFORE_LOOP_OPTS}
> 2826: * up to and including {@link CompilePhase#AFTER_LOOP_OPTS}
Suggestion:
* up to and including {@link CompilePhase#AFTER_LOOP_OPTS}.
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 68:
> 66: arr[i] = i;
> 67: }
> 68:
Suggestion:
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 81:
> 79: IRNode.AUTO_VECTORIZATION_CHECK_PARSE_PREDICATE, "3" },
> 80: phase = CompilePhase.BEFORE_LOOP_UNSWITCHING)
> 81: // Since we know that predication happens after unswitching, we can test the
I suggest to capitalized the optimization and predicate names: Loop Unswitching, Loop Predication, Template Assertion Predicate etc. to better highlight them.
Suggestion:
// Since we know that Loop Predication happens after Loop Unswitching, we can test the
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 82:
> 80: phase = CompilePhase.BEFORE_LOOP_UNSWITCHING)
> 81: // Since we know that predication happens after unswitching, we can test the
> 82: // predicate cloning before predication, such that the useless, killed predicates
Suggestion:
// predicate cloning before Loop Predication, such that the useless, killed predicates
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 88:
> 86: IRNode.LOOP_LIMIT_CHECK_PARSE_PREDICATE, "3",
> 87: IRNode.AUTO_VECTORIZATION_CHECK_PARSE_PREDICATE, "4" },
> 88: phase = CompilePhase.BEFORE_LOOP_PREDICATION_IC)
I suggest to match on RC to have matching before/after RC pairs
Suggestion:
phase = CompilePhase.BEFORE_LOOP_PREDICATION_RC)
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 89:
> 87: IRNode.AUTO_VECTORIZATION_CHECK_PARSE_PREDICATE, "4" },
> 88: phase = CompilePhase.BEFORE_LOOP_PREDICATION_IC)
> 89: // Check that opaque template assertion predicated are added in loop predication
Suggestion:
// Check that Template Assertion Predicates are added in Loop Predication
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 90:
> 88: phase = CompilePhase.BEFORE_LOOP_PREDICATION_IC)
> 89: // Check that opaque template assertion predicated are added in loop predication
> 90: // even if loop predication only happens after loop unswitching.
Suggestion:
// even if Loop Predication only happens after Loop Unswitching.
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 123:
> 121: @Test
> 122: // Check that Loop Unswitching doubled the number of parse and tempalte
> 123: // assertion predicates. Again, the Loop Limit Check Parse Predicate
Suggestion:
// Check that Loop Unswitching doubled the number of Parse and Template
// Assertion Predicates. Again, the Loop Limit Check Parse Predicate
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 133:
> 131: IRNode.AUTO_VECTORIZATION_CHECK_PARSE_PREDICATE, "1" },
> 132: phase = CompilePhase.BEFORE_LOOP_UNSWITCHING)
> 133: // After loop unswitching and after removing the killed predicates.
Suggestion:
// After Loop Unswitching and after removing the killed predicates.
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 139:
> 137: IRNode.LOOP_LIMIT_CHECK_PARSE_PREDICATE, "1",
> 138: IRNode.AUTO_VECTORIZATION_CHECK_PARSE_PREDICATE, "2" },
> 139: phase = CompilePhase.BEFORE_LOOP_PREDICATION_IC)
This suggests that we apply another round of Loop Predication later? It might be confused with the first application of Loop Predication. Could we also match on `PHASEIDEALLOOP_ITERATIONS` instead? Would that work?
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 154:
> 152: @Test
> 153: // Check that Loop Unswitching doubled the number of all parse predicates.
> 154: // Since this is not counted loop, the Loop Limit Check parse predicate
Suggestion:
// Check that Loop Unswitching doubled the number of all Parse Predicates.
// Since this is not counted loop, the Loop Limit Check Parse Predicate
test/hotspot/jtreg/compiler/loopopts/TestUnswitchPredicateCloning.java line 162:
> 160: phase = CompilePhase.BEFORE_LOOP_UNSWITCHING)
> 161: // After loop unswitching and after removing the killed predicates all
> 162: // parse predicates are doubled..
Suggestion:
// After Loop Unswitching and after removing the killed predicates all
// Parse Predicates are doubled.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24479#pullrequestreview-2783752093
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053917190
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053899717
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053901193
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053921318
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053922091
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053922438
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053926155
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053924834
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053925405
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053927678
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053932438
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053935490
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053930780
PR Review Comment: https://git.openjdk.org/jdk/pull/24479#discussion_r2053936490
More information about the hotspot-compiler-dev
mailing list