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

[Tobias Hartmann]

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

Looks reasonable to me otherwise.

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

> 2116: class BFSActions : public StackObj {
> 2117:  public:
> 2118:   // Should a node's inputs further be visit in the BFS traversal? By default, we visit all data inputs. Override this

Suggestion:

  // Should a node's inputs further be visited in the BFS traversal? By default, we visit all data inputs. Override this

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

> 2117:  public:
> 2118:   // Should a node's inputs further be visit in the BFS traversal? By default, we visit all data inputs. Override this
> 2119:   // method to provide a costum filter.

Suggestion:

  // method to provide a custom filter.

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

> 2124: 
> 2125:   // Is the visited node a target node that we are looking for in the BFS traversal? We do not visit its inputs further
> 2126:   // but the BFS will continue to visited all unvisited nodes in the queue.

Suggestion:

  // but the BFS will continue to visit all unvisited nodes in the queue.

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

> 241:   void target_node_action(Node* target_node) override {
> 242:     if (target_node->is_OpaqueLoopInit()) {
> 243:       assert(!_found_init, "can only found one OpaqueLoopInitNode");

Suggestion:

      assert(!_found_init, "should only find one OpaqueLoopInitNode");

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

> 245:     } else {
> 246:       assert(target_node->is_OpaqueLoopStride(), "unexpected Opaque1 node");
> 247:       assert(!_found_stride, "can only found one OpaqueLoopStrideNode");

Suggestion:

      assert(!_found_stride, "should only find one OpaqueLoopStrideNode");

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22136#pullrequestreview-2448370726
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1850160256
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1850160472
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1850161016
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1850165293
PR Review Comment: https://git.openjdk.org/jdk/pull/22136#discussion_r1850166032