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

Wed Mar 19 14:36:30 UTC 2025

On Wed, 19 Mar 2025 14:33:32 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...
>
> Christian Hagedorn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Small things
>  - Fix test comments
>  - New approach: Marking unrelated Template Assertion Predicates outside of IGVN by storing a reference to the loop it originally was created for.
>  - Merge branch 'master' into JDK-8350579
>  - Revert fix completely
>  - 8350579: Remove Template Assertion Predicates belonging to a
>     loop once it is folded away during IGVN

src/hotspot/share/opto/loopTransform.cpp line 1703:

> 1701: // to the new stride.
> 1702: void PhaseIdealLoop::update_main_loop_assertion_predicates(CountedLoopNode* main_loop_head) {
> 1703:   Node* init = main_loop_head->init_trip();

unused

src/hotspot/share/opto/loopTransform.cpp line 2025:

> 2023:   loop_head->clear_strip_mined();
> 2024: 
> 2025:   update_main_loop_assertion_predicates(clone_head, stride_con);

Moved down here (see PR description).

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

> 180: 
> 181: // Clone this Template Assertion Predicate without modifying any OpaqueLoop*Node inputs.
> 182: TemplateAssertionPredicate TemplateAssertionPredicate::clone(Node* new_control, CountedLoopNode* new_loop_node,

Most changes in this file: Changing `phase` -> `_phase` and piping the new `CountedLoopNode` through the code such that we can initialized the new `OpaqueTemplateAssertionPredicate` nodes with them accordingly.

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

> 216: // This class is used to replace the input to OpaqueLoopStrideNode with a new node while leaving the other nodes
> 217: // unchanged.
> 218: class ReplaceOpaqueStrideInput : public BFSActions {

Moved this up because we now call it from `TemplateAssertionPredicate` and not `TempalteAssertionPredicateExpression`.

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

> 216:   DEBUG_ONLY(verify();)
> 217:   TemplateAssertionExpression expression(opaque_node());
> 218:   expression.replace_opaque_stride_input(new_stride, igvn);

Was just an indirection which I removed which makes it easier to use.

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

> 578:       new OpaqueTemplateAssertionPredicateNode(bool_into_opaque_node_clone, new_loop_node);
> 579:   _phase->C->add_template_assertion_predicate_opaque(opaque_clone);
> 580:   _phase->register_new_node(opaque_clone, new_control);

We don't clone the `OpaqueTemplateAssertionPredicateNode` anymore with `DataNodeGraph::clone_with_opaque_loop_transform_strategy` but directly here. This allows us to easily set the `new_loop_node` for it.

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

> 1099:     const TemplateAssertionPredicate& template_assertion_predicate) {
> 1100:   TemplateAssertionPredicate cloned_template_assertion_predicate =
> 1101:       template_assertion_predicate.clone(_old_target_loop_entry, _target_loop_head->as_CountedLoop(), _phase);

Moved method from `.hpp` file here because of incomplete `CountedLoopNode` when calling `as_CountedLoop()`.

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

> 1149:   }
> 1150:   replace_opaque_stride_input(template_assertion_predicate);
> 1151:   template_assertion_predicate.update_associated_loop_node(_loop_node);

We don't need to clone the Template Assertion Expression and hence we only need to update the loop node.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003442970
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003443681
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003456825
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003450551
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003451847
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003460961
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003476267
PR Review Comment: https://git.openjdk.org/jdk/pull/23823#discussion_r2003480253