[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

@@ -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.

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<tblgen::PredOpTrait>(&trait);
+  });
+}
+
 bool tblgen::Operator::hasTrait(StringRef trait) const {
   for (auto t : getTraits()) {
     if (auto opTrait = dyn_cast<tblgen::NativeOpTrait>(&t)) {

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<StringAttr>().getValue() == "A")) || ((this->getAttr("attr").cast<StringAttr>().getValue() == "B")) || ((this->getAttr("attr").cast<StringAttr>().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<StringAttr>().getValue() == "A")) || ((tblgen_attr.cast<StringAttr>().getValue() == "B")) || ((tblgen_attr.cast<StringAttr>().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);

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);
 }

mlir/test/mlir-tblgen/predicate.td

@@ -4,66 +4,69 @@ include "mlir/IR/OpBase.td"
 
 def I32OrF32 : Type<CPred<"{0}.isInteger(32) || {0}.isF32()">,
                     "32-bit integer or floating-point type">;
-def I32OrF32Tensor : TypedTensor<I32OrF32>;
 
-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<TensorType>().getElementType().isInteger(32) ||
-// CHECK-SAME: this->getOperation()->getOperand(0)->getType().cast<TensorType>().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<VectorOrTensorType>())) && (((*this->getOperation()).getOperand(0)->getType().cast<VectorOrTensorType>().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<VectorOrTensorType>())) && (((*this->getOperation()).getOperand(0)->getType().isa<VectorOrTensorType>())) && (((*this->getOperation()).getOperand(0)->getType().cast<VectorOrTensorType>().getElementType() == (*this->getOperation()).getOperand(0)->getType().cast<VectorOrTensorType>().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<VectorOrTensorType>())) && (((*this->getOperation()).getOperand(0)->getType().isa<VectorOrTensorType>())) && (((*this->getOperation()).getResult(0)->getType().cast<VectorOrTensorType>().getElementType() == (*this->getOperation()).getOperand(0)->getType().cast<VectorOrTensorType>().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<VectorOrTensorType>())) && (((*this->getOperation()).getOperand(0)->getType().cast<VectorOrTensorType>().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<I32Attr, [IntMinValue<10>]>:$attr);
 }
 
-// CHECK-LABEL: OpA::verify()
-// CHECK:       if (!(((true)) && ((this->getAttr("attr").cast<IntegerAttr>().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<IntegerAttr>().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<ArrayAttr, [ArrayMinCount<8>]>:$attr);
 }
 
-// CHECK-LABEL: OpB::verify()
-// CHECK:       if (!(((true)) && ((this->getAttr("attr").cast<ArrayAttr>().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<ArrayAttr>().size() >= 8)
+// CHECK-SAME:    return emitOpError("attribute 'attr' failed to satisfy constraint: array with at least 8 elements");

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<CodeInit>(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.