[mlir] Add OpAdaptorVerifier trait

[jpienaar]

Summary

Enables using OpAdaptor's verifier as trait. This allows for reordering
checking of OpAdaptor verification. Currently this verify method is
emitted in verify method via ODS which results in it being invoked last.
The basic verification of that the OpAdaptor does would be more
beneficial earlier and could rather always be verified first (e.g.,
automatically prepend OpAdaptorVerifier to traits in ODS) but this
change merely adds the trait pending discussion on alternatives to
achieve the above (which is also why its added only inside TestOps.td).

[bondhugula]

mlir/include/mlir/IR/OpDefinition.h

294

I think this is not placed at the right point in this file and is also missing the banner comment. Below, you have: "Operand traits", "Result traits", "Misc traits", etc. Could you please put them under one of those or introduce a new one.

294

Could you please also expand the doc comment a bit to make it self-contained? Your commit summary actually has some information, but things aren't clear from this.

[jpienaar]

Added heading and expanded comment

[rriddle]

Is there more planned here? Seems like uses of this trait right now will result in double verification + double code size?

[jpienaar]

In D97622#2606872, @rriddle wrote:

Is there more planned here? Seems like uses of this trait right now will result in double verification + double code size?

Correct (double verification, I don't think double code size as both calls the same verify method, but the compiler could inline). Next step would be to have ODS generate this as its first trait rather than invoking it in verify method generated. Now, this will result in many errors getting reported in different order and needing to update tests. I wanted to have this added/discussed in isolation before updating the tests (this only affects the one test op redundantly for now). And then the checking could be removed from type infrence methods using the adaptor verify manually today too (so that type inference can use attributes that are supposed to be specified).

The adaptor generated is also not as complete as it should be (it doesn't verify regions yet, nor does it verify as much of the operands as it could). So I'd want that more complete too.

[bondhugula]

Thanks. Please go with @rriddle's comments for the rest.

[jpienaar]

Avoid verifying twice OpAdaptor verification if specified

mlir/include/mlir/IR/OpBase.td

 def AttrSizedOperandSegments : NativeOpTrait<"AttrSizedOperandSegments">;
 // Similar to AttrSizedOperandSegments, but used for results. The attribute
 // should be named as `result_segment_sizes`.
 def AttrSizedResultSegments  : NativeOpTrait<"AttrSizedResultSegments">;
 
 // Op attached regions have no arguments
 def NoRegionArguments : NativeOpTrait<"NoRegionArguments">;
 
+// Verification using the generated OpAdaptor's verify method. This verification
+// is done in the generated verify method unless this trait is specified.
+def OpAdaptorVerifier : NativeOpTrait<"OpAdaptorVerifier">;
+
 //===----------------------------------------------------------------------===//
 // OpInterface definitions
 //===----------------------------------------------------------------------===//
 
 // Marker used to identify the argument list for an op or interface method.
 def ins;
 
 // This class represents a typed argument with optional default value for C

mlir/include/mlir/IR/OpDefinition.h

Operation *getOperation() {
     // be able to disambiguate the path for the C++ compiler.
     auto *trait = static_cast<TraitType<ConcreteType> *>(this);
     auto *concrete = static_cast<ConcreteType *>(trait);
     auto *base = static_cast<OpState *>(concrete);
     return base->getOperation();
   }
 };
 
 //===----------------------------------------------------------------------===//
+// Traits spanning Attributes, Operands & Regions
+
+/// This class provides verification for ops using their OpAdaptor. OpAdaptor
+/// is created automatically for ops defined via ODS. The OpAdaptor verify
+/// method verifies aspects of the op such as required named attributes and
+/// number of operands.
+///
+/// This verification is normally done post all other verifications unless this
+/// trait is used.
+template <typename ConcreteType>
+class OpAdaptorVerifier : public TraitBase<ConcreteType, OpAdaptorVerifier> {
+public:
+  static LogicalResult verifyTrait(Operation *op) {
+    return typename ConcreteType::Adaptor(
+               op->getOperands(), op->getAttrDictionary(), op->getRegions())
+        .verify(op->getLoc());
+  }
+};
+
+//===----------------------------------------------------------------------===//
 // Operand Traits
 
 namespace detail {
 /// Utility trait base that provides accessors for derived traits that have
 /// multiple operands.
 template <typename ConcreteType, template <typename> class TraitType>
 struct MultiOperandTraitBase : public TraitBase<ConcreteType, TraitType> {
   using operand_iterator = Operation::operand_iterator;

mlir/test/Dialect/traits.mlir

 
 // -----
 
 func @broadcastDifferentResultType(tensor<4xi32>, tensor<4xi32>) -> tensor<4xi1> {
 ^bb0(%arg0: tensor<4xi32>, %arg1: tensor<4xi32>):
   %0 = "test.broadcastable"(%arg0, %arg1) : (tensor<4xi32>, tensor<4xi32>) -> tensor<4xi1>
   return %0 : tensor<4xi1>
 }
+
+// -----
+
+func @adaptor_verifier(%arg0: tensor<2x2xi32>) -> tensor<4x7xi1> {
+  // expected-error@+1 {{op requires attribute 'names'}}
+  %0 = "test.op_adaptor_verifier"(%arg0) : (tensor<2x2xi32>) -> tensor<4x7xi1>
+  return %0 : tensor<4x7xi1>
+}

mlir/test/lib/Dialect/Test/TestOps.td

: TEST_Op<"string_attr_pretty_name",
            [DeclareOpInterfaceMethods<OpAsmOpInterface>]> {
   let arguments = (ins StrArrayAttr:$names);
   let results = (outs Variadic<I32>:$r);
 
   let printer = [{ return ::print(p, *this); }];
   let parser = [{ return ::parse$cppClass(parser, result); }];
 }
 
+def OpAdaptorVerifierOp : TEST_Op<"op_adaptor_verifier",
+    [
+      OpAdaptorVerifier,
+      ResultsBroadcastableShape
+    ]> {
+  let arguments = (ins TensorOf<[F32]>:$arg1, StrArrayAttr:$names);
+  let results = (outs AnyTensor);
+}
+
 //===----------------------------------------------------------------------===//
 // Test Locations
 //===----------------------------------------------------------------------===//
 
 def TestLocationSrcOp : TEST_Op<"loc_src"> {
   let arguments = (ins I32:$input);
   let results = (outs I32:$output);
 }

mlir/test/mlir-tblgen/op-decl-and-defs.td

 }
 
 def NS_KWithTraitOp : NS_Op<"KWithTrait", [NS_TestTrait]>;
 
 // CHECK-LABEL: NS::KWithTraitOp declarations
 // CHECK:       class KWithTraitOp : public ::mlir::Op<KWithTraitOp
 // CHECK-SAME:      SomeNamespace::TestTrait
 
+// Check OpAdator verify method only called once.
+// ---
+
+def NS_OpAdaptorVerifierOp : NS_Op<"op_adaptor_verifier",
+    [
+      OpAdaptorVerifier,
+      ResultsBroadcastableShape
+    ]> {
+  let arguments = (ins TensorOf<[F32]>:$arg1, StrArrayAttr:$names);
+  let results = (outs AnyTensor);
+}
+
+// CHECK: class OpAdaptorVerifierOp
+// CHECK: ::mlir::OpTrait::OpAdaptorVerifier, ::mlir::OpTrait::ResultsBroadcastableShape
+// DEFS: ::mlir::LogicalResult OpAdaptorVerifierOp::verify() {
+// DEFS-NOT: if (failed(OpAdaptorVerifierOpAdaptor
+
 // Test that type defs have the proper namespaces when used as a constraint.
 // ---
 
 def Test_Dialect2 : Dialect {
   let name = "test";
   let cppNamespace = "::mlir::dialect2";
 }
 def TestDialect2Type : TypeDef<Test_Dialect2, "Dialect2Type">;

 // CHECK-LABEL: _BOp declarations
 // CHECK: class _BOp : public ::mlir::Op<_BOp
 
 // REDUCE_INC-LABEL: NS::AOp declarations
 // REDUCE_INC-NOT: NS::BOp declarations
 
 // REDUCE_EXC-NOT: NS::AOp declarations
 // REDUCE_EXC-LABEL: NS::BOp declarations
+

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

void OpEmitter::genPrinter() {
   fctx.addSubst("cppClass", opClass.getClassName());
   auto printer = stringInit->getValue().ltrim().rtrim(" \t\v\f\r");
   method->body() << "  " << tgfmt(printer, &fctx);
 }
 
 void OpEmitter::genVerifier() {
   auto *method = opClass.addMethodAndPrune("::mlir::LogicalResult", "verify");
   auto &body = method->body();
+  // Verify using the OpAdaptor's verify method unless already verified earlier
+  // by OpAdaptorVerifier.
+  if (op.getTrait("::mlir::OpTrait::OpAdaptorVerifier") == nullptr) {
     body << "  if (failed(" << op.getAdaptorName()
          << "(*this).verify((*this)->getLoc()))) "
          << "return ::mlir::failure();\n";
+  }
 
   auto *valueInit = def.getValueInit("verifier");
   StringInit *stringInit = dyn_cast<StringInit>(valueInit);
   bool hasCustomVerify = stringInit && !stringInit->getValue().empty();
   populateSubstitutions(op, "(*this)->getAttr", "this->getODSOperands",
                         "this->getODSResults", verifyCtx);
 
   genAttributeVerifier(op, "(*this)->getAttr", "emitOpError(",