RFR: 8360510: C2: Template Assertion Predicates are not cloned to the inner counted loop with -XX:+StressDuplicateBackedge

Tue Nov 25 10:50:28 UTC 2025

On Wed, 19 Nov 2025 12:19:02 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> ### Strong Connection between Template Assertion Predicate and Counted Loop 
> In [JDK-8350579](https://bugs.openjdk.org/browse/JDK-8350579), we fixed the issue that a Template Assertion Predicate for a folded loop A could end up at another loop B. We then created an Initialized Assertion Predicate at loop B from the template of loop A and used the values from the already folded, completely unrelated loop A . As a result, we crashed with a halt because loop B violated the predicate with the wrong values. As a fix, we established a strong connection between Template Assertion Predicates and their associated loop node by adding a direct link from `OpaqueTemplateAssertionPredicate` -> `CountedLoop`.
> 
> #### Maintaining this Property
> In `PhaseIdealLoop::eliminate_useless_predicates()`, we walk through all counted loops and only keep those `OpaqueTemplateAssertionPredicate` nodes that can be found from the loop heads and are actually meant for this loop (using the strong connection):
> https://github.com/openjdk/jdk/blob/d2926dfd9a242928877d0b1e40eac498073975bd/src/hotspot/share/opto/predicates.cpp#L1245-L1249
> 
> All other opaque nodes are removed.
> 
> ### Additional Verification for Useless `OpaqueTemplateAssertionPredicate` Nodes
> As an additional verification for `OpaqueTemplateAssertionPredicate` nodes that are found to be useless in `eliminate_useless_predicates()`, we check that in this case the `CountedLoop` is really dead (otherwise, we should have found the `OpaqueTemplateAssertionPredicate` in our walks through all loop):
> https://github.com/openjdk/jdk/blob/d2926dfd9a242928877d0b1e40eac498073975bd/src/hotspot/share/opto/predicates.cpp#L1294-L1301
> 
> ### Violating the Additional Verification with `-XX:+StressLoopBackedge`
> In `PhaseIdealLoop::duplicate_loop_backedge()`, we convert a loop with a merge point into two loops which should enable us to transform the new inner loop into a counted loop. This only makes sense for a `Loop` that is not a counted loop, yet. However, to stress the transformation, we can also run with `-XX:+StressDuplicateBackedge` that also transforms a counted loop into an inner and an outer loop. This is a problem when we have Template Assertion Predicates above a counted loop to be stressed:
> 
> <img width="581" height="556" alt="image" src="https://github.com/user-attachments/assets/e7948804-e74d-4dd0-a0f8-64eedaa259ba" />
> 
> After duplicate backedge, the Template Assertion Predicates are now at the outer non-counted `Loop`:
> 
> <img width="567" heig...

Thanks for your continued work on the predicates 😊 

I have some questions below - generally it looks reasonable :)

src/hotspot/share/opto/loopopts.cpp line 4192:

> 4190: 
> 4191: // Clone Template Assertion Predicates to a target loop. The target loop is the original, not-cloned loop.
> 4192: // This is currently only used for StressDuplicateLoopBackedge.

Also from the PR description:

> Solution
>The solution I propose is to clone the Template Assertion Predicates to the inner counted loop. This can be guarded with an ifdef ASSERT because it can only happen with StressLoopBackedge which is a develop flag. This is straight forward and solves this "opaque <-> counted loop" mismatching problem.

This makes me a little nervous, applying a fix only to debug. But maybe I don't have to be nervous, let's see ;)

Can you add some additional justification? What could go wrong in product if we don't have this fix in product?

If I understand right: we just lose the predicates. That's not a correctness issue, we just can't optimize as much. And that's why we have your "Additional Verification". This is not about correctness but only about how much we can optimize, right?

If so: you could consider writing that down a bit more explicitly, and also enhancing the message of the assert.

What do you think?

src/hotspot/share/opto/loopopts.cpp line 4210:

> 4208:   void visit(const TemplateAssertionPredicate& template_assertion_predicate) override {
> 4209:     _clone_predicate_to_loop.clone_template_assertion_predicate(template_assertion_predicate);
> 4210:     template_assertion_predicate.kill(_phase->igvn());

Are we killing the old ones? If so: `clone + kill old` -> `move`. I would suggest calling it `MoveAssertionPredicatesVisitor`

src/hotspot/share/opto/loopopts.cpp line 4496:

> 4494: 
> 4495: #ifdef ASSERT
> 4496:   if (StressDuplicateBackedge && head->is_CountedLoop()) {

Could we somehow add an assert here?

Would this work?
`assert(StressDuplicateBackedge || !head->is_CountedLoop(), "counted loop only expected in stress mode");`

That would give us some additional confidence that this only happens in debug.

But then why limit the fix to debug, and not apply it to product too, just in case the assert fails?

test/hotspot/jtreg/compiler/predicates/assertion/TestAssertionPredicates.java line 150:

> 148:  * @test id=DataUpdateZGC
> 149:  * @key randomness
> 150:  * @bug 8288981 8350577

Suggestion:

 * @bug 8288981 8350577 8360510

Just a suggestion, feel free to ignore if it makes no sense ;)

test/hotspot/jtreg/compiler/predicates/assertion/TestStressDuplicateBackedgeWithAssertionPredicate.java line 28:

> 26:  * @bug 8360510
> 27:  * @summary Test that StressDuplicateBackedge correctly clones Template Assertion Predicates to the inner counted loop.
> 28:  * @run main/othervm -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:+StressDuplicateBackedge

Would it be useful to have a run without any flags?

test/hotspot/jtreg/compiler/predicates/assertion/TestStressDuplicateBackedgeWithAssertionPredicate.java line 62:

> 60:         // The Template Assertion Predicates are still at the outer loop. As a result, we find them to
> 61:         // be useless in the next predicate elimination call with EliminateUselessPredicates because
> 62:         // they cannot be found from the inner counted loop. However, we have verification code in place

First: cudos on the annotations in this test! Really much appreciated :)

I'm a bit confused here. Are you talking about the Template Assertion Predicates of the outer or inner loop here? Because you say both:
- `Template Assertion Predicates are still at the outer loop`
- `they cannot be found from the inner counted loop`
Can you clarify?

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

PR Review: https://git.openjdk.org/jdk/pull/28389#pullrequestreview-3504334956
PR Review Comment: https://git.openjdk.org/jdk/pull/28389#discussion_r2559480567
PR Review Comment: https://git.openjdk.org/jdk/pull/28389#discussion_r2559489861
PR Review Comment: https://git.openjdk.org/jdk/pull/28389#discussion_r2559498552
PR Review Comment: https://git.openjdk.org/jdk/pull/28389#discussion_r2559431674
PR Review Comment: https://git.openjdk.org/jdk/pull/28389#discussion_r2559408515
PR Review Comment: https://git.openjdk.org/jdk/pull/28389#discussion_r2559426922