[mlir][ods] Make Type- and AttrInterfaces also `Type`s and `Attr`s

[zero9178]

Summary

By making TypeInterfaces and AttrInterfaces, Types and Attrs respectively it'd then be possible to use them anywhere where a Type or Attr may go. That is within the arguments and results of an Op definition, in a RewritePattern etc.

Prior to this change users had to separately define a Type or Attr, with a predicate to check whether a type or attribute implements a given interface. Such code will be redundant now.
Removing such occurrences in upstream dialects will be part of a separate patch.

As part of implementing this patch, slight refactoring had to be done. In particular, Interfaces cppClassName field was renamed to cppInterfaceName as it "clashed" with TypeConstraints cppClassName. In particular Interfaces cppClassName expected just the class name, without any namespaces, while TypeConstraints cppClassName expected a fully qualified class name.

[Mogball]

mlir/include/mlir/IR/OpBase.td

1977

Where are the values for cppNamespace and cppClassName coming from?

1988

Can you add some tblgen lit tests for these? In particular, I'm pretty sure cppNamespace will always be empty here.

[zero9178]

• Don't use fully qualified names if cppNamespace is empty, as there exist interfaces that don't use cppNamespace, but instead are included into a namespace
• Add test checking correctly generated constraint code.

I am not 100% sure whether the test is supposed to live in that directory, so please do tell if I should move it elsewhere.

[zero9178]

mlir/include/mlir/IR/OpBase.td

1977

cppClassName was indeed an error of mine from back when Interface still had that field.
cppNamespace comes from Interface and may also be overwritten by the user using let statements.

The reason this works is because we inherit from Interface first. According to the TableGen language reference, classes are inherited and visited from left to right and depth first. The reason cppNamespace is correct here and also works properly even if a user only later sets cppNamespace using a let statement is that let statements are actually evaluated before any expressions of any other fields are evaluated.

See the language reference here: https://llvm.org/docs/TableGen/ProgRef.html#how-records-are-built

I added a test with both a TypeInterface and AttrInterface that set cppNamespace to show that this works.

[Mogball]

mlir/include/mlir/IR/OpBase.td

1977

even if a user only later sets cppNamespace using a let statement is that let statements are actually evaluated before any expressions of any other fields are evaluated.

Cool! I didn't know this is how it actually worked and kept it in my brain as a "TableGen black box full of magic"

mlir/test/mlir-tblgen/interfaces-as-constraints.td

11–13

nit

[Mogball]

mlir/include/mlir/IR/OpBase.td

1975

