[mlir][TableGen] Emit interface traits after all interfaces #147699

andrey-golubev · 2025-07-09T11:19:17Z

Interface traits may provide default implementation of methods. When this happens, the implementation may rely on another interface that is not yet defined meaning that one gets "incomplete type" error during C++ compilation. In pseudo-code, the problem is the following:

InterfaceA has methodB() { return InterfaceB(); }
InterfaceB defined later

// What's generated is:
class InterfaceA { ... }
class InterfaceATrait {
  // error: InterfaceB is an incomplete type
  InterfaceB methodB() { return InterfaceB(); }
}
class InterfaceB { ... } // defined here

The two more "advanced" cases are:

Cyclic dependency (A requires B and B requires A)
Type-traited usage of an incomplete type (e.g. FailureOr<InterfaceB>)

It seems reasonable to emit interface traits after all of the interfaces have been defined to avoid the problem altogether.

As a drive by, make forward declarations of the interfaces early so that user code does not need to forward declare.

llvmbot · 2025-07-09T11:19:50Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Andrei Golubev (andrey-golubev)

Changes

Interface traits may provide default implementation of methods. When this happens, the implementation may rely on another interface that is not yet defined meaning that one gets "incomplete type" error during C++ compilation. In pseudo-code, the problem is the following:

InterfaceA has methodB() { return InterfaceB(); }
InterfaceB defined later

// What's generated is:
class InterfaceA { ... }
class InterfaceATrait {
  // error: InterfaceB is an incomplete type
  InterfaceB methodB() { return InterfaceB(); }
}
class InterfaceB { ... } // defined here

The two more "advanced" cases are:

Cyclic dependency (A requires B and B requires A)
Type-traited usage of an incomplete type (e.g. FailureOr<InterfaceB>)

It seems reasonable to emit interface traits after all of the interfaces have been defined to avoid the problem altogether.

As a drive by, make forward declarations of the interfaces early so that user code does not need to forward declare.

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

2 Files Affected:

(modified) mlir/test/lib/Dialect/Test/TestInterfaces.td (+28)
(modified) mlir/tools/mlir-tblgen/OpInterfacesGen.cpp (+34-10)

diff --git a/mlir/test/lib/Dialect/Test/TestInterfaces.td b/mlir/test/lib/Dialect/Test/TestInterfaces.td
index dea26b8dda62a..d3d96ea5a65a4 100644
--- a/mlir/test/lib/Dialect/Test/TestInterfaces.td
+++ b/mlir/test/lib/Dialect/Test/TestInterfaces.td
@@ -174,4 +174,32 @@ def TestOptionallyImplementedTypeInterface
   }];
 }
 
+// Dummy type interface "A" that requires type interface "B" to be complete.
+def TestCyclicTypeInterfaceA : TypeInterface<"TestCyclicTypeInterfaceA"> {
+  let cppNamespace = "::mlir";
+  let methods = [
+    InterfaceMethod<"",
+      "::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB>",
+      /*methodName=*/"returnB",
+      (ins),
+      /*methodBody=*/"",
+      /*defaultImpl=*/"return mlir::failure();"
+    >,
+  ];
+}
+
+// Dummy type interface "B" that requires type interface "A" to be complete.
+def TestCyclicTypeInterfaceB : TypeInterface<"TestCyclicTypeInterfaceB"> {
+  let cppNamespace = "::mlir";
+  let methods = [
+    InterfaceMethod<"",
+      "::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA>",
+      /*methodName=*/"returnA",
+      (ins),
+      /*methodBody=*/"",
+      /*defaultImpl=*/"return mlir::failure();"
+    >,
+  ];
+}
+
 #endif // MLIR_TEST_DIALECT_TEST_INTERFACES
