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>

src/hotspot/share/opto/node.hpp line 2139:

> 2137: 
> 2138:  public:
> 2139:   explicit DataNodeBFS(BFSActions& bfs_action) : _bfs_actions(bfs_action) {}

Is this restricted to data-nodes? If so, you should verify that the `start_node` is a data node. But we could also generalize this to any BFS, and then check in `should_visit` if it is a data node of CFG.

You should also say that it traverses inputs/def, not outputs/uses.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1853759705