[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

2176–2177

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

556

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

556

Done.

mlir/include/mlir/IR/OpBase.td

 class PredAttrTrait<string descr, Pred pred>
     : PredTrait<descr, pred>, AttrTrait;
 
 //===----------------------------------------------------------------------===//
 // OpTrait definitions
 //===----------------------------------------------------------------------===//
 
 // OpTrait represents a trait regarding an operation.
-// TODO: Remove this class in favor of using Trait.
-class OpTrait;
+class OpTrait<list<OpTrait> traits = []> {
+  // Specify the list of traits that need to be verified before the verification
+  // of this OpTrait.
+  list<OpTrait> dependentTraits = traits;
+}
 
 // Define a OpTrait corresponding to a list of OpTraits, this allows for
 // specifying a list of traits as trait. Avoids needing to do
 // `[Traits, ...] # ListOfTraits # [Others, ...]` while still allowing providing
 // convenient groupings.
 class OpTraitList<list<OpTrait> props> : OpTrait {
   list<OpTrait> traits = props;
 }
 
 // These classes are used to define operation specific traits.
-class NativeOpTrait<string name> : NativeTrait<name, "Op">, OpTrait;
-class ParamNativeOpTrait<string prop, string params>
-    : ParamNativeTrait<prop, params, "Op">, OpTrait;
-class GenInternalOpTrait<string prop> : GenInternalTrait<prop, "Op">, OpTrait;
-class PredOpTrait<string descr, Pred pred> : PredTrait<descr, pred>, OpTrait;
+class NativeOpTrait<string name, list<OpTrait> traits = []>
+    : NativeTrait<name, "Op">, OpTrait<traits> {
+}
+class ParamNativeOpTrait<string prop, string params, list<OpTrait> traits = []>
+    : ParamNativeTrait<prop, params, "Op">, OpTrait<traits>;
+class GenInternalOpTrait<string prop, list<OpTrait> traits = []>
+    : GenInternalTrait<prop, "Op">, OpTrait<traits>;
+class PredOpTrait<string descr, Pred pred, list<OpTrait> traits = []>
+    : PredTrait<descr, pred>, OpTrait<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 = [{}]>
-    : InterfaceTrait<name>, OpTrait {
+class OpInterfaceTrait<string name, code verifyBody = [{}],
+                       list<OpTrait> traits = []>
+    : InterfaceTrait<name>, OpTrait<traits> {
   // Specify the body of the verification function. `$_op` will be replaced with
   // the operation being verified.
   code verify = verifyBody;
 }
 
 // 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.

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());
 
+    auto verifyTraitValidity = [&](Record *trait) {
+      if (!trait->isSubClassOf("OpTrait"))
+        PrintFatalError(def.getLoc(), trait->getValueAsString("trait") +
+                                          " is not an OpTrait");
+
+      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 be declared before itself");
+    };
+
     std::function<void(llvm::ListInit *)> insert;
     insert = [&](llvm::ListInit *traitList) {
       for (auto *traitInit : *traitList) {
         auto *def = cast<DefInit>(traitInit)->getDef();
         if (def->isSubClassOf("OpTraitList")) {
           insert(def->getValueAsListInit("traits"));
           continue;
         }
+
+        // Verify if the trait is an op trait and has all the dependent traits
+        // come 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 be declared before itself
+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 be declared before itself
+def OpInterfaceWithoutDependentTrait : Op<Test_Dialect, "default_value", [OpInterfaceB]> {}
+#endif