8173709: Fix VerifyLoopOptimizations - step 1 - minimal infrastructure

Reviewed-by: kvn, chagedorn, thartmann

src/hotspot/share/opto/loopnode.cpp

@@ -4447,7 +4447,7 @@ void PhaseIdealLoop::build_and_optimize() {
     assert(C->unique() == unique, "non-optimize _mode made Nodes? ? ?");
     return;
   }
-  if (VerifyLoopOptimizations) verify();
+  DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
   if (TraceLoopOpts && C->has_loops()) {
     _ltree_root->dump();
   }
@@ -4519,7 +4519,7 @@ void PhaseIdealLoop::build_and_optimize() {
   if (!C->major_progress() && SplitIfBlocks && do_split_ifs) {
     visited.clear();
     split_if_with_blocks( visited, nstack);
-    NOT_PRODUCT( if( VerifyLoopOptimizations ) verify(); );
+    DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
   }
 
   if (!C->major_progress() && do_expensive_nodes && process_expensive_nodes()) {
@@ -4642,43 +4642,126 @@ volatile int PhaseIdealLoop::_long_loop_counted_loops=0; // Number of long loops
 void PhaseIdealLoop::print_statistics() {
   tty->print_cr("PhaseIdealLoop=%d, sum _unique=%d, long loops=%d/%d/%d", _loop_invokes, _loop_work, _long_loop_counted_loops, _long_loop_nests, _long_loop_candidates);
 }
+#endif
 
-//------------------------------verify-----------------------------------------
+#ifdef ASSERT
-// Build a verify-only PhaseIdealLoop, and see that it agrees with me.
+// Build a verify-only PhaseIdealLoop, and see that it agrees with "this".
-static int fail;                // debug only, so its multi-thread dont care
 void PhaseIdealLoop::verify() const {
-  int old_progress = C->major_progress();
   ResourceMark rm;
-  PhaseIdealLoop loop_verify(_igvn, this);
+  int old_progress = C->major_progress();
-  VectorSet visited;
+  bool success = true;
 
-  fail = 0;
+  PhaseIdealLoop phase_verify(_igvn, this);
-  verify_compare(C->root(), &loop_verify, visited);
+
-  assert(fail == 0, "verify loops failed");
+  // Verify ctrl and idom of every node.
-  // Verify loop structure is the same
+  success &= verify_idom_and_nodes(C->root(), &phase_verify);
-  _ltree_root->verify_tree(loop_verify._ltree_root, nullptr);
+
-  // Reset major-progress.  It was cleared by creating a verify version of
+  // Verify loop-tree.
-  // PhaseIdealLoop.
+  success &= _ltree_root->verify_tree(phase_verify._ltree_root);
+
+  assert(success, "VerifyLoopOptimizations failed");
+
+  // Major progress was cleared by creating a verify version of PhaseIdealLoop.
   C->restore_major_progress(old_progress);
 }
 
-//------------------------------verify_compare---------------------------------
+// Perform a BFS starting at n, through all inputs.
-// Make sure me and the given PhaseIdealLoop agree on key data structures
+// Call verify_idom and verify_node on all nodes of BFS traversal.
-void PhaseIdealLoop::verify_compare( Node *n, const PhaseIdealLoop *loop_verify, VectorSet &visited ) const {
+bool PhaseIdealLoop::verify_idom_and_nodes(Node* root, const PhaseIdealLoop* phase_verify) const {
-  if( !n ) return;
+  Unique_Node_List worklist;
-  if( visited.test_set( n->_idx ) ) return;
+  worklist.push(root);
-  if( !_nodes[n->_idx] ) {      // Unreachable
+  bool success = true;
-    assert( !loop_verify->_nodes[n->_idx], "both should be unreachable" );
+  for (uint i = 0; i < worklist.size(); i++) {
-    return;
+    Node* n = worklist.at(i);
+    // process node
+    success &= verify_idom(n, phase_verify);
+    success &= verify_nodes(n, phase_verify);
+    // visit inputs
+    for (uint j = 0; j < n->req(); j++) {
+      if (n->in(j) != nullptr) {
+        worklist.push(n->in(j));
+      }
+    }
+  }
+  return success;
+}
+
+// Verify dominator structure (IDOM).
+bool PhaseIdealLoop::verify_idom(Node* n, const PhaseIdealLoop* phase_verify) const {
+  // Verify IDOM for all CFG nodes (except root).
+  if (!n->is_CFG() || n->is_Root()) {
+    return true; // pass
   }
 
-  uint i;
+  if (n->_idx >= _idom_size) {
-  for( i = 0; i < n->req(); i++ )
+    tty->print("CFG Node with no idom: ");
-    verify_compare( n->in(i), loop_verify, visited );
+    n->dump();
+    return false; // fail
+  }
 
-  // Check the '_nodes' block/loop structure
+  // Broken part of VerifyLoopOptimizations (C)
-  i = n->_idx;
+  // Reason:
-  if( has_ctrl(n) ) {           // We have control; verify has loop or ctrl
+  //   Idom not always set correctly, for example BUG in
+  //   PhaseIdealLoop::create_new_if_for_predicate
+  //   at "set_idom(rgn, nrdom, dom_depth(rgn));"
+  /*
+  Node *id = idom_no_update(n);
+  if( id != loop_verify->idom_no_update(n) ) {
+    tty->print("Unequals idoms for: ");
+    n->dump();
+    if( fail++ > 10 ) return;
+    tty->print("We have it as: ");
+    id->dump();
+    tty->print("Verify thinks: ");
+    loop_verify->idom_no_update(n)->dump();
+    tty->cr();
+  }
+  */
+  return true; // pass
+}
+
+// Verify "_nodes": control and loop membership.
+//  (0) _nodes[i] == nullptr -> node not reachable.
+//  (1) has_ctrl -> check lowest bit. 1 -> data node. 0 -> ctrl node.
+//  (2) has_ctrl true: get_ctrl_no_update returns ctrl of data node.
+//  (3) has_ctrl false: get_loop_idx returns IdealLoopTree for ctrl node.
+bool PhaseIdealLoop::verify_nodes(Node* n, const PhaseIdealLoop* phase_verify) const {
+  const uint i = n->_idx;
+  // The loop-tree was built from def to use (top-down).
+  // The verification happens from use to def (bottom-up).
+  // We may thus find nodes during verification that are not in the loop-tree.
+  if (_nodes[i] == nullptr || phase_verify->_nodes[i] == nullptr) {
+    if (_nodes[i] != nullptr || phase_verify->_nodes[i] != nullptr) {
+      tty->print_cr("Was reachable in only one. this %d, verify %d.",
+                 _nodes[i] != nullptr, phase_verify->_nodes[i] != nullptr);
+      n->dump();
+      return false; // fail
+    }
+    // Not reachable for both.
+    return true; // pass
+  }
+
+  if (n->is_CFG() == has_ctrl(n)) {
+    tty->print_cr("Exactly one should be true: %d for is_CFG, %d for has_ctrl.", n->is_CFG(), has_ctrl(n));
+    n->dump();
+    return false; // fail
+  }
+
+  if (has_ctrl(n) != phase_verify->has_ctrl(n)) {
+    tty->print_cr("Mismatch has_ctrl: %d for this, %d for verify.", has_ctrl(n), phase_verify->has_ctrl(n));
+    n->dump();
+    return false; // fail
+  } else if (has_ctrl(n)) {
+    assert(phase_verify->has_ctrl(n), "sanity");
+    // n is a data node.
+    // Verify that its ctrl is the same.
+
+    // Broken part of VerifyLoopOptimizations (A)
+    // Reason:
+    //   BUG, wrong control set for example in
+    //   PhaseIdealLoop::split_if_with_blocks
+    //   at "set_ctrl(x, new_ctrl);"
+    /*
     if( _nodes[i] != loop_verify->_nodes[i] &&
         get_ctrl_no_update(n) != loop_verify->get_ctrl_no_update(n) ) {
       tty->print("Mismatched control setting for: ");
@@ -4695,20 +4778,22 @@ void PhaseIdealLoop::verify_compare( Node *n, const PhaseIdealLoop *loop_verify,
         loop_verify->get_loop_idx(n)->dump();
       tty->cr();
     }
-  } else {                    // We have a loop
+    */
-    IdealLoopTree *us = get_loop_idx(n);
+    return true; // pass
-    if( loop_verify->has_ctrl(n) ) {
+  } else {
-      tty->print("Mismatched loop setting for: ");
+    assert(!phase_verify->has_ctrl(n), "sanity");
-      n->dump();
+    // n is a ctrl node.
-      if( fail++ > 10 ) return;
+    // Verify that not has_ctrl, and that get_loop_idx is the same.
-      tty->print("We have it as: ");
+
-      us->dump();
+    // Broken part of VerifyLoopOptimizations (B)
-      tty->print("Verify thinks: ");
+    // Reason:
-      loop_verify->get_ctrl_no_update(n)->dump();
+    //   NeverBranch node for example is added to loop outside its scope.
-      tty->cr();
+    //   Once we run build_loop_tree again, it is added to the correct loop.
-    } else if (!C->major_progress()) {
+    /*
+    if (!C->major_progress()) {
       // Loop selection can be messed up if we did a major progress
       // operation, like split-if.  Do not verify in that case.
+      IdealLoopTree *us = get_loop_idx(n);
       IdealLoopTree *them = loop_verify->get_loop_idx(n);
       if( us->_head != them->_head ||  us->_tail != them->_tail ) {
         tty->print("Unequals loops for: ");
@@ -4721,102 +4806,120 @@ void PhaseIdealLoop::verify_compare( Node *n, const PhaseIdealLoop *loop_verify,
         tty->cr();
       }
     }
+    */
+    return true; // pass
   }
-
-  // Check for immediate dominators being equal
-  if( i >= _idom_size ) {
-    if( !n->is_CFG() ) return;
-    tty->print("CFG Node with no idom: ");
-    n->dump();
-    return;
-  }
-  if( !n->is_CFG() ) return;
-  if( n == C->root() ) return; // No IDOM here
-
-  assert(n->_idx == i, "sanity");
-  Node *id = idom_no_update(n);
-  if( id != loop_verify->idom_no_update(n) ) {
-    tty->print("Unequals idoms for: ");
-    n->dump();
-    if( fail++ > 10 ) return;
-    tty->print("We have it as: ");
-    id->dump();
-    tty->print("Verify thinks: ");
-    loop_verify->idom_no_update(n)->dump();
-    tty->cr();
-  }
-
 }
 
-//------------------------------verify_tree------------------------------------
+int compare_tree(IdealLoopTree* const& a, IdealLoopTree* const& b) {
-// Verify that tree structures match.  Because the CFG can change, siblings
+  assert(a != nullptr && b != nullptr, "must be");
-// within the loop tree can be reordered.  We attempt to deal with that by
+  return a->_head->_idx - b->_head->_idx;
+}
+
+GrowableArray<IdealLoopTree*> IdealLoopTree::collect_sorted_children() const {
+  GrowableArray<IdealLoopTree*> children;
+  IdealLoopTree* child = _child;
+  while (child != nullptr) {
+    assert(child->_parent == this, "all must be children of this");
+    children.insert_sorted<compare_tree>(child);
+    child = child->_next;
+  }
+  return children;
+}
+
+// Verify that tree structures match. Because the CFG can change, siblings
+// within the loop tree can be reordered. We attempt to deal with that by
 // reordering the verify's loop tree if possible.
-void IdealLoopTree::verify_tree(IdealLoopTree *loop, const IdealLoopTree *parent) const {
+bool IdealLoopTree::verify_tree(IdealLoopTree* loop_verify) const {
-  assert( _parent == parent, "Badly formed loop tree" );
+  assert(_head == loop_verify->_head, "mismatched loop head");
+  assert(this->_parent != nullptr || this->_next == nullptr, "is_root_loop implies has_no_sibling");
 
-  // Siblings not in same order?  Attempt to re-order.
+  // Collect the children
-  if( _head != loop->_head ) {
+  GrowableArray<IdealLoopTree*> children = collect_sorted_children();
-    // Find _next pointer to update
+  GrowableArray<IdealLoopTree*> children_verify = loop_verify->collect_sorted_children();
-    IdealLoopTree **pp = &loop->_parent->_child;
-    while( *pp != loop )
-      pp = &((*pp)->_next);
-    // Find proper sibling to be next
-    IdealLoopTree **nn = &loop->_next;
-    while( (*nn) && (*nn)->_head != _head )
-      nn = &((*nn)->_next);
 
-    // Check for no match.
+  bool success = true;
-    if( !(*nn) ) {
+
-      // Annoyingly, irreducible loops can pick different headers
+  // Compare the two children lists
-      // after a major_progress operation, so the rest of the loop
+  for (int i = 0, j = 0; i < children.length() || j < children_verify.length(); ) {
-      // tree cannot be matched.
+    IdealLoopTree* child        = nullptr;
-      if (_irreducible && Compile::current()->major_progress())  return;
+    IdealLoopTree* child_verify = nullptr;
-      assert( 0, "failed to match loop tree" );
+    // Read from both lists, if possible.
+    if (i < children.length()) {
+      child = children.at(i);
+    }
+    if (j < children_verify.length()) {
+      child_verify = children_verify.at(j);
+    }
+    assert(child != nullptr || child_verify != nullptr, "must find at least one");
+    if (child != nullptr && child_verify != nullptr && child->_head != child_verify->_head) {
+      // We found two non-equal children. Select the smaller one.
+      if (child->_head->_idx < child_verify->_head->_idx) {
+        child_verify = nullptr;
+      } else {
+        child = nullptr;
+      }
+    }
+    // Process the two children, or potentially log the failure if we only found one.
+    if (child_verify == nullptr) {
+      if (child->_irreducible && Compile::current()->major_progress()) {
+        // Irreducible loops can pick a different header (one of its entries).
+      } else {
+        tty->print_cr("We have a loop that verify does not have");
+        child->dump();
+        success = false;
+      }
+      i++; // step for this
+    } else if (child == nullptr) {
+      if (child_verify->_irreducible && Compile::current()->major_progress()) {
+        // Irreducible loops can pick a different header (one of its entries).
+      } else if (child_verify->_head->as_Region()->is_in_infinite_subgraph()) {
+        // Infinite loops do not get attached to the loop-tree on their first visit.
+        // "this" runs before "loop_verify". It is thus possible that we find the
+        // infinite loop only for "child_verify". Only finding it with "child" would
+        // mean that we lost it, which is not ok.
+      } else {
+        tty->print_cr("Verify has a loop that we do not have");
+        child_verify->dump();
+        success = false;
+      }
+      j++; // step for verify
+    } else {
+      assert(child->_head == child_verify->_head, "We have both and they are equal");
+      success &= child->verify_tree(child_verify); // Recursion
+      i++; // step for this
+      j++; // step for verify
     }
-
-    // Move (*nn) to (*pp)
-    IdealLoopTree *hit = *nn;
-    *nn = hit->_next;
-    hit->_next = loop;
-    *pp = loop;
-    loop = hit;
-    // Now try again to verify
   }
 
-  assert( _head  == loop->_head , "mismatched loop head" );
+  // Broken part of VerifyLoopOptimizations (D)
+  // Reason:
+  //   split_if has to update the _tail, if it is modified. But that is done by
+  //   checking to what loop the iff belongs to. That info can be wrong, and then
+  //   we do not update the _tail correctly.
+  /*
   Node *tail = _tail;           // Inline a non-updating version of
   while( !tail->in(0) )         // the 'tail()' call.
     tail = tail->in(1);
   assert( tail == loop->_tail, "mismatched loop tail" );
+  */
 
-  // Counted loops that are guarded should be able to find their guards
+  if (_head->is_CountedLoop()) {
-  if( _head->is_CountedLoop() && _head->as_CountedLoop()->is_main_loop() ) {
     CountedLoopNode *cl = _head->as_CountedLoop();
-    Node *init = cl->init_trip();
-    Node *ctrl = cl->in(LoopNode::EntryControl);
-    assert( ctrl->Opcode() == Op_IfTrue || ctrl->Opcode() == Op_IfFalse, "" );
-    Node *iff  = ctrl->in(0);
-    assert( iff->Opcode() == Op_If, "" );
-    Node *bol  = iff->in(1);
-    assert( bol->Opcode() == Op_Bool, "" );
-    Node *cmp  = bol->in(1);
-    assert( cmp->Opcode() == Op_CmpI, "" );
-    Node *add  = cmp->in(1);
-    Node *opaq;
-    if( add->Opcode() == Op_Opaque1 ) {
-      opaq = add;
-    } else {
-      assert( add->Opcode() == Op_AddI || add->Opcode() == Op_ConI , "" );
-      assert( add == init, "" );
-      opaq = cmp->in(2);
-    }
-    assert( opaq->Opcode() == Op_Opaque1, "" );
 
+    Node* ctrl     = cl->init_control();
+    Node* back     = cl->back_control();
+    assert(ctrl != nullptr && ctrl->is_CFG(), "sane loop in-ctrl");
+    assert(back != nullptr && back->is_CFG(), "sane loop backedge");
+    cl->loopexit(); // assert implied
   }
 
-  if (_child != nullptr)  _child->verify_tree(loop->_child, this);
+  // Broken part of VerifyLoopOptimizations (E)
-  if (_next  != nullptr)  _next ->verify_tree(loop->_next,  parent);
+  // Reason:
+  //   PhaseIdealLoop::split_thru_region creates new nodes for loop that are not added
+  //   to the loop body. Or maybe they are not added to the correct loop.
+  //   at "Node* x = n->clone();"
+  /*
   // Innermost loops need to verify loop bodies,
   // but only if no 'major_progress'
   int fail = 0;
@@ -4859,8 +4962,9 @@ void IdealLoopTree::verify_tree(IdealLoopTree *loop, const IdealLoopTree *parent
     }
     assert( !fail, "loop body mismatch" );
   }
+  */
+  return success;
 }
-
 #endif
 
 //------------------------------set_idom---------------------------------------
@@ -6015,6 +6119,10 @@ void PhaseIdealLoop::build_loop_late_post_work(Node *n, bool pinned) {
   }
 
 #ifdef ASSERT
+  // Broken part of VerifyLoopOptimizations (F)
+  // Reason:
+  //   _verify_me->get_ctrl_no_update(n) seems to return wrong result
+  /*
   // If verifying, verify that 'verify_me' has a legal location
   // and choose it as our location.
   if( _verify_me ) {
@@ -6027,6 +6135,7 @@ void PhaseIdealLoop::build_loop_late_post_work(Node *n, bool pinned) {
     // Check for prior good location
     if( legal == v_ctrl ) least = legal; // Keep prior if found
   }
+  */
 #endif
 
   // Assign discovered "here or above" point

src/hotspot/share/opto/loopnode.hpp

@@ -791,7 +791,11 @@ public:
 #ifndef PRODUCT
   void dump_head();       // Dump loop head only
   void dump();            // Dump this loop recursively
-  void verify_tree(IdealLoopTree *loop, const IdealLoopTree *parent) const;
+#endif
+
+#ifdef ASSERT
+  GrowableArray<IdealLoopTree*> collect_sorted_children() const;
+  bool verify_tree(IdealLoopTree* loop_verify) const;
 #endif
 
  private:
@@ -1697,8 +1701,6 @@ public:
   void dump_idom(Node* n, uint count) const;
   void get_idoms(Node* n, uint count, Unique_Node_List& idoms) const;
   void dump(IdealLoopTree* loop, uint rpo_idx, Node_List &rpo_list) const;
-  void verify() const;          // Major slow  :-)
-  void verify_compare(Node* n, const PhaseIdealLoop* loop_verify, VectorSet &visited) const;
   IdealLoopTree* get_loop_idx(Node* n) const {
     // Dead nodes have no loop, so return the top level loop instead
     return _nodes[n->_idx] ? (IdealLoopTree*)_nodes[n->_idx] : _ltree_root;
@@ -1712,6 +1714,13 @@ public:
   static volatile int _long_loop_counted_loops;
 #endif
 
+#ifdef ASSERT
+  void verify() const;
+  bool verify_idom_and_nodes(Node* root, const PhaseIdealLoop* phase_verify) const;
+  bool verify_idom(Node* n, const PhaseIdealLoop* phase_verify) const;
+  bool verify_nodes(Node* n, const PhaseIdealLoop* phase_verify) const;
+#endif
+
   void rpo(Node* start, Node_Stack &stk, VectorSet &visited, Node_List &rpo_list) const;
 
   void check_counted_loop_shape(IdealLoopTree* loop, Node* x, BasicType bt) NOT_DEBUG_RETURN;

src/hotspot/share/opto/loopopts.cpp

@@ -802,7 +802,7 @@ Node *PhaseIdealLoop::conditional_move( Node *region ) {
         cmov->dump(1);
       }
     }
-    if (VerifyLoopOptimizations) verify();
+    DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
 #endif
   }
 
@@ -1436,9 +1436,7 @@ void PhaseIdealLoop::split_if_with_blocks_post(Node *n) {
           // Place it on the IGVN worklist for later cleanup.
           C->set_major_progress();
           dominated_by(prevdom->as_IfProj(), n->as_If(), false, true);
-#ifndef PRODUCT
+          DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
-          if( VerifyLoopOptimizations ) verify();
-#endif
           return;
         }
         prevdom = dom;

src/hotspot/share/opto/split_if.cpp

@@ -742,7 +742,5 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio
     *new_true_region = new_true;
   }
 
-#ifndef PRODUCT
+  DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
-  if( VerifyLoopOptimizations ) verify();
-#endif
 }