RFR: 8344213: Cleanup OpaqueLoop*Node verification code for Assertion Predicates

[Christian Hagedorn]

On Fri, 15 Nov 2024 08:17:22 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This patch cleans up the `OpaqueLoop*Node` verification code that is called with `PhaseIdeaLoop::assertion_predicate_has_loop_opaque_node()`.
> 
> There are some places where the verification code is
> - missing
> - called twice in row with different methods
> - unnecessarily called
> 
> This patch cleans this up and moves the verification code inside the `TemplateAssertionPredicate` and the `InitializedAssertionPredicate` class.
> 
> #### Details of this Patch
> - Doing a simpler BFS similar to what `ReplaceOpaqueStrideInput::replace()` is doing.
> - Noticed that the new code looks very similar, so I decided to create a dedicated `DataNodeBFS` class which could be reused again in the future to perform a BFS on data nodes.
>   - One can implement the new `BFSActions` interface to define
>     - Whether a node's input should be further visited.
>     - Whether a node is a target node for this BFS.
>     - What action that should be performed with the target node.
>   - Updated `ReplaceOpaqueStrideInput` to use the new `DataNodeBFS/BFSActions` classes.
>   - Implemented a new `OpaqueLoopNodesVerifier` class using `DataNodeBFS/BFSActions` which does the `OpaqueLoop*Node` verification previously done with `assertion_predicate_has_loop_opaque_node()`:
>     - Verify Template Assertion Predicates:
>       - For init value: Only `OpaqueLoopInit`
>       - For last value: Both `OpaqueLoop*Nodes`
>     - Verify Initialized Assertion Predicates:
>       - No `OpaqueLoop*Nodes`
> 
> Thanks,
> Christian

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

> 350:   IfTrueNode* template_success_proj = template_assertion_predicate.clone(unswitched_loop_parse_predicate->in(0), this);
> 351:   assert(assertion_predicate_has_loop_opaque_node(template_success_proj->in(0)->as_If()),
> 352:          "must find Assertion Predicate for fast loop");

Verification done in `TemplateAssertionPredicate::clone()`.

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

> 1375: // Create an Initialized Assertion Predicate from the template_assertion_predicate
> 1376: IfTrueNode* PhaseIdealLoop::create_initialized_assertion_predicate(IfNode* template_assertion_predicate, Node* new_init,
> 1377:                                                                    Node* new_stride, Node* new_control) {

Inlined method in last usage into `CreateAssertionPredicatesVisitor::initialize_from_template()`.

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

> 2762:                                                                         scale_con, int_offset, int_limit,
> 2763:                                                                         AssertionPredicateType::FinalIv);
> 2764:             assert(!assertion_predicate_has_loop_opaque_node(loop_entry->in(0)->as_If()), "unexpected");

Verification done in `InitializedAssertionPredicateCreator::create()`.

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

> 2770:                                                                                  this);
> 2771:           loop_entry = template_assertion_predicate_creator.create(loop_entry);
> 2772:           assert(assertion_predicate_has_loop_opaque_node(loop_entry->in(0)->as_If()), "unexpected");

Verification done in `TemplateAssertionPredicateCreator::create()`.

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

> 2776:                                                                       int_offset, int_limit,
> 2777:                                                                       AssertionPredicateType::InitValue);
> 2778:           assert(!assertion_predicate_has_loop_opaque_node(loop_entry->in(0)->as_If()), "unexpected");

Verification done in `InitializedAssertionPredicateCreator::create()`.

src/hotspot/share/opto/loopopts.cpp line 792:

> 790:   if (bol->is_OpaqueTemplateAssertionPredicate()) {
> 791:     // Ignore Template Assertion Predicates with OpaqueTemplateAssertionPredicate nodes.
> 792:     assert(assertion_predicate_has_loop_opaque_node(iff), "must find OpaqueLoop* nodes");

I don't think we are required to do this verification here since we are now always verifying these nodes when creating and cloning them. Here we just want to bail out if we find a Template Assertion Predicate.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1843362547
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1843363553
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1843363983
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1843364150
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1843364422
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1843366157