RFR: 8342943: Replace predicate walking and cloning code for main/post loops with a predicate visitor

Wed Oct 30 15:37:27 UTC 2024

On Wed, 30 Oct 2024 15:18:56 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> #### Replacing the Remaining Predicate Walking and Cloning Code
> The goal is to replace and unify all the remaining custom predicate walking and cloning code currently used for:
> -  <s>[JDK-8341977](https://bugs.openjdk.org/browse/JDK-8341977): Loop Peeling</s> (integrated with https://github.com/openjdk/jdk/pull/21679))
> - [JDK-8342943](https://bugs.openjdk.org/browse/JDK-8342943): Main and Post Loop (this PR)
> - [JDK-8342945](https://bugs.openjdk.org/browse/JDK-8342945): Loop Unswitching and removing useless Assertion Predicate (upcoming)
> - [JDK-8342946](https://bugs.openjdk.org/browse/JDK-8342946): Main and Post Loop (upcoming)
> 
> ---
> (Sections taken over from https://github.com/openjdk/jdk/pull/21679)
> 
> #### Single Template Assertion Predicate Check
> This replacement allows us to have a single `TemplateAssertionPredicate::is_predicate()` check that is called for all predicate matching code. This enables the removal of uncommon traps for Template Assertion Predicates with [JDK-8342047](https://bugs.openjdk.org/browse/JDK-8342047) which is a missing piece in order to fix the remaining problems with Assertion Predicates ([JDK-8288981](https://bugs.openjdk.org/browse/JDK-8288981)).
> 
> #### Common Refactorings for all the Patches in this Series
> In each of the patch, I will do similar refactoring ideas:
> - Replace the existing code in the corresponding `PhaseIdealLoop` method with call to a new (or existing) predicate visitor which extends the `PredicateVisitor` interface. 
> - The visitor implements the Assertion Predicate `visit()` methods to implement the cloning and initialization of the Template Assertion Predicates.
> - The predicate visitor is then passed to the `PredicateIterator` which walks through all predicates found at a loop and applies the visitor for each predicate. 
> - The visitor creates new nodes (if there are Template Assertion Predicates) either in place or at the loop entry of a target loop. In the latter case, the calling code of the `PredicateIterator` must make sure to connect the tail of the newly created predicate chain after the old loop entry to the target loop head.
> - Keep the semantics which includes to only apply the Template Assertion Predicate processing if there are Parse Predicates. This limitation should eventually be removed. But I want to do that separately at a later point.
> ---
> 
> #### Refactorings of this Patch
> This patch replaces the predicate walking and cloning code for **main and post loops**. The code can reuse the code establishe...

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

> 1318: // Assertion Predicates ensures that the main-loop is removed if some type ranges of Cast or Convert nodes become
> 1319: // impossible and are replaced by top (i.e. a sign that the main-loop is dead).
> 1320: void PhaseIdealLoop::copy_assertion_predicates_to_main_loop_helper(const PredicateBlock* predicate_block, Node* init,

Replaced with `PhaseIdealLoop::initialize_assertion_predicates_for_main_loop()`

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

> 1470: // removed after loop opts, these are never executed. We therefore insert a Halt node instead of an uncommon trap.
> 1471: Node* PhaseIdealLoop::clone_template_assertion_predicate(IfNode* iff, Node* new_init, Node* predicate, Node* uncommon_proj,
> 1472:                                                          Node* control, IdealLoopTree* outer_loop, Node* new_control) {

Moved to `TemplateAssertionPredicate::clone_and_replace_init()`.

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

> 1485: }
> 1486: 
> 1487: void PhaseIdealLoop::copy_assertion_predicates_to_main_loop(CountedLoopNode* pre_head, Node* init, Node* stride,

Replaced with `PhaseIdealLoop::initialize_assertion_predicates_for_main_loop()`.

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

> 1850:                                                                    const uint first_node_index_in_cloned_loop_body) {
> 1851:   const NodeInClonedLoopBody node_in_cloned_loop_body(first_node_index_in_cloned_loop_body);
> 1852:   create_assertion_predicates_at_loop(main_loop_head, post_loop_head, node_in_cloned_loop_body, false);

With the already refactored code for Loop Peeling, we can simply reuse `create_assertion_predicates_at_loop()` which simplifies the code a lot.

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

> 1862:   Node* target_loop_entry = target_outer_loop_head->in(LoopNode::EntryControl);
> 1863:   CreateAssertionPredicatesVisitor create_assertion_predicates_visitor(init, stride, target_loop_entry, this,
> 1864:                                                                        _node_in_loop_body, clone_template);

Not a very elegant solution to introduce `clone_template` but I'm planning to update this code again at a later point. This was the least invasive way to handle this.

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

> 1940: // Go over the Assertion Predicates of the main loop and make a copy for the post loop with its initial iv value and
> 1941: // stride as inputs.
> 1942: void PhaseIdealLoop::copy_assertion_predicates_to_post_loop(LoopNode* main_loop_head, CountedLoopNode* post_loop_head,

Replaced with `PhaseIdealLoop::initialize_assertion_predicates_for_post_loop()`.

src/hotspot/share/opto/loopnode.hpp line 947:

> 945:   IfTrueNode* create_initialized_assertion_predicate(IfNode* template_assertion_predicate, Node* new_init,
> 946:                                                      Node* new_stride, Node* control);
> 947:   DEBUG_ONLY(static bool assertion_predicate_has_loop_opaque_node(IfNode* iff);)

Needs to be public since I'm calling it from the `TemplateAssertionPredicate` class. Planning to refactor/move this method at a later point in time.

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

> 762:   }
> 763:   if (_clone_template) {
> 764:     _new_control = clone_template_and_replace_init_input(template_assertion_predicate);

For the main loop, we need to initialize templates **and** clone them to further create Initialized Assertion Predicates from later when unrolling. Note that in the full fix, we should **always** clone templates since we don't know if the loop is going to split further. However, I don't want to change semantics here as this is a simple refactoring patch. I'm addressing these problems in a later PR.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822858323
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822859741
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822861130
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822883179
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822866079
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822861954
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822874539
PR Review Comment: https://git.openjdk.org/jdk/pull/21790#discussion_r1822870022