[mlir] Support verification order (1/3)

[Chia-hungDuan]

Summary

This CL supports adding dependency between traits verifiers and the
dependency will be checked statically.

[Chia-hungDuan]

Upload a WIP CL for showing the concept I want to do, let's have discussion based on this change.

[jpienaar]

mlir/include/mlir/IR/OpBase.td

2187–2188

Could we make traits more self-documenting?

mlir/include/mlir/Support/InterfaceSupport.h

71 ↗

(On Diff #391984)

I'd rather prefer we generate the static asserts from ODS when generating interfaces than add to the class template (previously we havent had need to generate traits as there was not much more than verify method, that could be revised). I think this would allow simpler error messages/more control, simpler code and earlier flagging of issues. It would still be possible to add a static assert manually if not using generated code.

Could you try that and see what it looks like?

[Chia-hungDuan]

Refine the template structure and added test. Will add doc for this.
Other parts can start the review.

[Chia-hungDuan]

@jpienaar , Sorry I didn't notice your comment until uploaded a new revision. Will address them later.

[Chia-hungDuan]

Check the dependency from tblgen instead.

[jpienaar]

Nice, thanks for looking at the alternative specification (I think this is much simpler with nice error messages early in build process)

mlir/lib/TableGen/Operator.cpp

564

declared before could be overloaded here (the definition of the trait itself in the file vs the order listed on the op). Don't have too good a suggestion here for working. But let's add a comment with an example here: if this failure is hit and folks search in the code they can see an example.

(Perhaps the wording of the error could be "trait X requires trait Y to precede it in traits list")

[Chia-hungDuan]

Rebase and address the review comments

mlir/lib/TableGen/Operator.cpp

564

Done.

mlir/include/mlir/IR/OpBase.td

 class GenInternalAttrTrait<string prop> : GenInternalTrait<prop, "Attribute">;
 class PredAttrTrait<string descr, Pred pred> : PredTrait<descr, pred>;
 
 //===----------------------------------------------------------------------===//
 // OpTrait definitions
 //===----------------------------------------------------------------------===//
 
 // These classes are used to define operation specific traits.
-class NativeOpTrait<string name> : NativeTrait<name, "Op">;
-class ParamNativeOpTrait<string prop, string params>
-    : ParamNativeTrait<prop, params, "Op">;
-class GenInternalOpTrait<string prop> : GenInternalTrait<prop, "Op">;
-class PredOpTrait<string descr, Pred pred> : PredTrait<descr, pred>;
+class NativeOpTrait<string name, list<Trait> traits = []>
+    : NativeTrait<name, "Op"> {
+  // Specify the list of traits that need to be verified before the verification
+  // of this NativeOpTrait.
+  list<Trait> dependentTraits = traits;
+}
+class ParamNativeOpTrait<string prop, string params,
+                         list<Trait> traits = []>
+    : ParamNativeTrait<prop, params, "Op"> {
+  // Specify the list of traits that need to be verified before the verification
+  // of this ParamNativeOpTrait.
+  list<Trait> dependentTraits = traits;
+}
+class GenInternalOpTrait<string prop, list<Trait> traits = []>
+    : GenInternalTrait<prop, "Op"> {
+  // Specify the list of traits that need to be verified before the verification
+  // of this GenInternalOpTrait.
+  list<Trait> dependentTraits = traits;
+}
+class PredOpTrait<string descr, Pred pred, list<Trait> traits = []>
+    : PredTrait<descr, pred> {
+  // Specify the list of traits that need to be verified before the verification
+  // of this PredOpTrait.
+  list<Trait> dependentTraits = traits;
+}
 
 // Op defines an affine scope.
 def AffineScope : NativeOpTrait<"AffineScope">;
 // Op defines an automatic allocation scope.
 def AutomaticAllocationScope : NativeOpTrait<"AutomaticAllocationScope">;
 // Op supports operand broadcast behavior.
 def ResultsBroadcastableShape :
   NativeOpTrait<"ResultsBroadcastableShape">;
class InterfaceTrait<string name> : NativeTrait<"", ""> {
   // An optional code block containing extra declarations to place in the
   // interface trait declaration.
   code extraTraitClassDeclaration = "";
 }
 
 // OpInterfaceTrait corresponds to a specific 'OpInterface' class defined in
 // C++. The purpose to wrap around C++ symbol string with this class is to make
 // interfaces specified for ops in TableGen less alien and more integrated.
-class OpInterfaceTrait<string name, code verifyBody = [{}]>
+class OpInterfaceTrait<string name, code verifyBody = [{}],
+                       list<Trait> traits = []>
     : InterfaceTrait<name> {
   // Specify the body of the verification function. `$_op` will be replaced with
   // the operation being verified.
   code verify = verifyBody;
+
+  // Specify the list of traits that need to be verified before the verification
+  // of this OpInterfaceTrait.
+  list<Trait> dependentTraits = traits;
 }
 
 // This class represents a single, optionally static, interface method.
 // Note: non-static interface methods have an implicit parameter, either
 // $_op/$_attr/$_type corresponding to an instance of the derived value.
 class InterfaceMethod<string desc, string retTy, string methodName,
                       dag args = (ins), code methodBody = [{}],
                       code defaultImplementation = [{}]> {

mlir/lib/TableGen/Operator.cpp

void Operator::populateOpStructure() {
 
   // Create list of traits, skipping over duplicates: appending to lists in
   // tablegen is easy, making them unique less so, so dedupe here.
   if (auto *traitList = def.getValueAsListInit("traits")) {
     // This is uniquing based on pointers of the trait.
     SmallPtrSet<const llvm::Init *, 32> traitSet;
     traits.reserve(traitSet.size());
 
+    // The declaration order of traits imply the verification order of traits.
+    // Some traits may require other traits to be verified first then they can
+    // do further verification based on those verified facts. If you see this
+    // error, fix the traits declaration order by checking the `dependentTraits`
+    // field.
+    auto verifyTraitValidity = [&](Record *trait) {
+      auto *dependentTraits = trait->getValueAsListInit("dependentTraits");
+      for (auto *traitInit : *dependentTraits)
+        if (traitSet.find(traitInit) == traitSet.end())
+          PrintFatalError(
+              def.getLoc(),
+              trait->getValueAsString("trait") + " requires " +
+                  cast<DefInit>(traitInit)->getDef()->getValueAsString(
+                      "trait") +
+                  " to precede it in traits list");
+    };
+
     std::function<void(llvm::ListInit *)> insert;
     insert = [&](llvm::ListInit *traitList) {
       for (auto *traitInit : *traitList) {
         auto *def = cast<DefInit>(traitInit)->getDef();
         if (def->isSubClassOf("TraitList")) {
           insert(def->getValueAsListInit("traits"));
           continue;
         }
+
+        // Verify if the trait has all the dependent traits declared before
+        // itself.
+        verifyTraitValidity(def);
+
         // Keep traits in the same order while skipping over duplicates.
         if (traitSet.insert(traitInit).second)
           traits.push_back(Trait::create(traitInit));
       }
     };
     insert(traitList);
   }
 

mlir/test/mlir-tblgen/op-error.td

 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR1 %s 2>&1 | FileCheck --check-prefix=ERROR1 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR2 %s 2>&1 | FileCheck --check-prefix=ERROR2 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR3 %s 2>&1 | FileCheck --check-prefix=ERROR3 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR4 %s 2>&1 | FileCheck --check-prefix=ERROR4 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR5 %s 2>&1 | FileCheck --check-prefix=ERROR5 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR6 %s 2>&1 | FileCheck --check-prefix=ERROR6 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR7 %s 2>&1 | FileCheck --check-prefix=ERROR7 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR8 %s 2>&1 | FileCheck --check-prefix=ERROR8 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR9 %s 2>&1 | FileCheck --check-prefix=ERROR9 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR10 %s 2>&1 | FileCheck --check-prefix=ERROR10 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR11 %s 2>&1 | FileCheck --check-prefix=ERROR11 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR12 %s 2>&1 | FileCheck --check-prefix=ERROR12 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR13 %s 2>&1 | FileCheck --check-prefix=ERROR13 %s
 
 include "mlir/IR/OpBase.td"
 
 def Test_Dialect : Dialect {
   let name = "test_dialect";
 }
 
 #ifdef ERROR1

 
 #ifdef ERROR11
 // ERROR11: error: op has a conflict with regions and successors both having an entry with the name 'target'
 def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
   let successors = (successor AnySuccessor:$target);
   let regions = (region AnyRegion:$target);
 }
 #endif
+
+#ifdef ERROR12
+def OpTraitA : NativeOpTrait<"OpTraitA"> {}
+def OpTraitB : NativeOpTrait<"OpTraitB", [OpTraitA]> {}
+
+// ERROR12: error: OpTraitB requires OpTraitA to precede it in traits list
+def OpTraitWithoutDependentTrait : Op<Test_Dialect, "default_value", [OpTraitB]> {}
+#endif
+
+#ifdef ERROR13
+def OpTraitA : NativeOpTrait<"OpTraitA"> {}
+def OpInterfaceB : OpInterface<"OpInterfaceB"> {
+  let dependentTraits = [OpTraitA];
+}
+
+// ERROR13: error: OpInterfaceB::Trait requires OpTraitA to precede it in traits list
+def OpInterfaceWithoutDependentTrait : Op<Test_Dialect, "default_value", [OpInterfaceB]> {}
+#endif