[Verifier] Tidy up the code a bit, NFC.

[lattner]

Summary

This tidies up the code a bit:

• Eliminate the ctx member, which doesn't need to be stored.
• Rename verify(Operation) to make it more clear that it is doing more than verifyOperation and that the dominance check isn't being done multiple times.
• Rename mayNotHaveTerminator which was confusing about whether it wasn't known whether it had a terminator, when it is really about whether it is legal to have a terminator.
• Some minor optimizations: don't check for RegionKindInterface if there are no regions. Don't do two passes over the operations in a block in OperationVerifier::verifyDominance when one will do.

The optimizations are actually a measurable (but minor) win in some
CIRCT cases.

[lattner]

A more aggressive optimization would be to use a custom dominance check instead of standard dominance. This would be more efficient given the bulk nature of what verify is doing here, particularly when run on something big like a ModuleOp. Right now, dominance is really set up for sparse queries, e.g. it devolves into isBeforeInBlock vs doing a simple top-down pass keeping track of live values in a scoped hash table.

[rriddle]

LGTM

[lattner]

Thx for the review!

mlir/lib/IR/Verifier.cpp

 #include "llvm/Support/Regex.h"
 
 using namespace mlir;
 
 namespace {
 /// This class encapsulates all the state used to verify an operation region.
 class OperationVerifier {
 public:
-  explicit OperationVerifier(MLIRContext *ctx) : ctx(ctx) {}
+  explicit OperationVerifier() {}
 
   /// Verify the given operation.
-  LogicalResult verify(Operation &op);
+  LogicalResult verifyOpAndDominance(Operation &op);
 
 private:
   /// Verify the given potentially nested region or block.
   LogicalResult verifyRegion(Region &region);
   LogicalResult verifyBlock(Block &block);
   LogicalResult verifyOperation(Operation &op);
 
   /// Verify the dominance property of operations within the given Region.
   LogicalResult verifyDominance(Region &region);
 
   /// Verify the dominance property of regions contained within the given
   /// Operation.
   LogicalResult verifyDominanceOfContainedRegions(Operation &op);
 
   /// Emit an error for the given block.
   InFlightDiagnostic emitError(Block &bb, const Twine &message) {
     // Take the location information for the first operation in the block.
     if (!bb.empty())
       return bb.front().emitError(message);
 
     // Worst case, fall back to using the parent's location.
     return mlir::emitError(bb.getParent()->getLoc(), message);
   }
 
-  /// The current context for the verifier.
-  MLIRContext *ctx;
-
   /// Dominance information for this operation, when checking dominance.
   DominanceInfo *domInfo = nullptr;
 };
 } // end anonymous namespace
 
 /// Verify the given operation.
