Revert "[Verifier] Speed up and parallelize dominance checking. NFC"

This reverts commit 08664d005c02003180371049b19c7e5d01541c58, which according to
https://reviews.llvm.org/D103373 was pushed accidentally, and I believe it
causes timeouts in some internal mlir tests.

diff --git a/mlir/include/mlir/IR/Dominance.h b/mlir/include/mlir/IR/Dominance.h
index dcf4513..68513c2 100644 (file)
--- a/mlir/include/mlir/IR/Dominance.h
+++ b/mlir/include/mlir/IR/Dominance.h
@@ -35,7 +35,7 @@ class DominanceInfoBase {
   using DomTree = llvm::DominatorTreeBase<Block, IsPostDom>;
 
 public:
-  DominanceInfoBase(Operation *op = nullptr) {}
+  DominanceInfoBase(Operation *op) {}
   DominanceInfoBase(DominanceInfoBase &&) = default;
   DominanceInfoBase &operator=(DominanceInfoBase &&) = default;
   ~DominanceInfoBase();

diff --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp
index ffb8236..ec1ee8b 100644 (file)
--- a/mlir/lib/IR/Verifier.cpp
+++ b/mlir/lib/IR/Verifier.cpp
@@ -31,8 +31,9 @@
 #include "mlir/IR/Operation.h"
 #include "mlir/IR/RegionKindInterface.h"
 #include "llvm/ADT/StringMap.h"
-#include "llvm/Support/Parallel.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Regex.h"
 
 #include <atomic>
 
@@ -42,8 +43,7 @@ namespace {
 /// This class encapsulates all the state used to verify an operation region.
 class OperationVerifier {
 public:
-  explicit OperationVerifier(MLIRContext *context)
-      : parallelismEnabled(context->isMultithreadingEnabled()) {}
+  explicit OperationVerifier() {}
 
   /// Verify the given operation.
   LogicalResult verifyOpAndDominance(Operation &op);
@@ -56,8 +56,7 @@ private:
 
   /// Verify the dominance property of regions contained within the given
   /// Operation.
-  LogicalResult verifyDominanceOfContainedRegions(Operation &op,
-                                                  DominanceInfo &domInfo);
+  LogicalResult verifyDominanceOfContainedRegions(Operation &op);
 
   /// Emit an error for the given block.
   InFlightDiagnostic emitError(Block &bb, const Twine &message) {
@@ -69,8 +68,8 @@ private:
     return mlir::emitError(bb.getParent()->getLoc(), message);
   }
 
-  /// This is true if parallelism is enabled on the MLIRContext.
-  const bool parallelismEnabled;
+  /// Dominance information for this operation, when checking dominance.
+  DominanceInfo *domInfo = nullptr;
 };
 } // end anonymous namespace
 
@@ -84,11 +83,12 @@ LogicalResult OperationVerifier::verifyOpAndDominance(Operation &op) {
   // check for any nested regions. We do this as a second pass since malformed
   // CFG's can cause dominator analysis constructure to crash and we want the
   // verifier to be resilient to malformed code.
-  if (op.getNumRegions() != 0) {
-    DominanceInfo domInfo;
-    if (failed(verifyDominanceOfContainedRegions(op, /*domInfo*/ domInfo)))
-      return failure();
-  }
+  DominanceInfo theDomInfo(&op);
+  domInfo = &theDomInfo;
+  if (failed(verifyDominanceOfContainedRegions(op)))
+    return failure();
+
+  domInfo = nullptr;
   return success();
 }
 
@@ -301,83 +301,34 @@ static void diagnoseInvalidOperandDominance(Operation &op, unsigned operandNo) {
     note << " neither in a parent nor in a child region)";
 }
 
-/// Verify the dominance of each of the nested blocks within the given
-/// operation.  domInfo may be present or absent (null), depending on whether
-/// the caller computed it for a higher level.
+/// Verify the dominance of each of the nested blocks within the given operation
 LogicalResult
-OperationVerifier::verifyDominanceOfContainedRegions(Operation &opWithRegions,
-                                                     DominanceInfo &domInfo) {
-  // This vector keeps track of ops that have regions which should be checked
-  // in parallel.
-  SmallVector<Operation *> opsWithRegionsToCheckInParallel;
-
-  // Get information about the requirements on the regions in this op.
-  for (Region &region : opWithRegions.getRegions()) {
+OperationVerifier::verifyDominanceOfContainedRegions(Operation &op) {
+  for (Region &region : op.getRegions()) {
+    // Verify the dominance of each of the held operations.
     for (Block &block : region) {
       // Dominance is only meaningful inside reachable blocks.
-      bool isReachable = domInfo.isReachableFromEntry(&block);
-
-      // Check each operation in this block, and any operations in regions
-      // that these operations contain.
-      opsWithRegionsToCheckInParallel.clear();
+      bool isReachable = domInfo->isReachableFromEntry(&block);
 
       for (Operation &op : block) {
         if (isReachable) {
           // Check that operands properly dominate this use.
-          for (auto &operand : op.getOpOperands()) {
-            // If the operand doesn't dominate the user, then emit an error.
-            if (!domInfo.properlyDominates(operand.get(), &op)) {
-              diagnoseInvalidOperandDominance(op, operand.getOperandNumber());
-              return failure();
-            }
-          }
-        }
+          for (unsigned operandNo = 0, e = op.getNumOperands(); operandNo != e;
+               ++operandNo) {
+            if (domInfo->properlyDominates(op.getOperand(operandNo), &op))
+              continue;
 
-        // If this operation has any regions, we need to recursively verify
-        // dominance of the ops within it.
-        if (op.getNumRegions() == 0)
-          continue;
-
-        // If this is a non-isolated region (e.g. an affine for loop), pass down
-        // the current dominator information.
-        if (!op.hasTrait<OpTrait::IsIsolatedFromAbove>()) {
-          if (failed(verifyDominanceOfContainedRegions(op, domInfo)))
-            return failure();
-        } else if (parallelismEnabled) {
-          // If this is an IsolatedFromAbove op and parallelism is enabled, then
-          // enqueue this for processing later.
-          opsWithRegionsToCheckInParallel.push_back(&op);
-        } else {
-          // If not, just verify inline with a local dom scope.
-          DominanceInfo localDomInfo;
-          if (failed(verifyDominanceOfContainedRegions(op, localDomInfo)))
+            diagnoseInvalidOperandDominance(op, operandNo);
             return failure();
+          }
         }
-      }
 
-      // If we have multiple parallelizable subregions, check them in parallel.
-      if (opsWithRegionsToCheckInParallel.size() == 1) {
-        // Each isolated operation gets its own dom info.
-        Operation *op = opsWithRegionsToCheckInParallel[0];
-        DominanceInfo localDomInfo;
-        if (failed(verifyDominanceOfContainedRegions(*op, localDomInfo)))
-          return failure();
-      } else if (!opsWithRegionsToCheckInParallel.empty()) {
-        ParallelDiagnosticHandler handler(opWithRegions.getContext());
-        std::atomic<bool> passFailed(false);
-        llvm::parallelForEachN(
-            0, opsWithRegionsToCheckInParallel.size(), [&](size_t opIdx) {
-              handler.setOrderIDForThread(opIdx);
-              Operation *op = opsWithRegionsToCheckInParallel[opIdx];
-
-              // Each isolated operation gets its own dom info.
-              DominanceInfo localDomInfo;
-              if (failed(verifyDominanceOfContainedRegions(*op, localDomInfo)))
-                passFailed = true;
-              handler.eraseOrderIDForThread();
-            });
-        if (passFailed)
-          return failure();
+        // Recursively verify dominance within each operation in the
+        // block, even if the block itself is not reachable, or we are in
+        // a region which doesn't respect dominance.
+        if (op.getNumRegions() != 0)
+          if (failed(verifyDominanceOfContainedRegions(op)))
+            return failure();
       }
     }
   }
@@ -389,8 +340,8 @@ OperationVerifier::verifyDominanceOfContainedRegions(Operation &opWithRegions,
 //===----------------------------------------------------------------------===//
 
 /// Perform (potentially expensive) checks of invariants, used to detect
-/// compiler bugs.  On error, this reports the error through the MLIRContext
-/// and returns failure.
+/// compiler bugs.  On error, this reports the error through the MLIRContext and
+/// returns failure.
 LogicalResult mlir::verify(Operation *op) {
-  return OperationVerifier(op->getContext()).verifyOpAndDominance(*op);
+  return OperationVerifier().verifyOpAndDominance(*op);
 }