RFR: 8341977: Replace predicate walking and cloning code for Loop Peeling with a predicate visitor [v2]

[Emanuel Peter]

On Thu, 24 Oct 2024 11:57:39 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> #### Replacing the Remaining Predicate Walking and Cloning Code 
>> In the next series of patches (this, [JDK-8342943](https://bugs.openjdk.org/browse/JDK-8342943), [JDK-8342945](https://bugs.openjdk.org/browse/JDK-8342945), and [JDK-8342946](https://bugs.openjdk.org/browse/JDK-8342946)) I want to replace the predicate walking and cloning code used for Loop Peeling, Pre/Main/Post Loops, Loop Unswitching, removing useless Assertion Predicates and Loop Unrolling with new `PredicateVisitors` which can be used in combination with the new `PredicateIterator`.
>> 
>> #### 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 (see **P1** PR comment).
>> - Keep the semantics which includes to only apply the Template Assertion Predicate processing if there are Parse Predicates (see **P2** PR comment). This limitation should eventually be removed. But I want to do that separately at a later point.
>> 
>> #### Refactorings of this Patch
>> This first patch replaces the predicate walking and cloning code for Loop Peeling and lays the foundation for the replacement for main/post loops ([JDK-8342943](https://bugs.openjdk.org/browse/JDK-8342...
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   small update

Generally looks reasonable. A little hard to review without digging much deeper into the code. I have some questions/suggestions already.

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

> 817:   if (counted_loop && UseLoopPredicate) {
> 818:       initialize_assertion_predicates_for_peeled_loop(new_head->as_CountedLoop(), head->as_CountedLoop(),
> 819:                                                       first_node_index_in_cloned_loop_body, old_new);

Suggestion:

    initialize_assertion_predicates_for_peeled_loop(new_head->as_CountedLoop(), head->as_CountedLoop(),
                                                    first_node_index_in_cloned_loop_body, old_new);

Indentation seems off.

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

> 1987:   Node* source_loop_entry = source_loop_head->skip_strip_mined()->in(LoopNode::EntryControl);
> 1988:   PredicateIterator predicate_iterator(source_loop_entry);
> 1989:   predicate_iterator.for_each(assertion_predicates_for_loop);

The name `assertion_predicates_for_loop` does not tell me what this would do, when applied with the `for_each`

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

> 734:   if (deopt_reason == Deoptimization::Reason_predicate ||
> 735:       deopt_reason == Deoptimization::Reason_profile_predicate) {
> 736:     _current_parse_predicate = parse_predicate.tail();

We set it here. But do we need to unset it again for later predicate blocks?

src/hotspot/share/opto/predicates.hpp line 946:

> 944: // Visitor to create Initialized Assertion Predicates at a target loop from Template Assertion Predicates from a source
> 945: // loop. This visitor can be used in combination with a PredicateIterator.
> 946: class AssertionPredicatesForLoop : public PredicateVisitor {

I think this could have a more expressive name. It is a Visitor... hmm
Maybe `InitializedAssertionPredicatesFromTemplatesCreator`? Hmm not sure.

The current name suggests that it is just a collection of `AssertionPredicates`.

src/hotspot/share/opto/predicates.hpp line 952:

> 950:   Node* _new_control;
> 951:   PhaseIdealLoop* const _phase;
> 952:   ParsePredicateSuccessProj* _current_parse_predicate;

It looks to me like this could be a boolean, correct?

src/hotspot/share/opto/predicates.hpp line 967:

> 965:   NONCOPYABLE(AssertionPredicatesForLoop);
> 966: 
> 967:   using PredicateVisitor::visit;

What does this do?

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

PR Review: https://git.openjdk.org/jdk/pull/21679#pullrequestreview-2398539538
PR Review Comment: https://git.openjdk.org/jdk/pull/21679#discussion_r1818725177
PR Review Comment: https://git.openjdk.org/jdk/pull/21679#discussion_r1818781439
PR Review Comment: https://git.openjdk.org/jdk/pull/21679#discussion_r1818772147
PR Review Comment: https://git.openjdk.org/jdk/pull/21679#discussion_r1818760936
PR Review Comment: https://git.openjdk.org/jdk/pull/21679#discussion_r1818767972
PR Review Comment: https://git.openjdk.org/jdk/pull/21679#discussion_r1818745986