8258603: c1 IR::verify is expensive

LudwikJaniuk · Vladimir Kozlov · commit d70545d71080 · 2022-01-12T19:24:52.000Z
Reviewed-by: chagedorn, kvn
diff --git a/src/hotspot/share/c1/c1_IR.cpp b/src/hotspot/share/c1/c1_IR.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1263,21 +1263,36 @@ void IR::print(bool cfg_only, bool live_only) {
     tty->print_cr("invalid IR");
   }
 }
+#endif // PRODUCT
 
+#ifdef ASSERT
 class EndNotNullValidator : public BlockClosure {
  public:
-  EndNotNullValidator(IR* hir) {
-    hir->start()->iterate_postorder(this);
+  virtual void block_do(BlockBegin* block) {
+    assert(block->end() != NULL, "Expect block end to exist.");
   }
+};
 
-  void block_do(BlockBegin* block) {
-    assert(block->end() != NULL, "Expect block end to exist.");
+class XentryFlagValidator : public BlockClosure {
+ public:
+  virtual void block_do(BlockBegin* block) {
+    for (int i = 0; i < block->end()->number_of_sux(); i++) {
+      assert(!block->end()->sux_at(i)->is_set(BlockBegin::exception_entry_flag), "must not be xhandler");
+    }
+    for (int i = 0; i < block->number_of_exception_handlers(); i++) {
+      assert(block->exception_handler_at(i)->is_set(BlockBegin::exception_entry_flag), "must be xhandler");
+    }
   }
 };
 
 typedef GrowableArray<BlockList*> BlockListList;
 
-class PredecessorValidator : public BlockClosure {
+// Validation goals:
+// - code() length == blocks length
+// - code() contents == blocks content
+// - Each block's computed predecessors match sux lists (length)
+// - Each block's computed predecessors match sux lists (set content)
+class PredecessorAndCodeValidator : public BlockClosure {
  private:
   BlockListList* _predecessors; // Each index i will hold predecessors of block with id i
   BlockList*     _blocks;
@@ -1287,7 +1302,7 @@ class PredecessorValidator : public BlockClosure {
   }
 
  public:
-  PredecessorValidator(IR* hir) {
+  PredecessorAndCodeValidator(IR* hir) {
     ResourceMark rm;
     _predecessors = new BlockListList(BlockBegin::number_of_blocks(), BlockBegin::number_of_blocks(), NULL);
     _blocks = new BlockList(BlockBegin::number_of_blocks());
@@ -1308,20 +1323,10 @@ class PredecessorValidator : public BlockClosure {
 
   virtual void block_do(BlockBegin* block) {
     _blocks->append(block);
-    verify_successor_xentry_flag(block);
     collect_predecessors(block);
   }
 
  private:
-  void verify_successor_xentry_flag(const BlockBegin* block) const {
-    for (int i = 0; i < block->end()->number_of_sux(); i++) {
-      assert(!block->end()->sux_at(i)->is_set(BlockBegin::exception_entry_flag), "must not be xhandler");
-    }
-    for (int i = 0; i < block->number_of_exception_handlers(); i++) {
-      assert(block->exception_handler_at(i)->is_set(BlockBegin::exception_entry_flag), "must be xhandler");
-    }
-  }
-
   void collect_predecessors(BlockBegin* block) {
     for (int i = 0; i < block->end()->number_of_sux(); i++) {
       collect_predecessor(block, block->end()->sux_at(i));
@@ -1363,26 +1368,87 @@ class PredecessorValidator : public BlockClosure {
 };
 
 class VerifyBlockBeginField : public BlockClosure {
-
 public:
-
-  virtual void block_do(BlockBegin *block) {
-    for ( Instruction *cur = block; cur != NULL; cur = cur->next()) {
+  virtual void block_do(BlockBegin* block) {
+    for (Instruction* cur = block; cur != NULL; cur = cur->next()) {
       assert(cur->block() == block, "Block begin is not correct");
     }
   }
 };
 
-void IR::verify() {
-#ifdef ASSERT
-  PredecessorValidator pv(this);
-  EndNotNullValidator(this);
+class ValidateEdgeMutuality : public BlockClosure {
+ public:
+  virtual void block_do(BlockBegin* block) {
+    for (int i = 0; i < block->end()->number_of_sux(); i++) {
+      assert(block->end()->sux_at(i)->is_predecessor(block), "Block's successor should have it as predecessor");
+    }
+
+    for (int i = 0; i < block->number_of_exception_handlers(); i++) {
+      assert(block->exception_handler_at(i)->is_predecessor(block), "Block's exception handler should have it as predecessor");
+    }
+
+    for (int i = 0; i < block->number_of_preds(); i++) {
+      assert(block->pred_at(i) != NULL, "Predecessor must exist");
+      assert(block->pred_at(i)->end() != NULL, "Predecessor end must exist");
+      bool is_sux      = block->pred_at(i)->end()->is_sux(block);
+      bool is_xhandler = block->pred_at(i)->is_exception_handler(block);
+      assert(is_sux || is_xhandler, "Block's predecessor should have it as successor or xhandler");
+    }
+  }
+};
+
+void IR::expand_with_neighborhood(BlockList& blocks) {
+  int original_size = blocks.length();
+  for (int h = 0; h < original_size; h++) {
+    BlockBegin* block = blocks.at(h);
+
+    for (int i = 0; i < block->end()->number_of_sux(); i++) {
+      if (!blocks.contains(block->end()->sux_at(i))) {
+        blocks.append(block->end()->sux_at(i));
+      }
+    }
+
+    for (int i = 0; i < block->number_of_preds(); i++) {
+      if (!blocks.contains(block->pred_at(i))) {
+        blocks.append(block->pred_at(i));
+      }
+    }
+
+    for (int i = 0; i < block->number_of_exception_handlers(); i++) {
+      if (!blocks.contains(block->exception_handler_at(i))) {
+        blocks.append(block->exception_handler_at(i));
+      }
+    }
+  }
+}
+
+void IR::verify_local(BlockList& blocks) {
+  EndNotNullValidator ennv;
+  blocks.iterate_forward(&ennv);
+
+  ValidateEdgeMutuality vem;
+  blocks.iterate_forward(&vem);
+
   VerifyBlockBeginField verifier;
-  this->iterate_postorder(&verifier);
-#endif
+  blocks.iterate_forward(&verifier);
 }
 
-#endif // PRODUCT
+void IR::verify() {
+  XentryFlagValidator xe;
+  iterate_postorder(&xe);
+
+  PredecessorAndCodeValidator pv(this);
+
+  EndNotNullValidator ennv;
+  iterate_postorder(&ennv);
+
+  ValidateEdgeMutuality vem;
+  iterate_postorder(&vem);
+
+  VerifyBlockBeginField verifier;
+  iterate_postorder(&verifier);
+}
+#endif // ASSERT
 
 void SubstitutionResolver::visit(Value* v) {
   Value v0 = *v;
diff --git a/src/hotspot/share/c1/c1_IR.hpp b/src/hotspot/share/c1/c1_IR.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -342,7 +342,10 @@ class IR: public CompilationResourceObj {
   // debugging
   static void print(BlockBegin* start, bool cfg_only, bool live_only = false) PRODUCT_RETURN;
   void print(bool cfg_only, bool live_only = false)                           PRODUCT_RETURN;
-  void verify()                                                               PRODUCT_RETURN;
+
+  void expand_with_neighborhood(BlockList& blocks)                          NOT_DEBUG_RETURN;
+  void verify_local(BlockList&)                                             NOT_DEBUG_RETURN;
+  void verify()                                                             NOT_DEBUG_RETURN;
 };
 
 
diff --git a/src/hotspot/share/c1/c1_Instruction.hpp b/src/hotspot/share/c1/c1_Instruction.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1822,6 +1822,7 @@ BASE(BlockEnd, StateSplit)
   // successors
   int number_of_sux() const                      { return _sux != NULL ? _sux->length() : 0; }
   BlockBegin* sux_at(int i) const                { return _sux->at(i); }
+  bool is_sux(BlockBegin* sux) const             { return _sux == NULL ? false : _sux->contains(sux); }
   BlockBegin* default_sux() const                { return sux_at(number_of_sux() - 1); }
   void substitute_sux(BlockBegin* old_sux, BlockBegin* new_sux);
 };
diff --git a/src/hotspot/share/c1/c1_Optimizer.cpp b/src/hotspot/share/c1/c1_Optimizer.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -180,6 +180,25 @@ void CE_Eliminator::block_do(BlockBegin* block) {
     return;
   }
 
+#ifdef ASSERT
+#define DO_DELAYED_VERIFICATION
+  /*
+   * We need to verify the internal representation after modifying it.
+   * Verifying only the blocks that have been tampered with is cheaper than verifying the whole graph, but we must
+   * capture blocks_to_verify_later before making the changes, since they might not be reachable afterwards.
+   * DO_DELAYED_VERIFICATION ensures that the code for this is either enabled in full, or not at all.
+   */
+#endif // ASSERT
+
+#ifdef DO_DELAYED_VERIFICATION
+  BlockList blocks_to_verify_later;
+  blocks_to_verify_later.append(block);
+  blocks_to_verify_later.append(t_block);
+  blocks_to_verify_later.append(f_block);
+  blocks_to_verify_later.append(sux);
+  _hir->expand_with_neighborhood(blocks_to_verify_later);
+#endif // DO_DELAYED_VERIFICATION
+
   // 2) substitute conditional expression
   //    with an IfOp followed by a Goto
   // cut if_ away and get node before
@@ -248,7 +267,10 @@ void CE_Eliminator::block_do(BlockBegin* block) {
     tty->print_cr("%d. IfOp in B%d", ifop_count(), block->block_id());
   }
 
-  _hir->verify();
+#ifdef DO_DELAYED_VERIFICATION
+  _hir->verify_local(blocks_to_verify_later);
+#endif // DO_DELAYED_VERIFICATION
+
 }
 
 Value CE_Eliminator::make_ifop(Value x, Instruction::Condition cond, Value y, Value tval, Value fval) {
@@ -312,6 +334,7 @@ void Optimizer::eliminate_conditional_expressions() {
   CE_Eliminator ce(ir());
 }
 
+// This removes others' relation to block, but doesnt empty block's lists
 void disconnect_from_graph(BlockBegin* block) {
   for (int p = 0; p < block->number_of_preds(); p++) {
     BlockBegin* pred = block->pred_at(p);
@@ -387,6 +410,12 @@ class BlockMerger: public BlockClosure {
     assert(sux_state->caller_state() == end_state->caller_state(), "caller not equal");
 #endif
 
+#ifdef DO_DELAYED_VERIFICATION
+    BlockList blocks_to_verify_later;
+    blocks_to_verify_later.append(block);
+    _hir->expand_with_neighborhood(blocks_to_verify_later);
+#endif // DO_DELAYED_VERIFICATION
+
     // find instruction before end & append first instruction of sux block
     Instruction* prev = end->prev();
     Instruction* next = sux->next();
@@ -396,6 +425,9 @@ class BlockMerger: public BlockClosure {
 
     // disconnect this block from all other blocks
     disconnect_from_graph(sux);
+#ifdef DO_DELAYED_VERIFICATION
+    blocks_to_verify_later.remove(sux); // Sux is not part of graph anymore
+#endif // DO_DELAYED_VERIFICATION
     block->set_end(sux->end());
 
     // TODO Should this be done in set_end universally?
@@ -404,6 +436,7 @@ class BlockMerger: public BlockClosure {
       BlockBegin* xhandler = sux->exception_handler_at(k);
       block->add_exception_handler(xhandler);
 
+      // TODO This should be in disconnect from graph...
       // also substitute predecessor of exception handler
       assert(xhandler->is_predecessor(sux), "missing predecessor");
       xhandler->remove_predecessor(sux);
@@ -419,7 +452,9 @@ class BlockMerger: public BlockClosure {
                     _merge_count, block->block_id(), sux->block_id(), sux->state()->stack_size());
     }
 
-    _hir->verify();
+#ifdef DO_DELAYED_VERIFICATION
+    _hir->verify_local(blocks_to_verify_later);
+#endif // DO_DELAYED_VERIFICATION
 
     If* if_ = block->end()->as_If();
     if (if_) {
@@ -469,7 +504,9 @@ class BlockMerger: public BlockClosure {
                 tty->print_cr("%d. replaced If and IfOp at end of B%d with single If", _merge_count, block->block_id());
               }
 
-              _hir->verify();
+#ifdef DO_DELAYED_VERIFICATION
+              _hir->verify_local(blocks_to_verify_later);
+#endif // DO_DELAYED_VERIFICATION
             }
           }
         }
@@ -485,6 +522,9 @@ class BlockMerger: public BlockClosure {
   }
 };
 
+#ifdef ASSERT
+#undef DO_DELAYED_VERIFICATION
+#endif // ASSERT
 
 void Optimizer::eliminate_blocks() {
   // merge blocks if possible
