From 3c833344c858ae8af38fbd6f20ce9f07a685c15f Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Thu, 4 Apr 2019 04:29:58 -0700 Subject: [PATCH] [TableGen] Rework verifier generation and error messages Previously we bundle the existence check and the MLIR attribute kind check in one call. Further constraints (like element bitwidth) have to be split into following checks. That is not a nice separation given that we have more checks for constraints. Instead, this CL changes to generate a local variable for every attribute, check its existence first, then check the constraints. Creating a local variable for each attribute also avoids querying it multiple times using the raw getAttr() API. This is a win for both performance the readability of the generated code. This CL also changed the error message to be more greppable by delimiting the error message from constraints with boilerplate part with colon. -- PiperOrigin-RevId: 241906132 --- mlir/include/mlir/TableGen/Operator.h | 3 + mlir/lib/TableGen/Operator.cpp | 6 ++ mlir/test/mlir-tblgen/attr-enum.td | 5 +- mlir/test/mlir-tblgen/op-attribute.td | 15 ++++ mlir/test/mlir-tblgen/predicate.td | 87 +++++++++++---------- mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | 39 +++++---- 6 files changed, 90 insertions(+), 65 deletions(-) diff --git a/mlir/include/mlir/TableGen/Operator.h b/mlir/include/mlir/TableGen/Operator.h index a4b702ab3db0..64509117db3a 100644 --- a/mlir/include/mlir/TableGen/Operator.h +++ b/mlir/include/mlir/TableGen/Operator.h @@ -119,6 +119,9 @@ public: Argument getArg(int index); StringRef getArgName(int index) const; + // Returns the number of `PredOpTrait` traits. + int getNumPredOpTraits() const; + // Returns true if this op has the given MLIR C++ `trait`. // TODO: We should add a C++ wrapper class for TableGen OpTrait instead of // requiring the raw MLIR trait here. diff --git a/mlir/lib/TableGen/Operator.cpp b/mlir/lib/TableGen/Operator.cpp index e6453a554360..9c93105143d7 100644 --- a/mlir/lib/TableGen/Operator.cpp +++ b/mlir/lib/TableGen/Operator.cpp @@ -99,6 +99,12 @@ StringRef tblgen::Operator::getArgName(int index) const { return argumentValues->getArgName(index)->getValue(); } +int tblgen::Operator::getNumPredOpTraits() const { + return std::count_if(traits.begin(), traits.end(), [](const OpTrait &trait) { + return isa(&trait); + }); +} + bool tblgen::Operator::hasTrait(StringRef trait) const { for (auto t : getTraits()) { if (auto opTrait = dyn_cast(&t)) { diff --git a/mlir/test/mlir-tblgen/attr-enum.td b/mlir/test/mlir-tblgen/attr-enum.td index 52b88cf44bdb..6d27985256f0 100644 --- a/mlir/test/mlir-tblgen/attr-enum.td +++ b/mlir/test/mlir-tblgen/attr-enum.td @@ -20,8 +20,9 @@ def NS_OpA : Op<"op_a_with_enum_attr", []> { // DEF-NEXT: return attr.getValue(); // DEF-LABEL: OpA::verify() -// DEF: if (!(((this->getAttr("attr").cast().getValue() == "A")) || ((this->getAttr("attr").cast().getValue() == "B")) || ((this->getAttr("attr").cast().getValue() == "C")))) -// DEF-SAME: return emitOpError("attribute 'attr' failed to satisfy some enum attribute constraints"); +// DEF: auto tblgen_attr = this->getAttr("attr"); +// DEF: if (!(((tblgen_attr.cast().getValue() == "A")) || ((tblgen_attr.cast().getValue() == "B")) || ((tblgen_attr.cast().getValue() == "C")))) +// DEF-SAME: return emitOpError("attribute 'attr' failed to satisfy constraint: some enum"); def NS_OpB : Op<"op_b_with_enum_attr", []> { let arguments = (ins NS_SomeEnum:$attr); diff --git a/mlir/test/mlir-tblgen/op-attribute.td b/mlir/test/mlir-tblgen/op-attribute.td index 349e9019a7ea..6c81dc6a6bfe 100644 --- a/mlir/test/mlir-tblgen/op-attribute.td +++ b/mlir/test/mlir-tblgen/op-attribute.td @@ -50,6 +50,21 @@ def AOp : Op<"a_op", []> { // CHECK: for (const auto& pair : attributes) // CHECK-NEXT: tblgen_state->addAttribute(pair.first, pair.second); +// Test verify method +// --- + +// CHECK: AOp::verify() +// CHECK: auto tblgen_aAttr = this->getAttr("aAttr"); +// CHECK-NEXT: if (!tblgen_aAttr) return emitOpError("requires attribute 'aAttr'"); +// CHECK: if (!((some-condition))) return emitOpError("attribute 'aAttr' failed to satisfy constraint: some attribute kind"); +// CHECK: auto tblgen_bAttr = this->getAttr("bAttr"); +// CHECK-NEXT: if (tblgen_bAttr) { +// CHECK-NEXT: if (!((some-condition))) return emitOpError("attribute 'bAttr' failed to satisfy constraint: some attribute kind"); +// CHECK: auto tblgen_cAttr = this->getAttr("cAttr"); +// CHECK-NEXT: if (tblgen_cAttr) { +// CHECK-NEXT: if (!((some-condition))) return emitOpError("attribute 'cAttr' failed to satisfy constraint: some attribute kind"); + + def MixOperandsAndAttrs : Op<"mix_operands_and_attrs", []> { let arguments = (ins F32Attr:$attr, F32:$operand, F32Attr:$otherAttr, F32:$otherArg); } diff --git a/mlir/test/mlir-tblgen/predicate.td b/mlir/test/mlir-tblgen/predicate.td index d2fef04eb15a..fc2ab3fb6d22 100644 --- a/mlir/test/mlir-tblgen/predicate.td +++ b/mlir/test/mlir-tblgen/predicate.td @@ -4,66 +4,69 @@ include "mlir/IR/OpBase.td" def I32OrF32 : Type, "32-bit integer or floating-point type">; -def I32OrF32Tensor : TypedTensor; -def AllOfPredOp : Op<"identity_i32", [PredOpTrait<"both first and second holds", - AllOf<[CPred<"first">, CPred<"second">]>>]> { - let arguments = (ins Tensor:$x); - let results = (outs Tensor:$y); +def OpA : Op<"op_for_CPred_containing_multiple_same_placeholder", []> { + let arguments = (ins I32OrF32:$x); } -def TautologyElementTest: PredOpTrait< - "first operand is a vector or tensor with the same elemental type as itself", - TCopVTEtIsSameAs<0, 0>>; +// CHECK-LABEL: OpA::verify +// CHECK: if (!((this->getOperation()->getOperand(0)->getType().isInteger(32) || this->getOperation()->getOperand(0)->getType().isF32()))) -def OperandAndResultElementTypeTest : PredOpTrait< - "first operand is a vector or tensor with the same elemental type as first result", - TCresVTEtIsSameAsOp<0, 0>>; - -def Identity : Op<"identity", [TautologyElementTest, OperandAndResultElementTypeTest]> { - let arguments = (ins - I32OrF32Tensor:$x); - let results = (outs - I32OrF32Tensor:$y); +def OpB : Op<"op_for_AllOf_PredOpTrait", [ + PredOpTrait<"both first and second holds", + AllOf<[CPred<"first">, CPred<"second">]>>]> { } -def IdentityI32 : Op<"identity_i32", [PredOpTrait< - "first operand has i32 element type", TCopVTEtIs<0, I32>>]> { - let arguments = (ins Tensor:$x); - let results = (outs Tensor:$y); -} - -// CHECK-LABEL: AllOfPredOp::verify +// CHECK-LABEL: OpB::verify // CHECK: if (!(((first)) && ((second)))) -// CHECK-LABEL: Identity::verify -// Verify arg constraints. -// CHECK: this->getOperation()->getOperand(0)->getType().cast().getElementType().isInteger(32) || -// CHECK-SAME: this->getOperation()->getOperand(0)->getType().cast().getElementType().isF32() -// Verify tautology constraint. +def OpC : Op<"op_for_TCopVTEtIs", [ + PredOpTrait<"first operand has i32 element type", + TCopVTEtIs<0, I32>>]> { + let arguments = (ins Tensor:$x); +} + +// CHECK-LABEL: OpC::verify +// CHECK: if (!((((*this->getOperation()).getNumOperands() > 0)) && (((*this->getOperation()).getOperand(0)->getType().isa())) && (((*this->getOperation()).getOperand(0)->getType().cast().getElementType().isInteger(32))))) + + +def OpD : Op<"op_for_TCOpVTEtIsSameAs", [ + PredOpTrait<"first operand is a vector or tensor with the same " + "elemental type as itself", + TCopVTEtIsSameAs<0, 0>>]> { + let arguments = (ins Tensor:$x); +} + +// CHECK-LABEL: OpD::verify // CHECK: if (!((((*this->getOperation()).getNumOperands() > std::max(0,0))) && (((*this->getOperation()).getOperand(0)->getType().isa())) && (((*this->getOperation()).getOperand(0)->getType().isa())) && (((*this->getOperation()).getOperand(0)->getType().cast().getElementType() == (*this->getOperation()).getOperand(0)->getType().cast().getElementType())))) // CHECK-NEXT: return emitOpError("failed to verify that first operand is a vector or tensor with the same elemental type as itself"); -// Verify OperandAndResultElementTypeTest constraint + + +def OpE : Op<"op_for_TCresVTEtIsSameAsOp", [ + PredOpTrait<"first operand is a vector or tensor with the same " + "elemental type as first result", + TCresVTEtIsSameAsOp<0, 0>>]> { + let arguments = (ins Tensor:$x); + let results = (outs Tensor:$y); +} + +// CHECK-LABEL: OpE::verify // CHECK: if (!((((*this->getOperation()).getNumResults() > 0)) && (((*this->getOperation()).getNumOperands() > 0)) && (((*this->getOperation()).getResult(0)->getType().isa())) && (((*this->getOperation()).getOperand(0)->getType().isa())) && (((*this->getOperation()).getResult(0)->getType().cast().getElementType() == (*this->getOperation()).getOperand(0)->getType().cast().getElementType())))) // CHECK-NEXT: return emitOpError("failed to verify that first operand is a vector or tensor with the same elemental type as first result"); -// CHECK-LABEL: IdentityI32::verify -// CHECK: if (!((((*this->getOperation()).getNumOperands() > 0)) && (((*this->getOperation()).getOperand(0)->getType().isa())) && (((*this->getOperation()).getOperand(0)->getType().cast().getElementType().isInteger(32))))) -// CHECK-NEXT: return emitOpError("failed to verify that first operand has i32 element type"); -def OpA : Op<"op_for_int_min_val", []> { +def OpF : Op<"op_for_int_min_val", []> { let arguments = (ins Confined]>:$attr); } -// CHECK-LABEL: OpA::verify() -// CHECK: if (!(((true)) && ((this->getAttr("attr").cast().getInt() >= 10)))) -// CHECK-SAME: return emitOpError("attribute 'attr' failed to satisfy 32-bit integer whose minimal value is 10 attribute constraints"); +// CHECK-LABEL: OpF::verify() +// CHECK: (tblgen_attr.cast().getInt() >= 10) +// CHECK-SAME: return emitOpError("attribute 'attr' failed to satisfy constraint: 32-bit integer whose minimal value is 10"); -def OpB : Op<"op_for_arr_min_count", []> { +def OpG : Op<"op_for_arr_min_count", []> { let arguments = (ins Confined]>:$attr); } -// CHECK-LABEL: OpB::verify() -// CHECK: if (!(((true)) && ((this->getAttr("attr").cast().size() >= 8)))) -// CHECK-SAME: return emitOpError("attribute 'attr' failed to satisfy array with at least 8 elements attribute constraints"); - +// CHECK-LABEL: OpG::verify() +// CHECK: (tblgen_attr.cast().size() >= 8) +// CHECK-SAME: return emitOpError("attribute 'attr' failed to satisfy constraint: array with at least 8 elements"); diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp index 98f03b26a7ef..7c3a6cf7b697 100644 --- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp +++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp @@ -36,8 +36,9 @@ using namespace mlir; using mlir::tblgen::Operator; +static const char *const tblgenNamePrefix = "tblgen_"; +static const char *const generatedArgName = "tblgen_arg"; static const char *const builderOpState = "tblgen_state"; -static const char *const generatedArgName = "_arg"; static const char *const opCommentHeader = R"( //===----------------------------------------------------------------------===// @@ -785,7 +786,8 @@ void OpEmitter::genVerifier() { auto valueInit = def.getValueInit("verifier"); CodeInit *codeInit = dyn_cast(valueInit); bool hasCustomVerify = codeInit && !codeInit->getValue().empty(); - if (!hasCustomVerify && op.getNumArgs() == 0 && op.getNumResults() == 0) + if (!hasCustomVerify && op.getNumArgs() == 0 && op.getNumResults() == 0 && + op.getNumPredOpTraits() == 0) return; auto &method = opClass.newMethod("LogicalResult", "verify", /*params=*/""); @@ -798,38 +800,33 @@ void OpEmitter::genVerifier() { if (attr.isDerivedAttr()) continue; - auto name = namedAttr.getName(); - if (!attr.hasStorageType() && !attr.hasDefaultValue()) { - // TODO: Some verification can be done even without storage type. - body << " if (!this->getAttr(\"" << name - << "\")) return emitOpError(\"requires attribute '" << name - << "'\");\n"; - continue; - } + auto attrName = namedAttr.getName(); + // Prefix with `tblgen_` to avoid hiding the attribute accessor. + auto varName = tblgenNamePrefix + namedAttr.name; + body << formatv(" auto {0} = this->getAttr(\"{1}\");\n", varName, + attrName); bool allowMissingAttr = attr.hasDefaultValue() || attr.isOptional(); if (allowMissingAttr) { // If the attribute has a default value, then only verify the predicate if // set. This does effectively assume that the default value is valid. // TODO: verify the debug value is valid (perhaps in debug mode only). - body << " if (this->getAttr(\"" << name << "\")) {\n"; + body << " if (" << varName << ") {\n"; + } else { + body << " if (!" << varName + << ") return emitOpError(\"requires attribute '" << attrName + << "'\");\n {\n"; } - body << " if (!this->getAttr(\"" << name << "\").dyn_cast_or_null<" - << attr.getStorageType() << ">()) return emitOpError(\"requires " - << attr.getDescription() << " attribute '" << name << "'\");\n"; - auto attrPred = attr.getPredicate(); if (!attrPred.isNull()) { body << formatv(" if (!({0})) return emitOpError(\"attribute '{1}' " - "failed to satisfy {2} attribute constraints\");\n", - formatv(attrPred.getCondition(), - formatv("this->getAttr(\"{0}\")", name)), - name, attr.getDescription()); + "failed to satisfy constraint: {2}\");\n", + formatv(attrPred.getCondition(), varName), attrName, + attr.getDescription()); } - if (allowMissingAttr) - body << " }\n"; + body << " }\n"; } // Emits verification code for an operand or result.