[Mlir-commits] [mlir] f0c22c3 - [Verifier] Tidy up the code a bit, NFC.
Chris Lattner
llvmlistbot at llvm.org
Mon Apr 26 11:54:09 PDT 2021
Author: Chris Lattner
Date: 2021-04-26T11:54:02-07:00
New Revision: f0c22c3d584c5f335b2457b6f9c2723ebdcfa06e
URL: https://github.com/llvm/llvm-project/commit/f0c22c3d584c5f335b2457b6f9c2723ebdcfa06e
DIFF: https://github.com/llvm/llvm-project/commit/f0c22c3d584c5f335b2457b6f9c2723ebdcfa06e.diff
LOG: [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
Added:
Modified:
mlir/lib/IR/Verifier.cpp
Removed:
################################################################################
diff --git a/mlir/lib/IR/Verifier.cpp b/mlir/lib/IR/Verifier.cpp
index c01581d45b4cc..65dad3ea35cd1 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 @@ class OperationVerifier {
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<OpTrait::IsTerminator>())
- return block.back().emitError("block with no terminator, has ")
- << terminator;
-
// Verify that this block is not branching to a block of a
diff erent
// region.
for (Block *successor : block.getSuccessors())
@@ -171,6 +160,14 @@ LogicalResult OperationVerifier::verifyBlock(Block &block) {
return block.back().emitOpError(
"branching to block of a
diff erent 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();
}
@@ -194,31 +191,32 @@ LogicalResult OperationVerifier::verifyOperation(Operation &op) {
if (opInfo && failed(opInfo->verifyInvariants(&op)))
return failure();
- 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++) {
- 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;
-
- // 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 (unsigned numRegions = op.getNumRegions()) {
+ auto kindInterface = dyn_cast<mlir::RegionKindInterface>(op);
+
+ // 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";
+ }
+ 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);
}
More information about the Mlir-commits
mailing list