diff --git a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
index 4dfa1908b3267..ba1396e7d12be 100644
--- a/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp
@@ -96,9 +96,9 @@ class InterfaceGenerator {
   void emitConceptDecl(const Interface &interface);
   void emitModelDecl(const Interface &interface);
   void emitModelMethodsDef(const Interface &interface);
-  void emitTraitDecl(const Interface &interface, StringRef interfaceName,
-                     StringRef interfaceTraitsName);
+  void forwardDeclareInterface(const Interface &interface);
   void emitInterfaceDecl(const Interface &interface);
+  void emitInterfaceTraitDecl(const Interface &interface);
 
   /// The set of interface records to emit.
   std::vector<const Record *> defs;
@@ -445,9 +445,16 @@ void InterfaceGenerator::emitModelMethodsDef(const Interface &interface) {
     os << "} // namespace " << ns << "\n";
 }
 
-void InterfaceGenerator::emitTraitDecl(const Interface &interface,
-                                       StringRef interfaceName,
-                                       StringRef interfaceTraitsName) {
+void InterfaceGenerator::emitInterfaceTraitDecl(const Interface &interface) {
+  llvm::SmallVector<StringRef, 2> namespaces;
+  llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
+  for (StringRef ns : namespaces)
+    os << "namespace " << ns << " {\n";
+
+  os << "namespace detail {\n";
+
+  StringRef interfaceName = interface.getName();
+  auto interfaceTraitsName = (interfaceName + "InterfaceTraits").str();
   os << llvm::formatv("  template <typename {3}>\n"
                       "  struct {0}Trait : public ::mlir::{2}<{0},"
                       " detail::{1}>::Trait<{3}> {{\n",
@@ -494,6 +501,10 @@ void InterfaceGenerator::emitTraitDecl(const Interface &interface,
     os << tblgen::tgfmt(*extraTraitDecls, &traitMethodFmt) << "\n";
 
   os << "  };\n";
+  os << "}// namespace detail\n";
+
+  for (StringRef ns : llvm::reverse(namespaces))
+    os << "} // namespace " << ns << "\n";
 }
 
 static void emitInterfaceDeclMethods(const Interface &interface,
@@ -517,6 +528,19 @@ static void emitInterfaceDeclMethods(const Interface &interface,
     os << tblgen::tgfmt(extraDecls->rtrim(), &extraDeclsFmt) << "\n";
 }
 
+void InterfaceGenerator::forwardDeclareInterface(const Interface &interface) {
+  llvm::SmallVector<StringRef, 2> namespaces;
+  llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
+  for (StringRef ns : namespaces)
+    os << "namespace " << ns << " {\n";
+
+  StringRef interfaceName = interface.getName();
+  os << "class " << interfaceName << ";\n";
+
+  for (StringRef ns : llvm::reverse(namespaces))
+    os << "} // namespace " << ns << "\n";
+}
+
 void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
   llvm::SmallVector<StringRef, 2> namespaces;
   llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
@@ -533,7 +557,6 @@ void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
   if (!comments.empty()) {
     os << comments << "\n";
   }
-  os << "class " << interfaceName << ";\n";
 
   // Emit the traits struct containing the concept and model declarations.
   os << "namespace detail {\n"
@@ -603,10 +626,6 @@ void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
 
   os << "};\n";
 
-  os << "namespace detail {\n";
-  emitTraitDecl(interface, interfaceName, interfaceTraitsName);
-  os << "}// namespace detail\n";
-
   for (StringRef ns : llvm::reverse(namespaces))
     os << "} // namespace " << ns << "\n";
 }
@@ -619,10 +638,15 @@ bool InterfaceGenerator::emitInterfaceDecls() {
   llvm::sort(sortedDefs, [](const Record *lhs, const Record *rhs) {
     return lhs->getID() < rhs->getID();
   });
+  for (const Record *def : sortedDefs)
+    forwardDeclareInterface(Interface(def));
   for (const Record *def : sortedDefs)
     emitInterfaceDecl(Interface(def));
   for (const Record *def : sortedDefs)
     emitModelMethodsDef(Interface(def));
+  for (const Record *def : sortedDefs)
+    emitInterfaceTraitDecl(Interface(def));
+
   return false;
 }

andrey-golubev · 2025-07-09T11:20:26Z

@ftynse @joker-eph @River707 please take a look! I have stumbled upon this problem when patching one-shot bufferization and figured it makes sense to fix the behaviour instead of working around it.

andrey-golubev · 2025-07-09T11:22:24Z

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp

fyi: Interface ctor can be rather expensive (it has a vector of base interfaces that it populates + vector of methods).
As we re-create the objects 4 times here, does it make sense to refactor? (I failed to do it immediately so it'd probably go into a separate patch).

FWIW tablegen is part of the build system so if it is slow, the whole build is slow.

andrey-golubev · 2025-07-22T09:04:37Z

Gentle ping @ftynse @River707 @joker-eph.

Also, I haven't looked at it (due to the lack of time), but perhaps just moving method definitions later (similarly to how it's done for methods?) would be possible and more preferred?

andrey-golubev · 2025-08-05T10:32:37Z

Gentle ping.

andrey-golubev · 2025-08-14T14:24:19Z

Gentle ping @ftynse @joker-eph.

@matthias-springer do you feel comfortable to review this PR? I infer that Alex / Mehdi are on vacation or busy, so perhaps you could take a look instead. This isn't super controversial afaict.

Maybe you could cast your vote on this as well:

Also, I haven't looked at it (due to the lack of time), but perhaps just moving method definitions later (similarly to how it's done for methods?) would be possible and more preferred?

andrey-golubev · 2025-09-11T12:35:03Z

ping.

matthias-springer · 2025-09-12T09:24:25Z

Can you link the before/after output of mlir/test/lib/Dialect/Test/TestInterfaces.td in a Github Gist etc.?

andrey-golubev · 2025-09-15T09:25:55Z

Can you link the before/after output of mlir/test/lib/Dialect/Test/TestInterfaces.td in a Github Gist etc.?

Sure. Here's the full gist with "before" and "after" - https://gist.github.com/andrey-golubev/a337305e3ef31c595b8c2f4e9267ab78

In case this goes stale, the snippet below shows the issue (note: my added comments start with // ag:):

namespace mlir {
class TestCyclicTypeInterfaceA; // ag: forward declaration - only for "A"!
namespace detail {
struct TestCyclicTypeInterfaceAInterfaceTraits {
  struct Concept {
    /// The methods defined by the interface.
    ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB> (*returnB)(const Concept *impl, ::mlir::Type );
    // ag: here, TestCyclicTypeInterfaceB must be declared
  };
  template<typename ConcreteType>
  class Model : public Concept {
  public:
    using Interface = ::mlir::TestCyclicTypeInterfaceA;
    Model() : Concept{returnB} {}

    static inline ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB> returnB(const Concept *impl, ::mlir::Type tablegen_opaque_val);
  };
  template<typename ConcreteType>
  class FallbackModel : public Concept {
  public:
    using Interface = ::mlir::TestCyclicTypeInterfaceA;
    FallbackModel() : Concept{returnB} {}

    static inline ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB> returnB(const Concept *impl, ::mlir::Type tablegen_opaque_val);
  };
  template<typename ConcreteModel, typename ConcreteType>
  class ExternalModel : public FallbackModel<ConcreteModel> {
  public:
    using ConcreteEntity = ConcreteType;
    ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB> returnB(::mlir::Type tablegen_opaque_val) const;
  };
};
template <typename ConcreteType>
struct TestCyclicTypeInterfaceATrait;

} // namespace detail

// ag: this is a definition for the interface - works fine as there are no method definitions
class TestCyclicTypeInterfaceA : public ::mlir::TypeInterface<TestCyclicTypeInterfaceA, detail::TestCyclicTypeInterfaceAInterfaceTraits> {
public:
  using ::mlir::TypeInterface<TestCyclicTypeInterfaceA, detail::TestCyclicTypeInterfaceAInterfaceTraits>::TypeInterface;
  template <typename ConcreteType>
  struct Trait : public detail::TestCyclicTypeInterfaceATrait<ConcreteType> {};
  ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB> returnB() const;
};
// ag: this is a trait generated for the interface above, it has interface method *definitions* inline (due to default implementation)
namespace detail {
  template <typename ConcreteType>
  struct TestCyclicTypeInterfaceATrait : public ::mlir::TypeInterface<TestCyclicTypeInterfaceA, detail::TestCyclicTypeInterfaceAInterfaceTraits>::Trait<ConcreteType> {
    ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceB> returnB() const {
      // ag: here, TestCyclicTypeInterfaceB must be *complete*
      return mlir::failure();
    }
  };
}// namespace detail
} // namespace mlir
// ag: this is the start of interface "B" definition (but it's too late)
namespace mlir {
class TestCyclicTypeInterfaceB;
namespace detail {
struct TestCyclicTypeInterfaceBInterfaceTraits {
  struct Concept {
    /// The methods defined by the interface.
    ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA> (*returnA)(const Concept *impl, ::mlir::Type );
  };
  template<typename ConcreteType>
  class Model : public Concept {
  public:
    using Interface = ::mlir::TestCyclicTypeInterfaceB;
    Model() : Concept{returnA} {}

    static inline ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA> returnA(const Concept *impl, ::mlir::Type tablegen_opaque_val);
  };
  template<typename ConcreteType>
  class FallbackModel : public Concept {
  public:
    using Interface = ::mlir::TestCyclicTypeInterfaceB;
    FallbackModel() : Concept{returnA} {}

    static inline ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA> returnA(const Concept *impl, ::mlir::Type tablegen_opaque_val);
  };
  template<typename ConcreteModel, typename ConcreteType>
  class ExternalModel : public FallbackModel<ConcreteModel> {
  public:
    using ConcreteEntity = ConcreteType;
    ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA> returnA(::mlir::Type tablegen_opaque_val) const;
  };
};
template <typename ConcreteType>
struct TestCyclicTypeInterfaceBTrait;

} // namespace detail
class TestCyclicTypeInterfaceB : public ::mlir::TypeInterface<TestCyclicTypeInterfaceB, detail::TestCyclicTypeInterfaceBInterfaceTraits> {
public:
  using ::mlir::TypeInterface<TestCyclicTypeInterfaceB, detail::TestCyclicTypeInterfaceBInterfaceTraits>::TypeInterface;
  template <typename ConcreteType>
  struct Trait : public detail::TestCyclicTypeInterfaceBTrait<ConcreteType> {};
  ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA> returnA() const;
};
namespace detail {
  template <typename ConcreteType>
  struct TestCyclicTypeInterfaceBTrait : public ::mlir::TypeInterface<TestCyclicTypeInterfaceB, detail::TestCyclicTypeInterfaceBInterfaceTraits>::Trait<ConcreteType> {
    ::mlir::FailureOr<::mlir::TestCyclicTypeInterfaceA> returnA() const {
      return mlir::failure();
    }
  };
}// namespace detail
} // namespace mlir

what the PR does is:

moves method definitions (actually, full trait definitions right now) below, when interfaces, etc. are defined (i.e. complete types)
adds forward declaration section to the very top to avoid needing to forward declare every time in user code (where #include "TestTypeInterfaces.h.inc" happens) - this is mostly for convenience

Interface traits may provide default implementation of methods. When this happens, the implementation may rely on another interface that is not yet defined meaning that one gets "incomplete type" error during C++ compilation. In pseudo-code, the problem is the following: ``` InterfaceA has methodB() { return InterfaceB(); } InterfaceB defined later // What's generated is: class InterfaceA { ... } class InterfaceATrait { // error: InterfaceB is an incomplete type InterfaceB methodB() { return InterfaceB(); } } class InterfaceB { ... } // defined here ``` The two more "advanced" cases are: * Cyclic dependency (A requires B and B requires A) * Type-traited usage of an incomplete type (e.g. `FailureOr<InterfaceB>`) It seems reasonable to emit interface traits *after* all of the interfaces have been defined to avoid the problem altogether. As a drive by, make forward declarations of the interfaces early so that user code does not need to forward declare.

andrey-golubev · 2025-09-15T14:58:57Z

@matthias-springer do you know anything about:
Github Actions CodeQL / Github Actions CodeQL (pull_request)Cancelled after 36s

? i'm not sure if the fact that it's cancelled is OK to merge this one.

matthias-springer · 2025-09-15T15:11:06Z

I think it's fine to merge given that the Linux + Windows CIs passed.

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Jul 9, 2025

andrey-golubev commented Jul 9, 2025

View reviewed changes

andrey-golubev changed the title ~~[mlir][TableGen][NFC] Emit interface traits after all interfaces~~ [mlir][TableGen] Emit interface traits after all interfaces Jul 9, 2025

andrey-golubev requested review from ftynse and River707 July 16, 2025 07:14

andrey-golubev requested a review from matthias-springer September 12, 2025 07:50

andrey-golubev force-pushed the tablegen_interface_decl branch from db672ed to b7ee12f Compare September 15, 2025 08:53

matthias-springer approved these changes Sep 15, 2025

View reviewed changes

andrey-golubev added 2 commits September 15, 2025 13:10

Adapt tests to new changes

25b4a76

andrey-golubev force-pushed the tablegen_interface_decl branch from b7ee12f to 25b4a76 Compare September 15, 2025 13:50

andrey-golubev merged commit f5d3cf4 into llvm:main Sep 15, 2025
9 of 10 checks passed

andrey-golubev deleted the tablegen_interface_decl branch September 15, 2025 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][TableGen] Emit interface traits after all interfaces #147699

[mlir][TableGen] Emit interface traits after all interfaces #147699

Uh oh!

andrey-golubev commented Jul 9, 2025

Uh oh!

llvmbot commented Jul 9, 2025 •

edited

Loading

Uh oh!

andrey-golubev commented Jul 9, 2025

Uh oh!

andrey-golubev Jul 9, 2025

Uh oh!

andrey-golubev commented Jul 22, 2025

Uh oh!

andrey-golubev commented Aug 5, 2025

Uh oh!

andrey-golubev commented Aug 14, 2025

Uh oh!

andrey-golubev commented Sep 11, 2025

Uh oh!

matthias-springer commented Sep 12, 2025

Uh oh!

andrey-golubev commented Sep 15, 2025 •

edited

Loading

Uh oh!

andrey-golubev commented Sep 15, 2025

Uh oh!

matthias-springer commented Sep 15, 2025

Uh oh!

Uh oh!

Uh oh!

[mlir][TableGen] Emit interface traits after all interfaces #147699

[mlir][TableGen] Emit interface traits after all interfaces #147699

Uh oh!

Conversation

andrey-golubev commented Jul 9, 2025

Uh oh!

llvmbot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrey-golubev commented Jul 9, 2025

Uh oh!

andrey-golubev Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

andrey-golubev commented Jul 22, 2025

Uh oh!

andrey-golubev commented Aug 5, 2025

Uh oh!

andrey-golubev commented Aug 14, 2025

Uh oh!

andrey-golubev commented Sep 11, 2025

Uh oh!

matthias-springer commented Sep 12, 2025

Uh oh!

andrey-golubev commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrey-golubev commented Sep 15, 2025

Uh oh!

matthias-springer commented Sep 15, 2025

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 9, 2025 •

edited

Loading

andrey-golubev commented Sep 15, 2025 •

edited

Loading