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

[Christian Hagedorn]

On Thu, 27 Feb 2025 16:46:06 GMT, Roland Westrelin <roland 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...
>
> That looks reasonable to me but imaking sure that predicates in the process of being removed are properly stepped over feels like something that could be fragile. 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?

@rwestrel @eme64 I pushed an updated and added a new section `New Proposed Solution` in  the PR description to explain the changes. I completely reverted the original IGVN based approach and implemented a new one inside `PhaseIdealLoop::eliminate_useless_predicates()`.

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

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