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

[Christian Hagedorn]

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

Thanks Roland for having a look. I agree that it is indeed quite fragile. It started out with a quite simple fix but then I found more and more cases with fuzzing where we have some weird in-between states in IGVN while a predicate is being folded where matching failed. I was not super happy with matching predicates during IGVN which is difficult and error-prone to get right.

>  So I'm wondering if there would be a way to mark predicates as being for a particular loop (maybe storing the loop's node id they apply to in predicate nodes and making sure it's properly updated as loops are cloned etc.) so when there is a mismatch between the loop and predicate it can be detected?

That's an interesting idea that could work more reliably. Let me think about that more.

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

PR Comment: https://git.openjdk.org/jdk/pull/23823#issuecomment-2689007300