diff --git a/mlir/docs/OpDefinitions.md b/mlir/docs/OpDefinitions.md --- a/mlir/docs/OpDefinitions.md +++ b/mlir/docs/OpDefinitions.md @@ -565,18 +565,16 @@ ```tablegen let hasVerifier = 1; -``` - -or - -```tablegen let hasRegionVerifier = 1; ``` -This will generate either `LogicalResult verify()` or -`LogicalResult verifyRegions()` method declaration on the op class -that can be defined with any additional verification constraints. These method -will be invoked on its verification order. +This will generate `LogicalResult verify()`/`LogicalResult verifyRegions()` +method declarations on the op class that can be defined with any additional +verification constraints. For verificaiton which needs to access the nested +operations, you should use `hasRegionVerifier` to ensure that it won't access +any ill-formed operation. Except that, The other verifications can be +implemented with `hasVerifier`. Check the next section for the execution order +of these verification methods. #### Verification Ordering diff --git a/mlir/include/mlir/Dialect/Affine/IR/AffineOps.td b/mlir/include/mlir/Dialect/Affine/IR/AffineOps.td --- a/mlir/include/mlir/Dialect/Affine/IR/AffineOps.td +++ b/mlir/include/mlir/Dialect/Affine/IR/AffineOps.td @@ -344,7 +344,7 @@ let hasCanonicalizer = 1; let hasCustomAssemblyFormat = 1; let hasFolder = 1; - let hasVerifier = 1; + let hasRegionVerifier = 1; } def AffineIfOp : Affine_Op<"if", diff --git a/mlir/include/mlir/Dialect/Async/IR/AsyncOps.td b/mlir/include/mlir/Dialect/Async/IR/AsyncOps.td --- a/mlir/include/mlir/Dialect/Async/IR/AsyncOps.td +++ b/mlir/include/mlir/Dialect/Async/IR/AsyncOps.td @@ -84,7 +84,7 @@ let hasCustomAssemblyFormat = 1; let skipDefaultBuilders = 1; - let hasVerifier = 1; + let hasRegionVerifier = 1; let builders = [ OpBuilder<(ins "TypeRange":$resultTypes, "ValueRange":$dependencies, "ValueRange":$operands, diff --git a/mlir/include/mlir/Dialect/GPU/GPUOps.td b/mlir/include/mlir/Dialect/GPU/GPUOps.td --- a/mlir/include/mlir/Dialect/GPU/GPUOps.td +++ b/mlir/include/mlir/Dialect/GPU/GPUOps.td @@ -556,7 +556,7 @@ let hasCanonicalizer = 1; let hasCustomAssemblyFormat = 1; - let hasVerifier = 1; + let hasRegionVerifier = 1; } def GPU_PrintfOp : GPU_Op<"printf", [MemoryEffects<[MemWrite]>]>, @@ -677,7 +677,7 @@ let regions = (region AnyRegion:$body); let assemblyFormat = [{ custom($op) $value $body attr-dict `:` functional-type(operands, results) }]; - let hasVerifier = 1; + let hasRegionVerifier = 1; } def GPU_ShuffleOpXor : I32EnumAttrCase<"XOR", 0, "xor">; diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td --- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td +++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td @@ -1140,6 +1140,7 @@ let hasCustomAssemblyFormat = 1; let hasVerifier = 1; + let hasRegionVerifier = 1; } def LLVM_GlobalCtorsOp : LLVM_Op<"mlir.global_ctors", [ @@ -1277,6 +1278,7 @@ let hasCustomAssemblyFormat = 1; let hasVerifier = 1; + let hasRegionVerifier = 1; } def LLVM_NullOp diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td --- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td +++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td @@ -34,6 +34,7 @@ }]; let cppNamespace = "::mlir::linalg"; let verify = [{ return detail::verifyContractionInterface($_op); }]; + let verifyWithRegions = 1; let methods = [ InterfaceMethod< /*desc=*/"Returns the left-hand side operand.", @@ -1136,6 +1137,7 @@ }]; let verify = [{ return detail::verifyStructuredOpInterface($_op); }]; + let verifyWithRegions = 1; } #endif // LINALG_IR_LINALGINTERFACES diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td --- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td +++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td @@ -204,6 +204,7 @@ }]; let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// @@ -437,7 +438,8 @@ } -def CriticalOp : OpenMP_Op<"critical"> { +def CriticalOp : OpenMP_Op<"critical", + [DeclareOpInterfaceMethods]> { let summary = "critical construct"; let description = [{ The critical construct imposes a restriction on the associated structured @@ -451,7 +453,6 @@ let assemblyFormat = [{ (`(` $name^ `)`)? $region attr-dict }]; - let hasVerifier = 1; } //===----------------------------------------------------------------------===// @@ -671,6 +672,7 @@ $x `:` type($x) $region attr-dict }]; let hasVerifier = 1; + let hasRegionVerifier = 1; } def AtomicCaptureOp : OpenMP_Op<"atomic.capture", @@ -717,7 +719,7 @@ |`hint` `(` custom($hint_val) `)`) $region attr-dict }]; - let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// @@ -771,7 +773,7 @@ return atomicReductionRegion().front().getArgument(0).getType(); } }]; - let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// diff --git a/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td b/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td --- a/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td +++ b/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td @@ -453,7 +453,7 @@ /// Returns the rewrite operation of this pattern. RewriteOp getRewriter(); }]; - let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// @@ -632,7 +632,7 @@ ($body^)? attr-dict-with-keyword }]; - let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// diff --git a/mlir/include/mlir/Dialect/SCF/SCFOps.td b/mlir/include/mlir/Dialect/SCF/SCFOps.td --- a/mlir/include/mlir/Dialect/SCF/SCFOps.td +++ b/mlir/include/mlir/Dialect/SCF/SCFOps.td @@ -308,6 +308,7 @@ let hasCanonicalizer = 1; let hasCustomAssemblyFormat = 1; let hasVerifier = 1; + let hasRegionVerifier = 1; } def IfOp : SCF_Op<"if", @@ -535,7 +536,7 @@ let arguments = (ins AnyType:$operand); let hasCustomAssemblyFormat = 1; let regions = (region SizedRegion<1>:$reductionOperator); - let hasVerifier = 1; + let hasRegionVerifier = 1; } def ReduceReturnOp : @@ -688,7 +689,7 @@ let hasCanonicalizer = 1; let hasCustomAssemblyFormat = 1; - let hasVerifier = 1; + let hasRegionVerifier = 1; } def YieldOp : SCF_Op<"yield", [NoSideEffect, ReturnLike, Terminator, diff --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td --- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td +++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td @@ -309,6 +309,9 @@ let hasOpcode = 0; let autogenSerialization = 0; + + let hasVerifier = 0; + let hasRegionVerifier = 1; } // ----- @@ -470,6 +473,9 @@ let autogenSerialization = 0; let hasCanonicalizer = 1; + + let hasVerifier = 0; + let hasRegionVerifier = 1; } #endif // MLIR_DIALECT_SPIRV_IR_CONTROLFLOW_OPS diff --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVStructureOps.td b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVStructureOps.td --- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVStructureOps.td +++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVStructureOps.td @@ -516,6 +516,9 @@ static StringRef getVCETripleAttrName() { return "vce_triple"; } }]; + + let hasVerifier = 0; + let hasRegionVerifier = 1; } // ----- @@ -749,6 +752,9 @@ let hasOpcode = 0; let autogenSerialization = 0; + + let hasVerifier = 0; + let hasRegionVerifier = 1; } // ----- diff --git a/mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td b/mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td --- a/mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td +++ b/mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td @@ -408,6 +408,7 @@ let hasCanonicalizer = 1; let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// @@ -983,6 +984,7 @@ let hasCanonicalizer = 1; let hasFolder = 1; let hasVerifier = 1; + let hasRegionVerifier = 1; } //===----------------------------------------------------------------------===// diff --git a/mlir/include/mlir/IR/OpBase.td b/mlir/include/mlir/IR/OpBase.td --- a/mlir/include/mlir/IR/OpBase.td +++ b/mlir/include/mlir/IR/OpBase.td @@ -2073,9 +2073,11 @@ // X op Y == Y op X def Commutative : NativeOpTrait<"IsCommutative">; // op op X == op X (unary) / X op X == X (binary) -def Idempotent : NativeOpTrait<"IsIdempotent">; +// FIXME: Idempotent should depend on SameOperandsAndResultType +def Idempotent : NativeOpTrait<"IsIdempotent">; // op op X == X -def Involution : NativeOpTrait<"IsInvolution">; +// FIXME: Involution should depend on SameOperandsAndResultType +def Involution : NativeOpTrait<"IsInvolution">; // Op behaves like a constant. def ConstantLike : NativeOpTrait<"ConstantLike">; // Op is isolated from above. @@ -2103,11 +2105,11 @@ // Op is elementwise on tensor/vector operands and results. def Elementwise : NativeOpTrait<"Elementwise">; // Elementwise op can be applied to scalars instead tensor/vector operands. -def Scalarizable : NativeOpTrait<"Scalarizable">; +def Scalarizable : NativeOpTrait<"Scalarizable", [Elementwise]>; // Elementwise op can be applied to all-vector operands. -def Vectorizable : NativeOpTrait<"Vectorizable">; +def Vectorizable : NativeOpTrait<"Vectorizable", [Elementwise]>; // Elementwise op can be applied to all-tensor operands. -def Tensorizable : NativeOpTrait<"Tensorizable">; +def Tensorizable : NativeOpTrait<"Tensorizable", [Elementwise]>; // Group together `Elementwise`, `Scalarizable`, `Vectorizable`, and // `Tensorizable` for convenience. @@ -2489,7 +2491,9 @@ // verified (aside from those verified by other ODS constructs). If set to `1`, // an additional `LogicalResult verify()` declaration will be generated on the // operation class. The operation should implement this method and verify the - // additional necessary invariants. + // additional necessary invariants. This verifier shouldn't access any nested + // operations because those operations may ill-formed. Use the + // `hasRegionVerifier` below instead. bit hasVerifier = 0; // A bit indicating if the operation has additional invariants that need to diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h --- a/mlir/include/mlir/IR/OpDefinition.h +++ b/mlir/include/mlir/IR/OpDefinition.h @@ -920,7 +920,7 @@ /// The type of the operation used as the implicit terminator type. using ImplicitTerminatorOpT = TerminatorOpType; - static LogicalResult verifyTrait(Operation *op) { + static LogicalResult verifyRegionTrait(Operation *op) { if (failed(Base::verifyTrait(op))) return failure(); for (unsigned i = 0, e = op->getNumRegions(); i < e; ++i) { @@ -1218,7 +1218,7 @@ class IsIsolatedFromAbove : public TraitBase { public: - static LogicalResult verifyTrait(Operation *op) { + static LogicalResult verifyRegionTrait(Operation *op) { return impl::verifyIsIsolatedFromAbove(op); } }; @@ -1262,7 +1262,7 @@ class Impl : public TraitBase { public: static LogicalResult verifyTrait(Operation *op) { - if (llvm::isa(op->getParentOp())) + if (llvm::isa_and_nonnull(op->getParentOp())) return success(); return op->emitOpError() diff --git a/mlir/include/mlir/IR/SymbolTable.h b/mlir/include/mlir/IR/SymbolTable.h --- a/mlir/include/mlir/IR/SymbolTable.h +++ b/mlir/include/mlir/IR/SymbolTable.h @@ -337,7 +337,7 @@ template class SymbolTable : public TraitBase { public: - static LogicalResult verifyTrait(Operation *op) { + static LogicalResult verifyRegionTrait(Operation *op) { return ::mlir::detail::verifySymbolTable(op); } diff --git a/mlir/include/mlir/Interfaces/InferTypeOpInterface.td b/mlir/include/mlir/Interfaces/InferTypeOpInterface.td --- a/mlir/include/mlir/Interfaces/InferTypeOpInterface.td +++ b/mlir/include/mlir/Interfaces/InferTypeOpInterface.td @@ -64,6 +64,8 @@ >, ]; + // Inferring result types may need to access the region operations. + let verifyWithRegions = 1; let verify = [{ return detail::verifyInferredResultTypes($_op); }]; diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp --- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp +++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp @@ -1304,7 +1304,7 @@ bodyBuilder); } -LogicalResult AffineForOp::verify() { +LogicalResult AffineForOp::verifyRegions() { // Check that the body defines as single block argument for the induction // variable. auto *body = getBody(); diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp --- a/mlir/lib/Dialect/Async/IR/Async.cpp +++ b/mlir/lib/Dialect/Async/IR/Async.cpp @@ -236,7 +236,7 @@ return success(); } -LogicalResult ExecuteOp::verify() { +LogicalResult ExecuteOp::verifyRegions() { // Unwrap async.execute value operands types. auto unwrappedTypes = llvm::map_range(operands(), [](Value operand) { return operand.getType().cast().getValueType(); diff --git a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp --- a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp +++ b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp @@ -275,7 +275,7 @@ return walkResult.wasInterrupted() ? failure() : success(); } -LogicalResult gpu::AllReduceOp::verify() { +LogicalResult gpu::AllReduceOp::verifyRegions() { if (body().empty() != op().hasValue()) return emitError("expected either an op attribute or a non-empty body"); if (!body().empty()) { @@ -407,7 +407,7 @@ return KernelDim3{getOperand(3), getOperand(4), getOperand(5)}; } -LogicalResult LaunchOp::verify() { +LogicalResult LaunchOp::verifyRegions() { // Kernel launch takes kNumConfigOperands leading operands for grid/block // sizes and transforms them into kNumConfigRegionAttributes region arguments // for block/thread identifiers and grid/block sizes. diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp --- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp +++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp @@ -1795,26 +1795,6 @@ "attribute"); } - if (Block *b = getInitializerBlock()) { - ReturnOp ret = cast(b->getTerminator()); - if (ret.operand_type_begin() == ret.operand_type_end()) - return emitOpError("initializer region cannot return void"); - if (*ret.operand_type_begin() != getType()) - return emitOpError("initializer region type ") - << *ret.operand_type_begin() << " does not match global type " - << getType(); - - for (Operation &op : *b) { - auto iface = dyn_cast(op); - if (!iface || !iface.hasNoEffect()) - return op.emitError() - << "ops with side effects not allowed in global initializers"; - } - - if (getValueOrNull()) - return emitOpError("cannot have both initializer value and region"); - } - if (getLinkage() == Linkage::Common) { if (Attribute value = getValueOrNull()) { if (!isZeroAttribute(value)) { @@ -1843,6 +1823,30 @@ return success(); } +LogicalResult GlobalOp::verifyRegions() { + if (Block *b = getInitializerBlock()) { + ReturnOp ret = cast(b->getTerminator()); + if (ret.operand_type_begin() == ret.operand_type_end()) + return emitOpError("initializer region cannot return void"); + if (*ret.operand_type_begin() != getType()) + return emitOpError("initializer region type ") + << *ret.operand_type_begin() << " does not match global type " + << getType(); + + for (Operation &op : *b) { + auto iface = dyn_cast(op); + if (!iface || !iface.hasNoEffect()) + return op.emitError() + << "ops with side effects not allowed in global initializers"; + } + + if (getValueOrNull()) + return emitOpError("cannot have both initializer value and region"); + } + + return success(); +} + //===----------------------------------------------------------------------===// // LLVM::GlobalCtorsOp //===----------------------------------------------------------------------===// @@ -2123,7 +2127,6 @@ // - functions don't have 'common' linkage // - external functions have 'external' or 'extern_weak' linkage; // - vararg is (currently) only supported for external functions; -// - entry block arguments are of LLVM types and match the function signature. LogicalResult LLVMFuncOp::verify() { if (getLinkage() == LLVM::Linkage::Common) return emitOpError() << "functions cannot have '" @@ -2152,6 +2155,15 @@ if (isVarArg()) return emitOpError("only external functions can be variadic"); + return success(); +} + +/// Verifies LLVM- and implementation-specific properties of the LLVM func Op: +/// - entry block arguments are of LLVM types and match the function signature. +LogicalResult LLVMFuncOp::verifyRegions() { + if (isExternal()) + return success(); + unsigned numArguments = getType().getNumParams(); Block &entryBlock = front(); for (unsigned i = 0; i < numArguments; ++i) { diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp --- a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp +++ b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp @@ -1207,7 +1207,7 @@ return emitOpError("expected single non-empty parent region"); if (auto linalgOp = dyn_cast(parentOp)) - return verifyYield(*this, cast(parentOp)); + return verifyYield(*this, linalgOp); return emitOpError("expected parent op with LinalgOp interface"); } diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -369,6 +369,8 @@ return success(); } + // TODO: The followings should be done in + // SymbolUserOpInterface::verifySymbolUses. DenseSet accumulators; for (auto args : llvm::zip(reductionVars, *reductions)) { Value accum = std::get<0>(args); @@ -719,6 +721,10 @@ return emitError( "expected equal sizes for allocate and allocator variables"); + return verifyReductionVarList(*this, reductions(), reduction_vars()); +} + +LogicalResult SectionsOp::verifyRegions() { for (auto &inst : *region().begin()) { if (!(isa(inst) || isa(inst))) { return emitOpError() @@ -726,7 +732,7 @@ } } - return verifyReductionVarList(*this, reductions(), reduction_vars()); + return success(); } /// Parses an OpenMP Workshare Loop operation @@ -851,7 +857,7 @@ printer.printRegion(region); } -LogicalResult ReductionDeclareOp::verify() { +LogicalResult ReductionDeclareOp::verifyRegions() { if (initializerRegion().empty()) return emitOpError() << "expects non-empty initializer region"; Block &initializerEntryBlock = initializerRegion().front(); @@ -941,10 +947,11 @@ return verifySynchronizationHint(*this, hint_val()); } -LogicalResult CriticalOp::verify() { +LogicalResult +CriticalOp::verifySymbolUses(SymbolTableCollection &symbol_table) { if (nameAttr()) { SymbolRefAttr symbolRef = nameAttr(); - auto decl = SymbolTable::lookupNearestSymbolFrom( + auto decl = symbol_table.lookupNearestSymbolFrom( *this, symbolRef); if (!decl) { return emitOpError() << "expected symbol reference " << symbolRef @@ -1038,15 +1045,19 @@ } } - if (region().getNumArguments() != 1) - return emitError("the region must accept exactly one argument"); - if (x().getType().cast().getElementType() != region().getArgument(0).getType()) { return emitError("the type of the operand must be a pointer type whose " "element type is the same as that of the region argument"); } + return success(); +} + +LogicalResult AtomicUpdateOp::verifyRegions() { + if (region().getNumArguments() != 1) + return emitError("the region must accept exactly one argument"); + if (region().front().getOperations().size() < 2) return emitError() << "the update region must have at least two operations " "(binop and terminator)"; @@ -1064,7 +1075,7 @@ // Verifier for AtomicCaptureOp //===----------------------------------------------------------------------===// -LogicalResult AtomicCaptureOp::verify() { +LogicalResult AtomicCaptureOp::verifyRegions() { Block::OpListType &ops = region().front().getOperations(); if (ops.size() != 3) return emitError() diff --git a/mlir/lib/Dialect/PDL/IR/PDL.cpp b/mlir/lib/Dialect/PDL/IR/PDL.cpp --- a/mlir/lib/Dialect/PDL/IR/PDL.cpp +++ b/mlir/lib/Dialect/PDL/IR/PDL.cpp @@ -269,7 +269,7 @@ // pdl::PatternOp //===----------------------------------------------------------------------===// -LogicalResult PatternOp::verify() { +LogicalResult PatternOp::verifyRegions() { Region &body = getBodyRegion(); Operation *term = body.front().getTerminator(); auto rewriteOp = dyn_cast(term); @@ -402,7 +402,7 @@ // pdl::RewriteOp //===----------------------------------------------------------------------===// -LogicalResult RewriteOp::verify() { +LogicalResult RewriteOp::verifyRegions() { Region &rewriteRegion = body(); // Handle the case where the rewrite is external. diff --git a/mlir/lib/Dialect/SCF/SCF.cpp b/mlir/lib/Dialect/SCF/SCF.cpp --- a/mlir/lib/Dialect/SCF/SCF.cpp +++ b/mlir/lib/Dialect/SCF/SCF.cpp @@ -282,6 +282,19 @@ if (cst.value() <= 0) return emitOpError("constant step operand must be positive"); + auto opNumResults = getNumResults(); + if (opNumResults == 0) + return success(); + // If ForOp defines values, check that the number and types of + // the defined values match ForOp initial iter operands and backedge + // basic block arguments. + if (getNumIterOperands() != opNumResults) + return emitOpError( + "mismatch in number of loop-carried values and defined values"); + return success(); +} + +LogicalResult ForOp::verifyRegions() { // Check that the body defines as single block argument for the induction // variable. auto *body = getBody(); @@ -293,15 +306,11 @@ auto opNumResults = getNumResults(); if (opNumResults == 0) return success(); - // If ForOp defines values, check that the number and types of - // the defined values match ForOp initial iter operands and backedge - // basic block arguments. - if (getNumIterOperands() != opNumResults) - return emitOpError( - "mismatch in number of loop-carried values and defined values"); + if (getNumRegionIterArgs() != opNumResults) return emitOpError( "mismatch in number of basic block args and defined values"); + auto iterOperands = getIterOperands(); auto iterArgs = getRegionIterArgs(); auto opResults = getResults(); @@ -2130,7 +2139,7 @@ body->getArgument(1)); } -LogicalResult ReduceOp::verify() { +LogicalResult ReduceOp::verifyRegions() { // The region of a ReduceOp has two arguments of the same type as its operand. auto type = getOperand().getType(); Block &block = getReductionOperator().front(); @@ -2333,7 +2342,7 @@ return nullptr; } -LogicalResult scf::WhileOp::verify() { +LogicalResult scf::WhileOp::verifyRegions() { auto beforeTerminator = verifyAndGetTerminator( *this, getBefore(), "expects the 'before' region to terminate with 'scf.condition'"); diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp --- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp +++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp @@ -2935,7 +2935,7 @@ return branchOp && branchOp.getSuccessor() == &dstBlock; } -LogicalResult spirv::LoopOp::verify() { +LogicalResult spirv::LoopOp::verifyRegions() { auto *op = getOperation(); // We need to verify that the blocks follow the following layout: @@ -3072,6 +3072,7 @@ return emitOpError( "expected parent op to be 'spv.mlir.selection' or 'spv.mlir.loop'"); + // TODO: This check should be done in `verifyRegions` of parent op. Block &parentLastBlock = (*this)->getParentRegion()->back(); if (getOperation() != parentLastBlock.getTerminator()) return emitOpError("can only be used in the last block of " @@ -3173,7 +3174,7 @@ printer.printRegion(getRegion()); } -LogicalResult spirv::ModuleOp::verify() { +LogicalResult spirv::ModuleOp::verifyRegions() { Dialect *dialect = (*this)->getDialect(); DenseMap, spirv::EntryPointOp> entryPoints; @@ -3322,7 +3323,7 @@ /*printBlockTerminators=*/true); } -LogicalResult spirv::SelectionOp::verify() { +LogicalResult spirv::SelectionOp::verifyRegions() { auto *op = getOperation(); // We need to verify that the blocks follow the following layout: @@ -4106,7 +4107,7 @@ printer.printGenericOp(&body().front().front()); } -LogicalResult spirv::SpecConstantOperationOp::verify() { +LogicalResult spirv::SpecConstantOperationOp::verifyRegions() { Block &block = getRegion().getBlocks().front(); if (block.getOperations().size() != 2) diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp --- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp +++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp @@ -533,6 +533,11 @@ return emitError("must have as many index operands as dynamic extents " "in the result type"); + return success(); +} + +LogicalResult GenerateOp::verifyRegions() { + RankedTensorType resultTy = getType().cast(); // Ensure that region arguments span the index space. if (!llvm::all_of(body().getArgumentTypes(), [](Type ty) { return ty.isIndex(); })) @@ -1749,8 +1754,12 @@ << expectedType; } + return success(); +} + +LogicalResult PadOp::verifyRegions() { auto ®ion = getRegion(); - unsigned rank = resultType.getRank(); + unsigned rank = result().getType().cast().getRank(); Block &block = region.front(); if (block.getNumArguments() != rank) return emitError("expected the block to have ") << rank << " arguments"; diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp --- a/mlir/lib/IR/Operation.cpp +++ b/mlir/lib/IR/Operation.cpp @@ -1088,12 +1088,6 @@ while (!pendingRegions.empty()) { for (Operation &op : pendingRegions.pop_back_val()->getOps()) { for (Value operand : op.getOperands()) { - // operand should be non-null here if the IR is well-formed. But - // we don't assert here as this function is called from the verifier - // and so could be called on invalid IR. - if (!operand) - return op.emitOpError("operation's operand is null"); - // Check that any value that is used by an operation is defined in the // same region as either an operation result. auto *operandRegion = operand.getParentRegion(); diff --git a/mlir/test/Dialect/GPU/invalid.mlir b/mlir/test/Dialect/GPU/invalid.mlir --- a/mlir/test/Dialect/GPU/invalid.mlir +++ b/mlir/test/Dialect/GPU/invalid.mlir @@ -15,7 +15,7 @@ "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({ ^bb1(%bx: index, %by: index, %bz: index, %tx: index, %ty: index, %tz: index): - gpu.return + gpu.terminator }) : (index, index, index, index, index, index) -> () return } @@ -26,8 +26,9 @@ // @expected-note@+1 {{in 'gpu.launch' body region}} gpu.launch blocks(%bx, %by, %bz) in (%sbx = %sz, %sby = %sz, %sbz = %sz) threads(%tx, %ty, %tz) in (%stx = %sz, %sty = %sz, %stz = %sz) { - // @expected-error@+1 {{expected 'gpu.terminator' or a terminator with successors}} - return + // @expected-error@+2 {{expected 'gpu.terminator' or a terminator with successors}} + %one = arith.constant 1 : i32 + "gpu.yield"(%one) : (i32) -> () } return } @@ -293,7 +294,7 @@ // expected-error@+1 {{expected gpu.yield op in region}} %res = gpu.all_reduce %arg0 { ^bb(%lhs : f32, %rhs : f32): - return + "test.finish" () : () -> () } : (f32) -> (f32) return } diff --git a/mlir/test/Dialect/LLVMIR/global.mlir b/mlir/test/Dialect/LLVMIR/global.mlir --- a/mlir/test/Dialect/LLVMIR/global.mlir +++ b/mlir/test/Dialect/LLVMIR/global.mlir @@ -172,8 +172,7 @@ // ----- -// expected-error @+2 {{'llvm.mlir.global' op expects regions to end with 'llvm.return', found 'llvm.mlir.constant'}} -// expected-note @+1 {{in custom textual format, the absence of terminator implies 'llvm.return'}} +// expected-error @+2 {{block with no terminator}} llvm.mlir.global internal @g() : i64 { %c = llvm.mlir.constant(42 : i64) : i64 } diff --git a/mlir/test/Dialect/Linalg/invalid.mlir b/mlir/test/Dialect/Linalg/invalid.mlir --- a/mlir/test/Dialect/Linalg/invalid.mlir +++ b/mlir/test/Dialect/Linalg/invalid.mlir @@ -202,7 +202,7 @@ // ----- func @generic_mismatched_num_arguments(%arg0: memref) { - // expected-error @+1 {{expected as many non-induction variable region arguments as the number of input/output operands}} + // expected-error @+6 {{'linalg.yield' op expected number of yield values (2) to match the number of operands of the enclosing LinalgOp (1)}} linalg.generic { indexing_maps = [ affine_map<() -> ()>, affine_map<() -> ()> ], iterator_types = []} @@ -215,7 +215,7 @@ // ----- func @generic_shaped_operand_block_arg_type(%arg0: memref) { - // expected-error @+1 {{expected type of bb argument #0 ('i1') to match element or self type of the corresponding operand ('f32')}} + // expected-error @+6 {{'linalg.yield' op type of yield operand 1 ('i1') doesn't match the element type of the enclosing linalg.generic op ('f32')}} linalg.generic { indexing_maps = [ affine_map<() -> ()> ], iterator_types = []} @@ -228,7 +228,7 @@ // ----- func @generic_scalar_operand_block_arg_type(%arg0: tensor) { - // expected-error @+1 {{expected type of bb argument #0 ('i1') to match element or self type of the corresponding operand ('f32')}} + // expected-error @+6 {{'linalg.yield' op type of yield operand 1 ('i1') doesn't match the element type of the enclosing linalg.generic op ('f32')}} linalg.generic { indexing_maps = [ affine_map<() -> ()> ], iterator_types = []} @@ -284,15 +284,14 @@ // ----- -func @generic(%arg0: memref) { - // expected-error @+2 {{op expects regions to end with 'linalg.yield', found 'arith.addf'}} - // expected-note @+1 {{in custom textual format, the absence of terminator implies 'linalg.yield'}} +func @generic(%arg0: memref) { + // expected-error @+6 {{block with no terminator, has %0 = "arith.addf"(%arg1, %arg1) : (f32, f32) -> f32}} linalg.generic { indexing_maps = [ affine_map<(i, j) -> (i, j)> ], iterator_types = ["parallel", "parallel"]} - outs(%arg0 : memref) { - ^bb(%0: i4) : - %1 = arith.addf %0, %0: i4 + outs(%arg0 : memref) { + ^bb(%0: f32) : + %1 = arith.addf %0, %0: f32 } return } diff --git a/mlir/test/Dialect/PDL/invalid.mlir b/mlir/test/Dialect/PDL/invalid.mlir --- a/mlir/test/Dialect/PDL/invalid.mlir +++ b/mlir/test/Dialect/PDL/invalid.mlir @@ -159,7 +159,7 @@ // expected-error@below {{expected body to terminate with `pdl.rewrite`}} pdl.pattern : benefit(1) { // expected-note@below {{see terminator defined here}} - return + "test.finish" () : () -> () } // ----- diff --git a/mlir/test/Dialect/SCF/invalid.mlir b/mlir/test/Dialect/SCF/invalid.mlir --- a/mlir/test/Dialect/SCF/invalid.mlir +++ b/mlir/test/Dialect/SCF/invalid.mlir @@ -241,7 +241,7 @@ %zero = arith.constant 0.0 : f32 %res = scf.parallel (%i0) = (%arg0) to (%arg0) step (%arg0) init (%zero) -> f32 { - // expected-error@+1 {{the block inside reduce should not be empty}} + // expected-error@+1 {{empty block: expect at least a terminator}} scf.reduce(%arg1) : f32 { ^bb0(%lhs : f32, %rhs : f32): } @@ -289,7 +289,7 @@ // expected-error@+1 {{the block inside reduce should be terminated with a 'scf.reduce.return' op}} scf.reduce(%arg1) : f32 { ^bb0(%lhs : f32, %rhs : f32): - scf.yield + "test.finish" () : () -> () } } return diff --git a/mlir/test/Dialect/Tensor/invalid.mlir b/mlir/test/Dialect/Tensor/invalid.mlir --- a/mlir/test/Dialect/Tensor/invalid.mlir +++ b/mlir/test/Dialect/Tensor/invalid.mlir @@ -91,8 +91,7 @@ func @tensor.generate(%m : index, %n : index) -> tensor { - // expected-error @+2 {{op expects regions to end with 'tensor.yield', found 'func.return'}} - // expected-note @+1 {{in custom textual format, the absence of terminator implies 'tensor.yield'}} + // expected-error @+4 {{'func.return' op expects parent op 'builtin.func'}} %tnsr = tensor.generate %m, %n { ^bb0(%i : index, %j : index, %k : index): %elem = arith.constant 8.0 : f32 @@ -377,4 +376,4 @@ // expected-error@+1 {{must be integer/index/float type}} %w = tensor.splat %v : tensor<8xvector<8xf32>> return -} \ No newline at end of file +} diff --git a/mlir/test/IR/invalid.mlir b/mlir/test/IR/invalid.mlir --- a/mlir/test/IR/invalid.mlir +++ b/mlir/test/IR/invalid.mlir @@ -536,8 +536,7 @@ func @return_inside_loop() { affine.for %i = 1 to 100 { - // expected-error@-1 {{op expects regions to end with 'affine.yield', found 'func.return'}} - // expected-note@-2 {{in custom textual format, the absence of terminator implies}} + // expected-error@+1 {{'func.return' op expects parent op 'builtin.func'}} return } return diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp --- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp +++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp @@ -2297,10 +2297,6 @@ if (def.getValueAsBit("hasVerifier")) { auto *method = opClass.declareMethod("::mlir::LogicalResult", "verify"); ERROR_IF_PRUNED(method, "verify", op); - } else if (def.getValueAsBit("hasRegionVerifier")) { - auto *method = - opClass.declareMethod("::mlir::LogicalResult", "verifyRegions"); - ERROR_IF_PRUNED(method, "verifyRegions", op); } else if (hasCustomVerifyCodeBlock) { auto *method = opClass.addMethod("::mlir::LogicalResult", "verify"); ERROR_IF_PRUNED(method, "verify", op); @@ -2311,6 +2307,12 @@ auto printer = stringInit->getValue().ltrim().rtrim(" \t\v\f\r"); body << " " << tgfmt(printer, &fctx); } + + if (def.getValueAsBit("hasRegionVerifier")) { + auto *method = + opClass.declareMethod("::mlir::LogicalResult", "verifyRegions"); + ERROR_IF_PRUNED(method, "verifyRegions", op); + } } void OpEmitter::genOperandResultVerifier(MethodBody &body,