RFR: 8344035: Replace predicate walking code in Loop Unswitching with a predicate visitor

[Christian Hagedorn]

On Thu, 19 Dec 2024 13:56:41 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This patch is a follow up to the clean-ups done with [JDK-8342945](https://bugs.openjdk.org/browse/JDK-8342945) and introduces a new predicate visitor for Loop Unswitching to update the last remaining custom predicate cloning code.
> 
> This patch includes the following:
> 
> - New `CloneUnswitchedLoopPredicatesVisitor` class which delegates the cloning work to a new `ClonePredicateToTargetLoop` class.
>   - We walk the predicate chain in the `PredicateIterator` and call the `CloneUnswitchedLoopPredicatesVisitor` for each visited predicate. Then we clone the predicate on the fly to the target loop.
> - New `ClonePredicateToTargetLoop` class:
>   - Clones Parse Predicates
>   - Clones Template Assertion Predicates
>     - Includes rewiring of control dependent data nodes
>   - Rewires the cloned predicates to the target loop with new `TargetLoopPredicateChain` class:
>     - Keeps track of the current chain head, which is the target loop itself when the chain is still empty.
>     - Each time a new predicate is inserted at the target loop, the old predicate chain head is set as output of the new predicate.
>     - An example is shown as class comment at `TargetLoopPredicateChain`.
>     - I plan to reuse this class later again when also updating `CreateAssertionPredicatesVisitor` which is done when we tackle the actual still remaining Assertion Predicate bugs.
> - Removal of custom predicate cloning code found in `PhaseIdealLoop`.
> - Changed steps performed in Loop Unswitching from:
>   1. Clone loop
>   2. Clone predicates and insert them below the unswitched loop selector If projections
>   3. Connect the cloned predicates to the unswitched loops
> 
>   to:
> 
>   1. Clone loop
>   2. Connect unswitched loop selector If projections to unswitched loops such that they are now the new loop entries
>   3. Clone predicates and insert them between the unswitched loop selector If projections and the unswitched loops
> - Rename/update `get_template_assertion_predicates()`/`TemplateAssertionPredicateCollector` to reflect the only use left.
> 
> Thanks,
> Christian

src/hotspot/share/opto/loopPredicate.cpp line 103:

> 101: IfTrueNode* PhaseIdealLoop::create_new_if_for_predicate(ParsePredicateSuccessProj* parse_predicate_success_proj,
> 102:                                                         Node* new_entry, const Deoptimization::DeoptReason reason,
> 103:                                                         const int opcode, const bool rewire_uncommon_proj_phi_inputs) {

Squeezed this in here: `assertion_predicate_type` is dead.

src/hotspot/share/opto/loopPredicate.cpp line 274:

> 272: // Put all OpaqueTemplateAssertionPredicate nodes on a list, starting at 'predicate' and going up in the tree.
> 273: void PhaseIdealLoop::get_opaque_template_assertion_predicate_nodes(ParsePredicateSuccessProj* parse_predicate_proj,
> 274:                                                                    Unique_Node_List& list) {

Only applied a renaming here.

src/hotspot/share/opto/loopUnswitch.cpp line 250:

> 248: 
> 249:     _phase->recompute_dom_depth();
> 250:     remove_unswitch_candidate_from_loops(unswitched_loop_selector);

New order of operation (see PR description).

src/hotspot/share/opto/loopUnswitch.cpp line 279:

> 277:     Node* source_loop_entry = unswitched_loop_selector.selector()->in(0);
> 278:     PredicateIterator predicate_iterator(source_loop_entry);
> 279:     predicate_iterator.for_each(clone_unswitched_loop_predicates_visitor);

The main part that is new here: We use visitors to clone the predicates and rewire them correctly.

src/hotspot/share/opto/loopUnswitch.cpp line 300:

> 298:   Node* old_to_new(const Node* old) const {
> 299:     return _old_new[old->_idx];
> 300:   }

Only moved.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22828#discussion_r1892182197
PR Review Comment: https://git.openjdk.org/jdk/pull/22828#discussion_r1892183371
PR Review Comment: https://git.openjdk.org/jdk/pull/22828#discussion_r1892184902
PR Review Comment: https://git.openjdk.org/jdk/pull/22828#discussion_r1892186743
PR Review Comment: https://git.openjdk.org/jdk/pull/22828#discussion_r1892187103