You can drop the !if(!empty since prepending :: in front of class names with no namespace is what is done elsewhere.

[zero9178]

mlir/include/mlir/IR/OpBase.td

1975

I wasn't too sure about this since there do exist interfaces in upstream that do not make use cppNamespace, but instead just include the header in a namespace of their choice. For those, using ::GlobaName would actually produce broken code. See eg: https://github.com/llvm/llvm-project/blob/f1182bd6d538847acac58316e794f5b60bc3c89a/mlir/include/mlir/Dialect/Tosa/IR/TosaOps.h#L32

If doing that instead of using cppNamespace should be discouraged/unsupported (which I am personally for) I'd very happily change it your suggestion however.

[jpienaar]

SGTM, Attr & Type are constraints effectively and here we have interface behaving as constraint

[rriddle]

mlir/lib/Tools/PDLL/Parser/Parser.cpp

921–922

This doesn't look right, why is this creating OpConstraintDecl for all of them?

[Mogball]

mlir/include/mlir/IR/OpBase.td

1975

Sigh. Guess this is inconsistent as well.

mlir/lib/Tools/PDLL/Parser/Parser.cpp

921–922

This is only iterating over OpInterface now.

[zero9178]

mlir/lib/Tools/PDLL/Parser/Parser.cpp

921–922

I changed the loop to only handle OpInterfaces hence it only creating OpConstraintDecl should be correct the way I read the code. TypeInterfaces and AttrInterfacess are handled via the loops above since they are now subclasses of Type and Attr.

I know very little about PDLL however, so do tell whether them not being specially handled as interfaces has any negative consequences.

[rriddle]

mlir/lib/Tools/PDLL/Parser/Parser.cpp

921–922

This will require adding a check for DeclareInterfaceMethods to the other loops now I think, we don't want to bring any of those in here.

[zero9178]

Address review comments: Make sure to also skip DeclareInterfaceMethods subclasses for Types and Attrs.

mlir/docs/PDLL.md

was imported:
 
   * `Attr` constraints
     - Imported `Attr` constraints utilize the `storageType` field for native type translation.
   
   * `Type` constraints
     - Imported `Type` constraints utilize the `cppClassName` field for native type translation.
 
   * `AttrInterface`/`OpInterface`/`TypeInterface` constraints
-    - Imported interfaces utilize the `cppClassName` field for native type translation.
+    - Imported interfaces utilize the `cppInterfaceName` field for native type translation.
 
 #### Defining Constraints Inline
 
 In addition to global scope, PDLL Constraints and Native Constraints defined in
 PDLL may be specified *inline* at any level of nesting. This means that they may
 be defined in Patterns, other Constraints, Rewrites, etc:
 
 ```pdll

mlir/include/mlir/IR/OpBase.td

: InterfaceMethod<desc, retTy, methodName, args, methodBody,
                       defaultImplementation>;
 
 // Interface represents a base interface.
 class Interface<string name> {
   // A human-readable description of what this interface does.
   string description = "";
 
   // The name given to the c++ interface class.
-  string cppClassName = name;
+  string cppInterfaceName = name;
 
   // The C++ namespace that this interface should be placed into.
   //
   // To specify nested namespaces, use "::" as the delimiter, e.g., given
   // "A::B", ops will be placed in `namespace A { namespace B { <def> } }`.
   string cppNamespace = "";
 
   // The list of methods defined by this interface.
   list<InterfaceMethod> methods = [];
 
   // An optional code block containing extra declarations to place in the
   // interface declaration.
   code extraClassDeclaration = "";
 
   // An optional code block containing extra declarations to place in both
   // the interface and trait declaration.
   code extraSharedClassDeclaration = "";
 }
 
 // AttrInterface represents an interface registered to an attribute.
-class AttrInterface<string name> : Interface<name>, InterfaceTrait<name>;
+class AttrInterface<string name> : Interface<name>, InterfaceTrait<name>,
+  Attr<CPred<"$_self.isa<"
+    # !if(!empty(cppNamespace),"", cppNamespace # "::") # name # ">()">,
+      name # " instance">
+{
+  let storageType = !if(!empty(cppNamespace), "", cppNamespace # "::") # name;
+  let returnType = storageType;
+  let convertFromStorage = "$_self";
+}
 
 // OpInterface represents an interface registered to an operation.
 class OpInterface<string name> : Interface<name>, OpInterfaceTrait<name>;
 
 // TypeInterface represents an interface registered to a type.
-class TypeInterface<string name> : Interface<name>, InterfaceTrait<name>;
+class TypeInterface<string name> : Interface<name>, InterfaceTrait<name>,
+  Type<CPred<"$_self.isa<"
+    # !if(!empty(cppNamespace),"", cppNamespace # "::") # name # ">()">,
+      name # " instance",
+        !if(!empty(cppNamespace),"", cppNamespace # "::") # name>;
 
 // Whether to declare the interface methods in the user entity's header. This
 // class simply wraps an Interface but is used to indicate that the method
 // declarations should be generated. This class takes an optional set of methods
 // that should have declarations generated even if the method has a default
 // implementation.
 class DeclareInterfaceMethods<list<string> overridenMethods = []> {
     // This field contains a set of method names that should always have their
     // declarations generated. This allows for generating declarations for
     // methods with default implementations that need to be overridden.
     list<string> alwaysOverriddenMethods = overridenMethods;
 }
 class DeclareAttrInterfaceMethods<AttrInterface interface,
                                   list<string> overridenMethods = []>
       : DeclareInterfaceMethods<overridenMethods>,
-        AttrInterface<interface.cppClassName> {
+        AttrInterface<interface.cppInterfaceName> {
     let description = interface.description;
-    let cppClassName = interface.cppClassName;
+    let cppInterfaceName = interface.cppInterfaceName;
     let cppNamespace = interface.cppNamespace;
     let methods = interface.methods;
 }
 class DeclareOpInterfaceMethods<OpInterface interface,
                                 list<string> overridenMethods = []>
       : DeclareInterfaceMethods<overridenMethods>,
-        OpInterface<interface.cppClassName> {
+        OpInterface<interface.cppInterfaceName> {
     let description = interface.description;
-    let cppClassName = interface.cppClassName;
+    let cppInterfaceName = interface.cppInterfaceName;
     let cppNamespace = interface.cppNamespace;
     let methods = interface.methods;
 }
 class DeclareTypeInterfaceMethods<TypeInterface interface,
                                   list<string> overridenMethods = []>
       : DeclareInterfaceMethods<overridenMethods>,
-        TypeInterface<interface.cppClassName> {
+        TypeInterface<interface.cppInterfaceName> {
     let description = interface.description;
-    let cppClassName = interface.cppClassName;
+    let cppInterfaceName = interface.cppInterfaceName;
     let cppNamespace = interface.cppNamespace;
     let methods = interface.methods;
 }
 
 //===----------------------------------------------------------------------===//
 // Op definitions
 //===----------------------------------------------------------------------===//
 

mlir/lib/TableGen/Interfaces.cpp

Interface::Interface(const llvm::Record *def) : def(def) {
 
   auto *listInit = dyn_cast<llvm::ListInit>(def->getValueInit("methods"));
   for (llvm::Init *init : listInit->getValues())
     methods.emplace_back(cast<llvm::DefInit>(init)->getDef());
 }
 
 // Return the name of this interface.
 StringRef Interface::getName() const {
-  return def->getValueAsString("cppClassName");
+  return def->getValueAsString("cppInterfaceName");
 }
 
 // Return the C++ namespace of this interface.
 StringRef Interface::getCppNamespace() const {
   return def->getValueAsString("cppNamespace");
 }
 
 // Return the methods of this interface.

mlir/lib/Tools/PDLL/Parser/Parser.cpp

for (const tblgen::NamedTypeConstraint &operand : op.getOperands()) {
       odsOp->appendOperand(operand.name, getLengthKind(operand),
                            addTypeConstraint(operand));
     }
     for (const tblgen::NamedTypeConstraint &result : op.getResults()) {
       odsOp->appendResult(result.name, getLengthKind(result),
                           addTypeConstraint(result));
     }
   }
+
+  auto shouldBeSkipped = [this](llvm::Record *def) {
+    return def->isAnonymous() || curDeclScope->lookup(def->getName()) ||
+           def->isSubClassOf("DeclareInterfaceMethods");
+  };
+
   /// Attr constraints.
   for (llvm::Record *def : tdRecords.getAllDerivedDefinitions("Attr")) {
-    if (!def->isAnonymous() && !curDeclScope->lookup(def->getName())) {
-      tblgen::Attribute constraint(def);
-      decls.push_back(
-          createODSNativePDLLConstraintDecl<ast::AttrConstraintDecl>(
-              constraint, convertLocToRange(def->getLoc().front()), attrTy,
-              constraint.getStorageType()));
-    }
-  }
+    if (shouldBeSkipped(def))
+      continue;
+
+    decls.push_back(createODSNativePDLLConstraintDecl<ast::AttrConstraintDecl>(
   /// Type constraints.
   for (llvm::Record *def : tdRecords.getAllDerivedDefinitions("Type")) {
-    if (!def->isAnonymous() && !curDeclScope->lookup(def->getName())) {
-      tblgen::TypeConstraint constraint(def);
-      decls.push_back(
-          createODSNativePDLLConstraintDecl<ast::TypeConstraintDecl>(
-              constraint, convertLocToRange(def->getLoc().front()), typeTy,
-              constraint.getCPPClassName()));
-    }
-  }
-  /// Interfaces.
+    if (shouldBeSkipped(def))
+      continue;
+
+    decls.push_back(createODSNativePDLLConstraintDecl<ast::TypeConstraintDecl>(
+  /// OpInterfaces.
   ast::Type opTy = ast::OperationType::get(ctx);
-  for (llvm::Record *def : tdRecords.getAllDerivedDefinitions("Interface")) {
-    StringRef name = def->getName();
-    if (def->isAnonymous() || curDeclScope->lookup(name) ||
-        def->isSubClassOf("DeclareInterfaceMethods"))
+  for (llvm::Record *def : tdRecords.getAllDerivedDefinitions("OpInterface")) {
+    if (shouldBeSkipped(def))
       continue;
+
     SMRange loc = convertLocToRange(def->getLoc().front());
 
     std::string cppClassName =
         llvm::formatv("{0}::{1}", def->getValueAsString("cppNamespace"),
-                      def->getValueAsString("cppClassName"))
+                      def->getValueAsString("cppInterfaceName"))
             .str();
     std::string codeBlock =
         llvm::formatv("return ::mlir::success(llvm::isa<{0}>(self));",
                       cppClassName)
             .str();
 
     std::string desc =
         processAndFormatDoc(def->getValueAsString("description"));
-    if (def->isSubClassOf("OpInterface")) {
-      decls.push_back(createODSNativePDLLConstraintDecl<ast::OpConstraintDecl>(
-          name, codeBlock, loc, opTy, cppClassName, desc));
-    } else if (def->isSubClassOf("AttrInterface")) {
-      decls.push_back(
-          createODSNativePDLLConstraintDecl<ast::AttrConstraintDecl>(
-              name, codeBlock, loc, attrTy, cppClassName, desc));
-    } else if (def->isSubClassOf("TypeInterface")) {
-      decls.push_back(
-          createODSNativePDLLConstraintDecl<ast::TypeConstraintDecl>(
-              name, codeBlock, loc, typeTy, cppClassName, desc));
-    }
+        def->getName(), codeBlock, loc, opTy, cppClassName, desc));
   }
 }
 
 template <typename ConstraintT>
 ast::Decl *Parser::createODSNativePDLLConstraintDecl(
     StringRef name, StringRef codeBlock, SMRange loc, ast::Type type,
     StringRef nativeType, StringRef docString) {
   // Build the single input parameter.

mlir/test/mlir-pdll/Parser/include_td.pdll

 // CHECK-NEXT:   CppClass: ::mlir::IntegerAttr
 // CHECK-NEXT: }
 
 // CHECK:      TypeConstraint `I64` {
 // CHECK-NEXT:   Summary: 64-bit signless integer
 // CHECK-NEXT:   CppClass: ::mlir::IntegerType
 // CHECK-NEXT: }
 
-// CHECK: UserConstraintDecl {{.*}} Name<TestAttrInterface> ResultType<Tuple<>> Code<return ::mlir::success(llvm::isa<::TestAttrInterface>(self));>
+// CHECK: UserConstraintDecl {{.*}} Name<TestAttrInterface> ResultType<Tuple<>> Code<return ::mlir::success((self.isa<TestAttrInterface>()));>
 // CHECK:  `Inputs`
 // CHECK:    `-VariableDecl {{.*}} Name<self> Type<Attr>
 // CHECK:      `Constraints`
 // CHECK:        `-AttrConstraintDecl
 
+// CHECK: UserConstraintDecl {{.*}} Name<TestTypeInterface> ResultType<Tuple<>> Code<return ::mlir::success((self.isa<TestTypeInterface>()));>
+// CHECK:  `Inputs`
+// CHECK:    `-VariableDecl {{.*}} Name<self> Type<Type>
+// CHECK:      `Constraints`
+// CHECK:        `-TypeConstraintDecl {{.*}}
+
 // CHECK: UserConstraintDecl {{.*}} Name<TestOpInterface> ResultType<Tuple<>> Code<return ::mlir::success(llvm::isa<::TestOpInterface>(self));>
 // CHECK:  `Inputs`
 // CHECK:    `-VariableDecl {{.*}} Name<self> Type<Op>
 // CHECK:      `Constraints`
 // CHECK:        `-OpConstraintDecl
 // CHECK:          `-OpNameDecl
-
-// CHECK: UserConstraintDecl {{.*}} Name<TestTypeInterface> ResultType<Tuple<>> Code<return ::mlir::success(llvm::isa<::TestTypeInterface>(self));>
-// CHECK:  `Inputs`
-// CHECK:    `-VariableDecl {{.*}} Name<self> Type<Type>
-// CHECK:      `Constraints`
-// CHECK:        `-TypeConstraintDecl {{.*}}

mlir/test/mlir-tblgen/interfaces-as-constraints.td

+// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s
+
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test";
+}
+
+def TopLevelTypeInterface : TypeInterface<"TopLevelTypeInterface">;
+
+def TypeInterfaceInNamespace : TypeInterface<"TypeInterfaceInNamespace"> {
+  let cppNamespace = "test";
+}
+
+def TopLevelAttrInterface : AttrInterface<"TopLevelAttrInterface">;
+
+def AttrInterfaceInNamespace : AttrInterface<"AttrInterfaceInNamespace"> {
+  let cppNamespace = "test";
+}
+
+def OpUsingAllOfThose : Op<Test_Dialect, "OpUsingAllOfThose"> {
+  let arguments = (ins TopLevelAttrInterface:$attr1, AttrInterfaceInNamespace:$attr2);
+  let results = (outs TopLevelTypeInterface:$res1, TypeInterfaceInNamespace:$res2);
+}
+
+// CHECK: static ::mlir::LogicalResult {{__mlir_ods_local_type_constraint.*}}(
+// CHECK:   if (!((type.isa<TopLevelTypeInterface>()))) {
+// CHECK-NEXT:    return op->emitOpError(valueKind) << " #" << valueIndex
+// CHECK-NEXT:        << " must be TopLevelTypeInterface instance, but got " << type;
+
+// CHECK: static ::mlir::LogicalResult {{__mlir_ods_local_type_constraint.*}}(
+// CHECK:   if (!((type.isa<test::TypeInterfaceInNamespace>()))) {
+// CHECK-NEXT:    return op->emitOpError(valueKind) << " #" << valueIndex
+// CHECK-NEXT:        << " must be TypeInterfaceInNamespace instance, but got " << type;
+
+// CHECK: static ::mlir::LogicalResult {{__mlir_ods_local_attr_constraint.*}}(
+// CHECK:   if (attr && !((attr.isa<TopLevelAttrInterface>()))) {
+// CHECK-NEXT:    return op->emitOpError("attribute '") << attrName
+// CHECK-NEXT:        << "' failed to satisfy constraint: TopLevelAttrInterface instance";
+
+// CHECK: static ::mlir::LogicalResult {{__mlir_ods_local_attr_constraint.*}}(
+// CHECK:   if (attr && !((attr.isa<test::AttrInterfaceInNamespace>()))) {
+// CHECK-NEXT:    return op->emitOpError("attribute '") << attrName
+// CHECK-NEXT:        << "' failed to satisfy constraint: AttrInterfaceInNamespace instance";
+
+// CHECK: TopLevelAttrInterface OpUsingAllOfThose::attr1()
+// CHECK: test::AttrInterfaceInNamespace OpUsingAllOfThose::attr2()

mlir/tools/mlir-tblgen/OpFormatGen.cpp

if (def.isSubClassOf("AllTypesMatch")) {
       handleSameTypesConstraint(variableTyResolver, /*includeResults=*/true);
     } else if (def.isSubClassOf("TypesMatchWith")) {
       handleTypesMatchConstraint(variableTyResolver, def);
     } else if (!op.allResultTypesKnown()) {
       // This doesn't check the name directly to handle
       //    DeclareOpInterfaceMethods<InferTypeOpInterface>
       // and the like.
       // TODO: Add hasCppInterface check.
-      if (auto name = def.getValueAsOptionalString("cppClassName")) {
+      if (auto name = def.getValueAsOptionalString("cppInterfaceName")) {
         if (*name == "InferTypeOpInterface" &&
             def.getValueAsString("cppNamespace") == "::mlir")
           canInferResultTypes = true;
       }
     }
   }
 
   // Verify the state of the various operation components.