[MLIR][ODS] Allow operations to specify interfaces using the HasParent trait constraint

[mortbopet]

This requires interfaces to expose a getOperationName(). The name is only emitted for the parent interface (i.e. base interfaces are not considered).

This change is needed by
https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/OpDefinition.h#L1325C41-L1325C41
wherein some name needs to be reported to the user.

should interfaces be identified by getOperationName and not i.e. getInterfaceName? it's a possibility, but would obviously complicate code which wants to treat operation types and interface types as equals.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Changes

... by emitting an operation name for interfaces. The name is only emitted for the parent interface (i.e. base interfaces are not considered).

This change is needed by https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/OpDefinition.h#L1324C38-L1324C38 wherein some name needs to be reported to the user.

... should interfaces be identified by getOperationName and not i.e. getInterfaceName? it's a possibility, but would obviously complicate code which wants to treat operation types and interface types as equals.

Full diff: https://github.com/llvm/llvm-project/pull/66196.diff

2 Files Affected:

• (modified) mlir/test/mlir-tblgen/op-interface.td (+4)
• (modified) mlir/tools/mlir-tblgen/OpInterfacesGen.cpp (+15-1)

diff --git a/mlir/test/mlir-tblgen/op-interface.td b/mlir/test/mlir-tblgen/op-interface.td
index 17bd631fe250d16..80878c9b3205176 100644
--- a/mlir/test/mlir-tblgen/op-interface.td
+++ b/mlir/test/mlir-tblgen/op-interface.td
@@ -167,6 +167,10 @@ def DeclareMethodsOp : Op<TestDialect, "declare_methods_op",
 def DeclareMethodsWithDefaultOp : Op<TestDialect, "declare_methods_op",
       [DeclareOpInterfaceMethods<TestOpInterface, ["default_foo"]>]>;
 
+
+// DECL: /// Returns the name of this interface.
+// DECL: static ::llvm::StringLiteral getOperationName() { return ::llvm::StringLiteral( "TestOpInterface"); }
+
 // DECL-LABEL: TestOpInterfaceInterfaceTraits
 // DECL: class TestOpInterface : public ::mlir::OpInterface<TestOpInterface, detail::TestOpInterfaceInterfaceTraits>
 
diff --git a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
index 9672a02cc08f68c..153543ab083525e 100644
--- a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
@@ -494,6 +494,16 @@ void InterfaceGenerator::emitTraitDecl(const Interface &interface,
   os << "  };\n";
 }
 
+static void emitInterfaceNameGetter(const Interface &interface,
+                                    raw_ostream &os) {
+  if (!isa<OpInterface>(interface))
+    return;
+  os << "  /// Returns the name of this interface.\n"
+     << "  static ::llvm::StringLiteral getOperationName() { return "
+        "::llvm::StringLiteral( \""
+     << interface.getName() << "\"); }\n";
+}
+
 static void emitInterfaceDeclMethods(const Interface &interface,
                                      raw_ostream &os, StringRef valueType,
                                      bool isOpInterface,
@@ -553,6 +563,9 @@ void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
                       "  struct Trait : public detail::{0}Trait<{1}> {{};\n",
                       interfaceName, valueTemplate);
 
+  // Emit the name of the interface.
+  emitInterfaceNameGetter(interface, os);
+
   // Insert the method declarations.
   bool isOpInterface = isa<OpInterface>(interface);
   emitInterfaceDeclMethods(interface, os, valueType, isOpInterface,
@@ -588,7 +601,8 @@ void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
        << "    auto* interface = getInterfaceFor(base);\n"
        << "    if (!interface)\n"
           "      return false;\n"
-          "    " << interfaceName << " odsInterfaceInstance(base, interface);\n"
+          "    "
+       << interfaceName << " odsInterfaceInstance(base, interface);\n"
        << "    " << tblgen::tgfmt(extraClassOf->trim(), &extraClassOfFmt)
        << "\n  }\n";
   }

[joker-eph]

We should add an operation example in the TestDialect to illustrate this (and test the "integration", beyond the ODS unit-test).

I'm not convinced by the getOperationName() on the interface right now, @Mogball @jpienaar ?

[Mogball]

Yeah I'm not sold on the interface name. Why does it do a check using a string name anyways. Shouldn't it use dyn_cast/isa? ODS can generate an error message directly

[mortbopet]

Just updated the PR with a reference to the location that relies on getOperationName (https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/OpDefinition.h#L1325C41-L1325C41).
I'm not tied to adding getOperationName - this was just the smallest change required to get things to seemingly work without issue.

[joker-eph]

The URL is not immutable (the code can move or change), hence I had used a description that made the message more standalone.

[mortbopet]

The URL is not immutable (the code can move or change), hence I had used a description that made the message more standalone.

Makes sense - my apologies, did not see that you modified the PR message 🤦.

[joker-eph]

No worries: I was just providing context in case you were wondering!

[mortbopet]

So how do we want to solve this? change getOperationName to getInterfaceName and have a separate trait i.e. HasParentInterface which reports the required interface name to the user via. getInterfaceName?

[Mogball]

Why do you need a name comparison? I would just do an isa<T> check for both the operation and interface case. This is already supported.

[joker-eph]

@Mogball : it's not for a name comparison, it's for diagnostics. The verifier code in the traits looks like:

    static LogicalResult verifyTrait(Operation *op) {
      if (llvm::isa_and_nonnull<ParentOpTypes...>(op->getParentOp()))
        return success();

      return op->emitOpError()
             << "expects parent op "
             << (sizeof...(ParentOpTypes) != 1 ? "to be one of '" : "'")
             << llvm::ArrayRef({ParentOpTypes::getOperationName()...}) << "'";
    }

[Mogball]

Ah I see. My mistake. But still, having getOperationName on an interface seems a little off. WDYT about adding a new trait called hasInterfaceParent?

[mortbopet]

WDYT about adding a new trait called hasInterfaceParent?

Sure, whatever can accomplish this pattern. The original idea of folding this into the HasParent trait was to allow for situations where an operation requires either a specific operation or a specific interface as a parent. But in reality, i don't think this is a realistic situation - if an op needs an interface, then that seems like a pretty explicit requirement that isn't to be mixed with other ops.
Adding an interface name, i.e. getInterfaceName still seems reasonable to me, for diagnostics reasons.