-LogicalResult OperationVerifier::verify(Operation &op) {
+LogicalResult OperationVerifier::verifyOpAndDominance(Operation &op) {
   // Verify the operation first.
   if (failed(verifyOperation(op)))
     return failure();
 
   // Since everything looks structurally ok to this point, we do a dominance
   // 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.

 }
 
 /// Returns true if this block may be valid without terminator. That is if:
 /// - it does not have a parent region.
 /// - Or the parent region have a single block and:
 ///    - This region does not have a parent op.
 ///    - Or the parent op is unregistered.
 ///    - Or the parent op has the NoTerminator trait.
-static bool mayNotHaveTerminator(Block *block) {
+static bool mayBeValidWithoutTerminator(Block *block) {
   if (!block->getParent())
     return true;
   if (!llvm::hasSingleElement(*block->getParent()))
     return false;
   Operation *op = block->getParentOp();
   return !op || op->mightHaveTrait<OpTrait::NoTerminator>();
 }
 
 LogicalResult OperationVerifier::verifyBlock(Block &block) {
   for (auto arg : block.getArguments())
     if (arg.getOwner() != &block)
       return emitError(block, "block argument not owned by block");
 
   // Verify that this block has a terminator.
-
   if (block.empty()) {
-    if (mayNotHaveTerminator(&block))
+    if (mayBeValidWithoutTerminator(&block))
       return success();
     return emitError(block, "empty block: expect at least a terminator");
   }
 
   // Verify the non-terminator operations separately so that we can verify
   // they have no successors.
   for (auto &op : llvm::make_range(block.begin(), std::prev(block.end()))) {
     if (op.getNumSuccessors() != 0)
       return op.emitError(
           "operation with block successors must terminate its parent block");
 
     if (failed(verifyOperation(op)))
       return failure();
   }
 
   // Verify the terminator.
   Operation &terminator = block.back();
   if (failed(verifyOperation(terminator)))
     return failure();
 
-  if (mayNotHaveTerminator(&block))
-    return success();
-
-  if (!terminator.mightHaveTrait<OpTrait::IsTerminator>())
-    return block.back().emitError("block with no terminator, has ")
-           << terminator;
-
   // Verify that this block is not branching to a block of a different
   // region.
   for (Block *successor : block.getSuccessors())
     if (successor->getParent() != block.getParent())
       return block.back().emitOpError(
           "branching to block of a different region");
 
+  // If this block doesn't have to have a terminator, don't require it.
+  if (mayBeValidWithoutTerminator(&block))
+    return success();
+
+  if (!terminator.mightHaveTrait<OpTrait::IsTerminator>())
+    return block.back().emitError("block with no terminator, has ")
+           << terminator;
+
   return success();
 }
 
 LogicalResult OperationVerifier::verifyOperation(Operation &op) {
   // Check that operands are non-nil and structurally ok.
   for (auto operand : op.getOperands())
     if (!operand)
       return op.emitError("null operand found");
 
   /// Verify that all of the attributes are okay.
   for (auto attr : op.getAttrs()) {
     // Check for any optional dialect specific attributes.
     if (auto *dialect = attr.first.getDialect())
       if (failed(dialect->verifyOperationAttribute(&op, attr)))
         return failure();
   }
 
   // If we can get operation info for this, check the custom hook.
   OperationName opName = op.getName();
   auto *opInfo = opName.getAbstractOperation();
   if (opInfo && failed(opInfo->verifyInvariants(&op)))
     return failure();
 
+  if (unsigned numRegions = op.getNumRegions()) {
     auto kindInterface = dyn_cast<mlir::RegionKindInterface>(op);
 
     // Verify that all child regions are ok.
-  unsigned numRegions = op.getNumRegions();
-  for (unsigned i = 0; i < numRegions; i++) {
+    for (unsigned i = 0; i < numRegions; ++i) {
       Region &region = op.getRegion(i);
       RegionKind kind =
           kindInterface ? kindInterface.getRegionKind(i) : RegionKind::SSACFG;
       // Check that Graph Regions only have a single basic block. This is
       // similar to the code in SingleBlockImplicitTerminator, but doesn't
       // require the trait to be specified. This arbitrary limitation is
       // designed to limit the number of cases that have to be handled by
-    // transforms and conversions until the concept stabilizes.
+      // transforms and conversions.
       if (op.isRegistered() && kind == RegionKind::Graph) {
         // Empty regions are fine.
         if (region.empty())
           continue;
 
         // Non-empty regions must contain a single basic block.
         if (std::next(region.begin()) != region.end())
           return op.emitOpError("expects graph region #")
                  << i << " to have 0 or 1 blocks";
       }
       if (failed(verifyRegion(region)))
         return failure();
     }
+  }
 
   // If this is a registered operation, there is nothing left to do.
   if (opInfo)
     return success();
 
   // Otherwise, verify that the parent dialect allows un-registered operations.
   Dialect *dialect = opName.getDialect();
   if (!dialect) {
-    if (!ctx->allowsUnregisteredDialects()) {
+    if (!op.getContext()->allowsUnregisteredDialects()) {
       return op.emitOpError()
              << "created with unregistered dialect. If this is "
                 "intended, please call allowUnregisteredDialects() on the "
                 "MLIRContext, or use -allow-unregistered-dialect with "
                 "mlir-opt";
     }
     return success();
   }
 
   if (!dialect->allowsUnknownOperations()) {
     return op.emitError("unregistered operation '")
            << op.getName() << "' found in dialect ('" << dialect->getNamespace()
            << "') that does not allow unknown operations";
   }
 
   return success();
 }
 
-/// Attach a note to an in-flight diagnostic that provide more information about
-/// where an op operand is defined.
-static void attachNoteForOperandDefinition(InFlightDiagnostic &diag,
-                                           Operation &op, Value operand) {
+/// Emit an error when the specified operand of the specified operation is an
+/// invalid use because of dominance properties.
+static void diagnoseInvalidOperandDominance(Operation &op, unsigned operandNo) {
+  InFlightDiagnostic diag = op.emitError("operand #")
+                            << operandNo << " does not dominate this use";
+
+  Value operand = op.getOperand(operandNo);
+
+  /// Attach a note to an in-flight diagnostic that provide more information
+  /// about where an op operand is defined.
   if (auto *useOp = operand.getDefiningOp()) {
     Diagnostic &note = diag.attachNote(useOp->getLoc());
     note << "operand defined here";
     Block *block1 = op.getBlock();
     Block *block2 = useOp->getBlock();
     Region *region1 = block1->getParent();
     Region *region2 = block2->getParent();
     if (block1 == block2)
static void diagnoseInvalidOperandDominance(Operation &op, unsigned operandNo) {
   else
     note << " neither in a parent nor in a child region)";
 }
 
 LogicalResult OperationVerifier::verifyDominance(Region &region) {
   // Verify the dominance of each of the held operations.
   for (Block &block : region) {
     // Dominance is only meaningful inside reachable blocks.
-    if (domInfo->isReachableFromEntry(&block))
-      for (Operation &op : block)
+    bool isReachable = domInfo->isReachableFromEntry(&block);
+
+    for (Operation &op : block) {
+      if (isReachable) {
         // Check that operands properly dominate this use.
         for (unsigned operandNo = 0, e = op.getNumOperands(); operandNo != e;
              ++operandNo) {
-          Value operand = op.getOperand(operandNo);
-          if (domInfo->properlyDominates(operand, &op))
+          if (domInfo->properlyDominates(op.getOperand(operandNo), &op))
             continue;
 
-          InFlightDiagnostic diag = op.emitError("operand #")
-                                    << operandNo
-                                    << " does not dominate this use";
-          attachNoteForOperandDefinition(diag, op, operand);
+          diagnoseInvalidOperandDominance(op, operandNo);
           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.
-    for (Operation &op : block)
       if (failed(verifyDominanceOfContainedRegions(op)))
         return failure();
     }
+  }
   return success();
 }
 
 /// Verify the dominance of each of the nested blocks within the given operation
 LogicalResult
 OperationVerifier::verifyDominanceOfContainedRegions(Operation &op) {
   for (Region &region : op.getRegions()) {
     if (failed(verifyDominance(region)))
       return failure();
   }
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Entrypoint
 //===----------------------------------------------------------------------===//
 
 /// Perform (potentially expensive) checks of invariants, used to detect
 /// compiler bugs.  On error, this reports the error through the MLIRContext and
 /// returns failure.
 LogicalResult mlir::verify(Operation *op) {
-  return OperationVerifier(op->getContext()).verify(*op);
+  return OperationVerifier().verifyOpAndDominance(*op);
 }