RFR: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor

Thu Mar 13 06:55:05 UTC 2025

On Wed, 12 Mar 2025 16:18:53 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This patch cleans the Parse and Template Assertion Predicate elimination code up. We now use a single `PredicateVisitor` and share code in a new `EliminateUselessPredicates` class which contains the code previously found in `PhaseIdealLoop::eliminate_useless_predicates()`.
> 
> ### Unified Logic to Clean Up Parse and Template Assertion Predicates
> We now use the following algorithm:
> https://github.com/openjdk/jdk/blob/5e4b6ca0ddafa80eee60690caacd257b74305d4e/src/hotspot/share/opto/predicates.cpp#L1174-L1179
> 
> This is different from the old algorithm where we used a single boolean state `_useless`. But that does no longer work because when we first mark Template Assertion Predicates useless, we are no longer visiting them when iterating through predicates:
> 
> https://github.com/openjdk/jdk/blob/a21fa463c4f8d067c18c09a072f3cdfa772aea5e/src/hotspot/share/opto/predicates.hpp#L704-L708
> 
> We therefore require a third state. Thus, I introduced a new tri-state `PredicateState` that provides a special `MaybeUseful` value which we can set each Predicate to.
> 
> #### Ignoring Useless Parse Predicates
> While working on this patch, I've noticed that we are always visiting Parse Predicates - even when they useless. We should change that to align it with what we have for the other Predicates (changed in JDK-8351280). To make this work, we also replace the `_useless` state in `ParsePredicateNode` with a new `PredicateState`.
> 
> #### Sharing Code for Parse and Template Assertion Predicates
> With all the mentioned changes in place, I could nicely share code for the elimination of Parse and Template Assertion Predicates in `EliminateUselessPredicates` by using templates. The following additional changes were required:
> 
> - Changing the template parameter of `_template_assertion_predicate_opaques` to the more specific `OpaqueTemplateAssertionPredicateNode` type.
> - Adding accessor methods to get the Predicate lists from `Compile`.
> - Updating `ParsePredicate::mark_useless()` to pass in `PhaseIterGVN`, as done for Assertion Predicates
> 
> Note that we still do not directly replace the useless Predicates but rather mark them useless as initiated by JDK-8351280.
> 
> ### Other Included Changes
> - During the various refactoring steps, I somehow dropped the code to add newly cloned Template Assertion Predicate to the `_template_assertion_predicate_opaques` list. It was done directly in the old cloning methods. This is not relevant for correctness but could hinder some optimizations. I've added the code now i...

src/hotspot/share/opto/cfgnode.hpp line 508:

> 506:   void mark_maybe_useful();
> 507:   bool is_useful() const;
> 508:   void mark_useful();

Needed to move these definitions to the source file because I cannot include `predicates.hpp` here due to circular dependencies. I solved this by forward declaring `PredicateState` and moving the definitions to the source file.

Same for these methods for `OpaqueTemplateAssertionPredicate` further down.

src/hotspot/share/opto/ifnode.cpp line 2220:

> 2218: const Type* ParsePredicateNode::Value(PhaseGVN* phase) const {
> 2219:   assert(_predicate_state != PredicateState::MaybeUseful, "should only be MaybeUseful when eliminating useless "
> 2220:                                                           "predicates during loop opts");

Best effort assert to ensure we are not seeing `MaybeUseful` anywhere else except during Predicate elimination. Same for `OpaqueTemplateAssertionPredicate`.

src/hotspot/share/opto/ifnode.cpp line 2249:

> 2247:       fatal("unknown kind");
> 2248:   }
> 2249:   if (_predicate_state == PredicateState::Useless) {

I only print `useless` since `MaybeUseful` is only set for a very brief moment and should normally not be visible when dumping/in IGV dumps.

src/hotspot/share/opto/node.cpp line 516:

> 514:   if (n->is_OpaqueTemplateAssertionPredicate()) {
> 515:     C->add_template_assertion_predicate_opaque(n->as_OpaqueTemplateAssertionPredicate());
> 516:   }

See PR description "Other Included Changes".

src/hotspot/share/opto/opaquenode.cpp line 115:

> 113: }
> 114: 
> 115: OpaqueTemplateAssertionPredicateNode::OpaqueTemplateAssertionPredicateNode(BoolNode* bol): Node(nullptr, bol),

Moved some methods to source file - see comment above for `ParsePredicateNode`.

src/hotspot/share/opto/opaquenode.hpp line 30:

> 28: #include "opto/node.hpp"
> 29: #include "opto/subnode.hpp"
> 30: 

Noticed that `opcodes.hpp` was unused and `subnode.hpp` missed the `opto` prefix. Fixed here as well.

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

> 175:   bool is_template_assertion_predicate = if_node->in(1)->is_OpaqueTemplateAssertionPredicate();
> 176:   assert(!is_template_assertion_predicate || AssertionPredicate::has_halt(maybe_success_proj->as_IfTrue()),
> 177:          "Template Assertion Predicate must have a Halt Node on the failing path");

New verification - see "Other Included Changes" in PR description.

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

> 331:   // of the Initialized Assertion Predicate (part of the loop body) while the OpaqueInitializedAssertionPredicate is not
> 332:   // cloned because it's outside the loop body. We end up sharing the OpaqueInitializedAssertionPredicate between the
> 333:   // original and the cloned If. This should be fine.

Reason for not reusing `AssertionPredicate::has_halt()`. I will revisit the `AssertionPredicate` class in a future PR, so I have not tried to unify these methods in a way.

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

> 329:   // the ParsePredicateNode is not marked useless.
> 330:   bool is_valid() const {
> 331:     return _parse_predicate_node != nullptr && !_parse_predicate_node->is_useless();

Avoids visiting useless Parse Predicates during Predicate iteration.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992876906
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992878757
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992879213
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992879809
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992882191
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992882542
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992881495
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992881891
PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1992884443