RFR: 8350579: Remove Template Assertion Predicates belonging to a loop once it is folded away during IGVN

[Emanuel Peter]

On Thu, 27 Feb 2025 13:07:46 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> The patch fixes the issue of creating an Initialized Assertion Predicate at a loop X from a Template Assertion Predicate that was originally created for a loop Y. Using the unrelated loop values from loop Y for the Initialized Assertion Predicate will let it fail during runtime and we execute a `halt` instruction. This was originally reported with [JDK-8305428](https://bugs.openjdk.org/browse/JDK-8305428).
> 
> Note that most of the line changes are from new tests.
> 
> ### The Problem
> There are multiple test cases triggering the same problem. In the following, when referring to "the test case", I'm referring to `testTemplateAssertionPredicateNotRemovedHalt()` which was written from scratch and contains more detailed comments explaining how we end up with executing a `Halt` node in more details. 
> 
> #### An Inner Loop without Parse Predicates
> The graph in `testTemplateAssertionPredicateNotRemovedHalt()` looks like this after creating `LoopNodes` for the outer `for` and inner `while (true)` loop:
> 
> ![image](https://github.com/user-attachments/assets/7ac60e35-0b7e-4f04-b9dd-6eb8c8654a15) 
> 
> We only have Parse Predicates for the outer loop. Why? 
> 
> Before beautify loop, we have the following region which merges multiple backedges - the one from the `for` loop and the one from the `while (true)` loop:
> 
> ![image](https://github.com/user-attachments/assets/7895161d-5ac1-46d6-93fe-5ab90ef24ab9)
> 
> In `IdealLoopTree::merge_many_backedges()`, we notice that the hottest backedge is hot enough such that it is worth to have a separate merge point region for the inner and outer loop. We set everything up and eventually in  `IdealLoopTree::split_outer_loop()`, we create a second `LoopNode`.
> 
> For this inner `LoopNode`, we cannot set up `Parse Predicates` with the same UCTs as used for the outer loop. It would be incorrect when taking the trap to re-execute the inner and outer loop again while having already executed some of the outer loop's iterations. Thus, we get the graph shape with back-to-back `LoopNodes` as shown above.
>  
> #### Predicates from a Folded Loop End up at Another Loop
> As described in the previous section, we have an inner and outer `LoopNode` while the inner does not have Parse Predicates. In a series of events (see test case comments for more details), we first hoist a range check out of the outer loop during Loop Predication with a Template Assertion Predicate. Then, we fold the outer loop away because we find that it is only running for a single iteration and the bac...

Generally looks good, I have a few suggestions and questions :)

src/hotspot/share/opto/loopnode.cpp line 2517:

> 2515:     KillTemplateAssertionPredicates kill_template_assertion_predicates(phase->is_IterGVN());
> 2516:     PredicateIterator predicate_iterator(skip_strip_mined()->in(EntryControl));
> 2517:     predicate_iterator.for_each(kill_template_assertion_predicates);

Suggestion:

    KillTemplateAssertionPredicateVisitor kill_template_assertion_predicate_visitor(phase->is_IterGVN());
    PredicateIterator predicate_iterator(skip_strip_mined()->in(EntryControl));
    predicate_iterator.for_each(kill_template_assertion_predicate_visitor);

Nit:
`KillTemplateAssertionPredicateVisitor` might be nicer because it tells me from the beginning that it is a visitor.
`KillTemplateAssertionPredicates` had me thinking that is some kind of constructor that goes ahead and kills things already. The plural "predicates" also indicated that it would do that for all predicates.

src/hotspot/share/opto/predicates.cpp line 62:

> 60:     return false;
> 61:   }
> 62:   return has_assertion_predicate_opaque_or_con_input(maybe_success_proj);

Quick control question: Could skipping over constants also mean that we traverse up too far at some point? Like skipping not not just the predicates but also an unrelated check that is about to constant fold? I suppose that should not really create issues?

src/hotspot/share/opto/predicates.hpp line 1230:

> 1228: // The visitor visits all Template Assertion Predicates and kills them by marking them useless. They will be removed
> 1229: // during next round of IGVN.
> 1230: class KillTemplateAssertionPredicates : public PredicateVisitor {

Suggestion:

class KillTemplateAssertionPredicateVisitor : public PredicateVisitor {

src/hotspot/share/opto/predicates.hpp line 1232:

> 1230: class KillTemplateAssertionPredicates : public PredicateVisitor {
> 1231:   PhaseIterGVN* _igvn;
> 1232:   public:

Suggestion:

 public:

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

> 103:  * @test id=NoFlags
> 104:  * @bug 8288981 8350579
> 105:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+AbortVMOnCompilationFailure

Can you explain why you are enabling `AbortVMOnCompilationFailure`?

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

> 157: 
> 158:     // Runs most of the tests except the really time-consuming ones.
> 159:     static void runAllTests() {

Sounds like a bit of a contradiction 😅 

`runAllTests` -> `runAllFastTests`?

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

PR Review: https://git.openjdk.org/jdk/pull/23823#pullrequestreview-2666689146
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r1984724182
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r1984734437
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r1984721363
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r1984740043
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r1984743343
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r1984746128