8344213: Cleanup OpaqueLoop*Node verification code for Assertion Predicates

Reviewed-by: thartmann, epeter

src/hotspot/share/opto/loopPredicate.cpp

@@ -348,8 +348,6 @@ PhaseIdealLoop::clone_assertion_predicate_for_unswitched_loops(IfTrueNode* templ
                                                                ParsePredicateNode* unswitched_loop_parse_predicate) {
   TemplateAssertionPredicate template_assertion_predicate(template_assertion_predicate_success_proj);
   IfTrueNode* template_success_proj = template_assertion_predicate.clone(unswitched_loop_parse_predicate->in(0), this);
-  assert(assertion_predicate_has_loop_opaque_node(template_success_proj->in(0)->as_If()),
-         "must find Assertion Predicate for fast loop");
   _igvn.replace_input_of(unswitched_loop_parse_predicate, 0, template_success_proj);
   set_idom(unswitched_loop_parse_predicate, template_success_proj, dom_depth(template_success_proj));
   return template_success_proj;

src/hotspot/share/opto/loopTransform.cpp

@@ -1312,80 +1312,6 @@ void PhaseIdealLoop::ensure_zero_trip_guard_proj(Node* node, bool is_main_loop)
 }
 #endif
 
-#ifdef ASSERT
-bool PhaseIdealLoop::assertion_predicate_has_loop_opaque_node(IfNode* iff) {
-  uint init;
-  uint stride;
-  count_opaque_loop_nodes(iff->in(1)->in(1), init, stride);
-  ResourceMark rm;
-  Unique_Node_List wq;
-  wq.clear();
-  wq.push(iff->in(1)->in(1));
-  uint verif_init = 0;
-  uint verif_stride = 0;
-  for (uint i = 0; i < wq.size(); i++) {
-    Node* n = wq.at(i);
-    int op = n->Opcode();
-    if (!n->is_CFG()) {
-      if (n->Opcode() == Op_OpaqueLoopInit) {
-        verif_init++;
-      } else if (n->Opcode() == Op_OpaqueLoopStride) {
-        verif_stride++;
-      } else {
-        for (uint j = 1; j < n->req(); j++) {
-          Node* m = n->in(j);
-          if (m != nullptr) {
-            wq.push(m);
-          }
-        }
-      }
-    }
-  }
-  assert(init == verif_init && stride == verif_stride, "missed opaque node");
-  assert(stride == 0 || init != 0, "init should be there every time stride is");
-  return init != 0;
-}
-
-void PhaseIdealLoop::count_opaque_loop_nodes(Node* n, uint& init, uint& stride) {
-  init = 0;
-  stride = 0;
-  ResourceMark rm;
-  Unique_Node_List wq;
-  wq.push(n);
-  for (uint i = 0; i < wq.size(); i++) {
-    Node* n = wq.at(i);
-    if (TemplateAssertionExpressionNode::is_maybe_in_expression(n)) {
-      if (n->is_OpaqueLoopInit()) {
-        init++;
-      } else if (n->is_OpaqueLoopStride()) {
-        stride++;
-      } else {
-        for (uint j = 1; j < n->req(); j++) {
-          Node* m = n->in(j);
-          if (m != nullptr) {
-            wq.push(m);
-          }
-        }
-      }
-    }
-  }
-}
-#endif // ASSERT
-
-// Create an Initialized Assertion Predicate from the template_assertion_predicate
-IfTrueNode* PhaseIdealLoop::create_initialized_assertion_predicate(IfNode* template_assertion_predicate, Node* new_init,
-                                                                   Node* new_stride, Node* new_control) {
-  assert(assertion_predicate_has_loop_opaque_node(template_assertion_predicate),
-         "must find OpaqueLoop* nodes for Template Assertion Predicate");
-  InitializedAssertionPredicateCreator initialized_assertion_predicate(this);
-  IfTrueNode* success_proj = initialized_assertion_predicate.create_from_template(template_assertion_predicate,
-                                                                                  new_control, new_init, new_stride);
-
-  assert(!assertion_predicate_has_loop_opaque_node(success_proj->in(0)->as_If()),
-         "Initialized Assertion Predicates do not have OpaqueLoop* nodes in the bool expression anymore");
-  return success_proj;
-}
-
 //------------------------------insert_pre_post_loops--------------------------
 // Insert pre and post loops.  If peel_only is set, the pre-loop can not have
 // more iterations added.  It acts as a 'peel' only, no lower-bound RCE, no
