From f0c22c3d584c5f335b2457b6f9c2723ebdcfa06e Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Sun, 25 Apr 2021 16:15:17 -0700 Subject: [PATCH] [Verifier] Tidy up the code a bit, NFC. 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. Differential Revision: https://reviews.llvm.org/D101267 --- mlir/lib/IR/Verifier.cpp | 116 ++++++++++++++++++++------------------- 1 file changed, 60 insertions(+), 56 deletions(-) diff --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp index c01581d45b4c..65dad3ea35cd 100644 --- a/mlir/lib/IR/Verifier.cpp +++ b/mlir/lib/IR/Verifier.cpp @@ -41,10 +41,10 @@ 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. @@ -69,16 +69,13 @@ private: 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(); @@ -119,7 +116,7 @@ LogicalResult OperationVerifier::verifyRegion(Region ®ion) { /// - 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())) @@ -134,9 +131,8 @@ LogicalResult OperationVerifier::verifyBlock(Block &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"); } @@ -157,13 +153,6 @@ LogicalResult OperationVerifier::verifyBlock(Block &block) { if (failed(verifyOperation(terminator))) return failure(); - if (mayNotHaveTerminator(&block)) - return success(); - - if (!terminator.mightHaveTrait()) - 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()) @@ -171,6 +160,14 @@ LogicalResult OperationVerifier::verifyBlock(Block &block) { 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()) + return block.back().emitError("block with no terminator, has ") + << terminator; + return success(); } @@ -194,31 +191,32 @@ LogicalResult OperationVerifier::verifyOperation(Operation &op) { if (opInfo && failed(opInfo->verifyInvariants(&op))) return failure(); - auto kindInterface = dyn_cast(op); + if (unsigned numRegions = op.getNumRegions()) { + auto kindInterface = dyn_cast(op); - // Verify that all child regions are ok. - unsigned numRegions = op.getNumRegions(); - for (unsigned i = 0; i < numRegions; i++) { - Region ®ion = 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. - if (op.isRegistered() && kind == RegionKind::Graph) { - // Empty regions are fine. - if (region.empty()) - continue; + // Verify that all child regions are ok. + for (unsigned i = 0; i < numRegions; ++i) { + Region ®ion = 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. + 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"; + // 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 (failed(verifyRegion(region))) - return failure(); } // If this is a registered operation, there is nothing left to do. @@ -228,7 +226,7 @@ LogicalResult OperationVerifier::verifyOperation(Operation &op) { // 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 " @@ -247,10 +245,16 @@ LogicalResult OperationVerifier::verifyOperation(Operation &op) { 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 ¬e = diag.attachNote(useOp->getLoc()); note << "operand defined here"; @@ -301,27 +305,27 @@ LogicalResult OperationVerifier::verifyDominance(Region ®ion) { // 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) + } + + // 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 (failed(verifyDominanceOfContainedRegions(op))) return failure(); + } } return success(); } @@ -344,5 +348,5 @@ OperationVerifier::verifyDominanceOfContainedRegions(Operation &op) { /// 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); }