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

[Emanuel Peter]

On Wed, 20 Nov 2024 11:51:32 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
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Apply suggestions from code review
>   
>   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

Looks reasonable :)

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

> 909:   IfTrueNode* initialized_predicate = initialized_assertion_predicate.create_from_template(template_head,_new_control,
> 910:                                                                                            _init, _stride);
> 911:   DEBUG_ONLY(InitializedAssertionPredicate::verify(initialized_predicate);)

Suggestion:

  InitializedAssertionPredicateCreator initialized_assertion_predicate_creator(_phase);
  IfTrueNode* initialized_predicate = initialized_assertion_predicate_creator.create_from_template(template_head,_new_control,
                                                                                                   _init, _stride);
  DEBUG_ONLY(InitializedAssertionPredicate::verify(initialized_predicate);)

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22136#pullrequestreview-2454198147
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1853766435