@@ -2761,7 +2687,6 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree *loop, Node_List &old_new) {
             loop_entry = initialized_assertion_predicate_creator.create(final_iv_placeholder, loop_entry, stride_con,
                                                                         scale_con, int_offset, int_limit,
                                                                         AssertionPredicateType::FinalIv);
-            assert(!assertion_predicate_has_loop_opaque_node(loop_entry->in(0)->as_If()), "unexpected");
           }
 
           // Add two Template Assertion Predicates to create new Initialized Assertion Predicates from when either
@@ -2769,13 +2694,11 @@ void PhaseIdealLoop::do_range_check(IdealLoopTree *loop, Node_List &old_new) {
           TemplateAssertionPredicateCreator template_assertion_predicate_creator(cl, scale_con , int_offset, int_limit,
                                                                                  this);
           loop_entry = template_assertion_predicate_creator.create(loop_entry);
-          assert(assertion_predicate_has_loop_opaque_node(loop_entry->in(0)->as_If()), "unexpected");
 
           // Initialized Assertion Predicate for the value of the initial main-loop.
           loop_entry = initialized_assertion_predicate_creator.create(init, loop_entry, stride_con, scale_con,
                                                                       int_offset, int_limit,
                                                                       AssertionPredicateType::InitValue);
-          assert(!assertion_predicate_has_loop_opaque_node(loop_entry->in(0)->as_If()), "unexpected");
 
         } else {
           if (PrintOpto) {

src/hotspot/share/opto/loopnode.hpp

@@ -941,12 +941,7 @@ private:
 #ifdef ASSERT
   static void ensure_zero_trip_guard_proj(Node* node, bool is_main_loop);
 #endif
- public:
-  IfTrueNode* create_initialized_assertion_predicate(IfNode* template_assertion_predicate, Node* new_init,
-                                                     Node* new_stride, Node* control);
-  DEBUG_ONLY(static bool assertion_predicate_has_loop_opaque_node(IfNode* iff);)
  private:
-  DEBUG_ONLY(static void count_opaque_loop_nodes(Node* n, uint& init, uint& stride);)
   static void get_template_assertion_predicates(ParsePredicateSuccessProj* parse_predicate_proj, Unique_Node_List& list, bool get_opaque = false);
   void update_main_loop_assertion_predicates(CountedLoopNode* main_loop_head);
   void initialize_assertion_predicates_for_peeled_loop(CountedLoopNode* peeled_loop_head,

src/hotspot/share/opto/loopopts.cpp

@@ -789,7 +789,6 @@ Node *PhaseIdealLoop::conditional_move( Node *region ) {
   assert(!bol->is_OpaqueInitializedAssertionPredicate(), "Initialized Assertion Predicates cannot form a diamond with Halt");
   if (bol->is_OpaqueTemplateAssertionPredicate()) {
     // Ignore Template Assertion Predicates with OpaqueTemplateAssertionPredicate nodes.
-    assert(assertion_predicate_has_loop_opaque_node(iff), "must find OpaqueLoop* nodes");
     return nullptr;
   }
   assert(bol->Opcode() == Op_Bool, "Unexpected node");

src/hotspot/share/opto/node.hpp

@@ -2114,4 +2114,51 @@ inline int Op_DivModIL(BasicType bt, bool is_unsigned) {
   }
 }
 
+// Interface to define actions that should be taken when running DataNodeBFS. Each use can extend this class to specify
+// a customized BFS.
+class BFSActions : public StackObj {
+ public:
+  // Should a node's inputs further be visited in the BFS traversal? By default, we visit all data inputs. Override this
+  // method to provide a custom filter.
+  virtual bool should_visit(Node* node) const {
+    // By default, visit all inputs.
+    return true;
+  };
+
+  // Is the visited node a target node that we are looking for in the BFS traversal? We do not visit its inputs further
+  // but the BFS will continue to visit all unvisited nodes in the queue.
+  virtual bool is_target_node(Node* node) const = 0;
+
+  // Defines an action that should be taken when we visit a target node in the BFS traversal.
+  virtual void target_node_action(Node* target_node) = 0;
+};
+
+// Class to perform a BFS traversal on the data nodes from a given start node. The provided BFSActions guide which
+// data node's inputs should be further visited, which data nodes are target nodes and what to do with the target nodes.
+class DataNodeBFS : public StackObj {
+  BFSActions& _bfs_actions;
+
+ public:
+  explicit DataNodeBFS(BFSActions& bfs_action) : _bfs_actions(bfs_action) {}
+
+  // Run the BFS starting from 'start_node' and apply the actions provided to this class.
+  void run(Node* start_node) {
+    ResourceMark rm;
+    Unique_Node_List _nodes_to_visit;
+    _nodes_to_visit.push(start_node);
+    for (uint i = 0; i < _nodes_to_visit.size(); i++) {
+      Node* next = _nodes_to_visit[i];
+      for (uint j = 1; j < next->req(); j++) {
+        Node* input = next->in(j);
+        if (_bfs_actions.is_target_node(input)) {
+          assert(_bfs_actions.should_visit(input), "must also pass node filter");
+          _bfs_actions.target_node_action(input);
+        } else if (_bfs_actions.should_visit(input)) {
+          _nodes_to_visit.push(input);
+        }
+      }
+    }
+  }
+};
+
 #endif // SHARE_OPTO_NODE_HPP

src/hotspot/share/opto/predicates.cpp

@@ -153,24 +153,21 @@ bool TemplateAssertionPredicate::is_predicate(Node* node) {
 
 // Clone this Template Assertion Predicate and replace the OpaqueLoopInitNode with the provided 'new_opaque_init' node.
 IfTrueNode* TemplateAssertionPredicate::clone(Node* new_control, PhaseIdealLoop* phase) const {
-  assert(PhaseIdealLoop::assertion_predicate_has_loop_opaque_node(_if_node),
+  DEBUG_ONLY(verify();)
-         "must find OpaqueLoop* nodes for Template Assertion Predicate");
   TemplateAssertionExpression template_assertion_expression(opaque_node());
   OpaqueTemplateAssertionPredicateNode* new_opaque_node = template_assertion_expression.clone(new_control, phase);
   AssertionPredicateIfCreator assertion_predicate_if_creator(phase);
   IfTrueNode* success_proj = assertion_predicate_if_creator.create_for_template(new_control, _if_node->Opcode(),
                                                                                 new_opaque_node,
                                                                                 _if_node->assertion_predicate_type());
-  assert(PhaseIdealLoop::assertion_predicate_has_loop_opaque_node(success_proj->in(0)->as_If()),
+  DEBUG_ONLY(TemplateAssertionPredicate::verify(success_proj);)
-         "Template Assertion Predicates must have OpaqueLoop* nodes in the bool expression");
   return success_proj;
 }
 
 // Clone this Template Assertion Predicate and replace the OpaqueLoopInitNode with the provided 'new_opaque_init' node.
 IfTrueNode* TemplateAssertionPredicate::clone_and_replace_init(Node* new_control, OpaqueLoopInitNode* new_opaque_init,
                                                                PhaseIdealLoop* phase) const {
-  assert(PhaseIdealLoop::assertion_predicate_has_loop_opaque_node(_if_node),
+  DEBUG_ONLY(verify();)
-         "must find OpaqueLoop* nodes for Template Assertion Predicate");
   TemplateAssertionExpression template_assertion_expression(opaque_node());
   OpaqueTemplateAssertionPredicateNode* new_opaque_node =
       template_assertion_expression.clone_and_replace_init(new_control, new_opaque_init, phase);
@@ -178,13 +175,13 @@ IfTrueNode* TemplateAssertionPredicate::clone_and_replace_init(Node* new_control
   IfTrueNode* success_proj = assertion_predicate_if_creator.create_for_template(new_control, _if_node->Opcode(),
                                                                                 new_opaque_node,
                                                                                 _if_node->assertion_predicate_type());
-  assert(PhaseIdealLoop::assertion_predicate_has_loop_opaque_node(success_proj->in(0)->as_If()),
+  DEBUG_ONLY(TemplateAssertionPredicate::verify(success_proj);)
-         "Template Assertion Predicates must have OpaqueLoop* nodes in the bool expression");
   return success_proj;
 }
 
 // Replace the input to OpaqueLoopStrideNode with 'new_stride' and leave the other nodes unchanged.
 void TemplateAssertionPredicate::replace_opaque_stride_input(Node* new_stride, PhaseIterGVN& igvn) const {
+  DEBUG_ONLY(verify();)
   TemplateAssertionExpression expression(opaque_node());
   expression.replace_opaque_stride_input(new_stride, igvn);
 }
@@ -192,15 +189,80 @@ void TemplateAssertionPredicate::replace_opaque_stride_input(Node* new_stride, P
 // Create a new Initialized Assertion Predicate from this template at 'new_control' and return the success projection
 // of the newly created Initialized Assertion Predicate.
 IfTrueNode* TemplateAssertionPredicate::initialize(PhaseIdealLoop* phase, Node* new_control) const {
-  assert(phase->assertion_predicate_has_loop_opaque_node(head()),
+  DEBUG_ONLY(verify();)
-         "must find OpaqueLoop* nodes for Template Assertion Predicate");
+  InitializedAssertionPredicateCreator initialized_assertion_predicate_creator(phase);
-  InitializedAssertionPredicateCreator initialized_assertion_predicate(phase);
+  IfTrueNode* success_proj = initialized_assertion_predicate_creator.create_from_template(head(), new_control);
-  IfTrueNode* success_proj = initialized_assertion_predicate.create_from_template(head(), new_control);
+  DEBUG_ONLY(InitializedAssertionPredicate::verify(success_proj);)
-  assert(!phase->assertion_predicate_has_loop_opaque_node(success_proj->in(0)->as_If()),
-         "Initialized Assertion Predicates do not have OpaqueLoop* nodes in the bool expression anymore");
   return success_proj;
 }
 
+#ifdef ASSERT
+// Class to verify Initialized and Template Assertion Predicates by trying to find OpaqueLoop*Nodes.
+class OpaqueLoopNodesVerifier : public BFSActions {
+  bool _found_init;
+  bool _found_stride;
+
+ public:
+  OpaqueLoopNodesVerifier()
+      : _found_init(false),
+        _found_stride(false) {}
+
+  // A Template Assertion Predicate has:
+  // - Always an OpaqueLoopInitNode
+  // - Only an OpaqueLoopStrideNode for the last value.
+  void verify(const TemplateAssertionPredicate& template_assertion_predicate) {
+    DataNodeBFS bfs(*this);
+    bfs.run(template_assertion_predicate.opaque_node());
+    if (template_assertion_predicate.is_last_value()) {
+      assert(_found_init && _found_stride,
+             "must find OpaqueLoopInit and OpaqueLoopStride for last value Template Assertion Predicate");
+    } else {
+      assert(_found_init && !_found_stride,
+             "must find OpaqueLoopInit but not OpaqueLoopStride for init value Template Assertion Predicate");
+    }
+  }
+
+  // An Initialized Assertion Predicate never has any OpaqueLoop*Nodes.
+  void verify(const InitializedAssertionPredicate& initialized_assertion_predicate) {
+    DataNodeBFS bfs(*this);
+    bfs.run(initialized_assertion_predicate.opaque_node());
+    assert(!_found_init && !_found_stride,
+           "must neither find OpaqueLoopInit nor OpaqueLoopStride for Initialized Assertion Predicate");
+  }
+
+  bool should_visit(Node* node) const override {
+    return TemplateAssertionExpressionNode::is_maybe_in_expression(node);
+  }
+
+  bool is_target_node(Node* node) const override {
+    return node->is_Opaque1();
+  }
+
+  void target_node_action(Node* target_node) override {
+    if (target_node->is_OpaqueLoopInit()) {
+      assert(!_found_init, "should only find one OpaqueLoopInitNode");
+      _found_init = true;
+    } else {
+      assert(target_node->is_OpaqueLoopStride(), "unexpected Opaque1 node");
+      assert(!_found_stride, "should only find one OpaqueLoopStrideNode");
+      _found_stride = true;
+    }
+  }
+};
+
+// Verify that the Template Assertion Predicate has the correct OpaqueLoop*Nodes.
+void TemplateAssertionPredicate::verify() const {
+  OpaqueLoopNodesVerifier opaque_loop_nodes_verifier;
+  opaque_loop_nodes_verifier.verify(*this);
+}
+
+// Verify that the Initialized Assertion Predicate has no OpaqueLoop*Node.
+void InitializedAssertionPredicate::verify() const {
+  OpaqueLoopNodesVerifier opaque_loop_nodes_verifier;
+  opaque_loop_nodes_verifier.verify(*this);
+}
+#endif // ASSERT
+
 // Initialized Assertion Predicates always have the dedicated OpaqueInitiailizedAssertionPredicate node to identify
 // them.
 bool InitializedAssertionPredicate::is_predicate(Node* node) {
@@ -418,36 +480,38 @@ TemplateAssertionExpression::clone(const TransformStrategyForOpaqueLoopNodes& tr
 
 // This class is used to replace the input to OpaqueLoopStrideNode with a new node while leaving the other nodes
 // unchanged.
-class ReplaceOpaqueStrideInput : public StackObj {
+class ReplaceOpaqueStrideInput : public BFSActions {
+  Node* _new_opaque_stride_input;
   PhaseIterGVN& _igvn;
-  Unique_Node_List _nodes_to_visit;
 
  public:
-  ReplaceOpaqueStrideInput(OpaqueTemplateAssertionPredicateNode* start_node, PhaseIterGVN& igvn) : _igvn(igvn) {
+  ReplaceOpaqueStrideInput(Node* new_opaque_stride_input, PhaseIterGVN& igvn)
-    _nodes_to_visit.push(start_node);
+      : _new_opaque_stride_input(new_opaque_stride_input),
-  }
+        _igvn(igvn) {}
   NONCOPYABLE(ReplaceOpaqueStrideInput);
 
-  void replace(Node* new_opaque_stride_input) {
+  void replace_for(OpaqueTemplateAssertionPredicateNode* opaque_node) {
-    for (uint i = 0; i < _nodes_to_visit.size(); i++) {
+    DataNodeBFS bfs(*this);
-      Node* next = _nodes_to_visit[i];
+    bfs.run(opaque_node);
-      for (uint j = 1; j < next->req(); j++) {
-        Node* input = next->in(j);
-        if (input->is_OpaqueLoopStride()) {
-          assert(TemplateAssertionExpressionNode::is_maybe_in_expression(input), "must also pass node filter");
-          _igvn.replace_input_of(input, 1, new_opaque_stride_input);
-        } else if (TemplateAssertionExpressionNode::is_maybe_in_expression(input)) {
-          _nodes_to_visit.push(input);
   }
+
+  bool should_visit(Node* node) const override {
+    return TemplateAssertionExpressionNode::is_maybe_in_expression(node);
   }
+
+  bool is_target_node(Node* node) const override {
+    return node->is_OpaqueLoopStride();
   }
+
+  void target_node_action(Node* target_node) override {
+    _igvn.replace_input_of(target_node, 1, _new_opaque_stride_input);
   }
 };
 
 // Replace the input to OpaqueLoopStrideNode with 'new_stride' and leave the other nodes unchanged.
 void TemplateAssertionExpression::replace_opaque_stride_input(Node* new_stride, PhaseIterGVN& igvn) {
-  ReplaceOpaqueStrideInput replace_opaque_stride_input(_opaque_node, igvn);
+  ReplaceOpaqueStrideInput replace_opaque_stride_input(new_stride, igvn);
-  replace_opaque_stride_input.replace(new_stride);
+  replace_opaque_stride_input.replace_for(_opaque_node);
 }
 
 // The transformations of this class fold the OpaqueLoop* nodes by returning their inputs.
@@ -676,10 +740,15 @@ IfTrueNode* TemplateAssertionPredicateCreator::create(Node* new_control) {
   IfTrueNode* template_predicate_success_proj =
       create_if_node(new_control, template_assertion_predicate_expression, does_overflow,
                      AssertionPredicateType::InitValue);
+  DEBUG_ONLY(TemplateAssertionPredicate::verify(template_predicate_success_proj);)
+
   template_assertion_predicate_expression = create_for_last_value(template_predicate_success_proj, opaque_init,
                                                                   does_overflow);
-  return create_if_node(template_predicate_success_proj, template_assertion_predicate_expression,
+  template_predicate_success_proj = create_if_node(template_predicate_success_proj,
-                        does_overflow, AssertionPredicateType::LastValue);
+                                                   template_assertion_predicate_expression, does_overflow,
+                                                   AssertionPredicateType::LastValue);
+  DEBUG_ONLY(TemplateAssertionPredicate::verify(template_predicate_success_proj);)
+  return template_predicate_success_proj;
 }
 
 InitializedAssertionPredicateCreator::InitializedAssertionPredicateCreator(PhaseIdealLoop* phase)
@@ -735,8 +804,10 @@ IfTrueNode* InitializedAssertionPredicateCreator::create(Node* operand, Node* ne
   bool does_overflow;
   OpaqueInitializedAssertionPredicateNode* assertion_expression =
       expression_creator.create_for_initialized(new_control, operand, does_overflow);
-  return create_control_nodes(new_control, does_overflow ? Op_If : Op_RangeCheck, assertion_expression,
+  IfTrueNode* success_proj = create_control_nodes(new_control, does_overflow ? Op_If : Op_RangeCheck,
-                              assertion_predicate_type);
+                                                  assertion_expression, assertion_predicate_type);
+  DEBUG_ONLY(InitializedAssertionPredicate::verify(success_proj);)
+  return success_proj;
 }
 
 // Creates the CFG nodes for the Initialized Assertion Predicate.
@@ -832,9 +903,13 @@ void CreateAssertionPredicatesVisitor::visit(const TemplateAssertionPredicate& t
 // Create an Initialized Assertion Predicate from the provided Template Assertion Predicate.
 IfTrueNode* CreateAssertionPredicatesVisitor::initialize_from_template(
     const TemplateAssertionPredicate& template_assertion_predicate) const {
+  DEBUG_ONLY(template_assertion_predicate.verify();)
   IfNode* template_head = template_assertion_predicate.head();
-  IfTrueNode* initialized_predicate = _phase->create_initialized_assertion_predicate(template_head, _init, _stride,
+  InitializedAssertionPredicateCreator initialized_assertion_predicate_creator(_phase);
-                                                                                     _new_control);
+  IfTrueNode* initialized_predicate = initialized_assertion_predicate_creator.create_from_template(template_head,
+                                                                                                   _new_control,
+                                                                                                   _init, _stride);
+  DEBUG_ONLY(InitializedAssertionPredicate::verify(initialized_predicate);)
   template_assertion_predicate.rewire_loop_data_dependencies(initialized_predicate, _node_in_loop_body, _phase);
   return initialized_predicate;
 }

src/hotspot/share/opto/predicates.hpp

@@ -400,6 +400,15 @@ class TemplateAssertionPredicate : public Predicate {
   void rewire_loop_data_dependencies(IfTrueNode* target_predicate, const NodeInLoopBody& data_in_loop_body,
                                      PhaseIdealLoop* phase) const;
   static bool is_predicate(Node* node);
+
+#ifdef ASSERT
+  static void verify(IfTrueNode* template_assertion_predicate_success_proj) {
+    TemplateAssertionPredicate template_assertion_predicate(template_assertion_predicate_success_proj);
+    template_assertion_predicate.verify();
+  }
+
+  void verify() const;
+#endif // ASSERT
 };
 
 // Class to represent an Initialized Assertion Predicate which always has a halt node on the failing path.
@@ -419,6 +428,10 @@ class InitializedAssertionPredicate : public Predicate {
     return _if_node->in(0);
   }
 
+  OpaqueInitializedAssertionPredicateNode* opaque_node() const {
+    return _if_node->in(1)->as_OpaqueInitializedAssertionPredicate();
+  }
+
   IfNode* head() const override {
     return _if_node;
   }
@@ -433,6 +446,15 @@ class InitializedAssertionPredicate : public Predicate {
 
   void kill(PhaseIdealLoop* phase) const;
   static bool is_predicate(Node* node);
+
+#ifdef ASSERT
+  static void verify(IfTrueNode* initialized_assertion_predicate_success_proj) {
+    InitializedAssertionPredicate initialized_assertion_predicate(initialized_assertion_predicate_success_proj);
+    initialized_assertion_predicate.verify();
+  }
+
+  void verify() const;
+#endif // ASSERT
 };
 
 // Interface to transform OpaqueLoopInit and OpaqueLoopStride nodes of a Template Assertion Expression.