[mlir] ODS: emit interface model method at the end of the header

[ftynse]

Summary

Previously, ODS interface generator was placing implementations of the
interface's internal "Model" class template immediately after the class
definitions in the header. This doesn't allow this implementation, and
consequently the interface itself, to return an instance of another
interface if its class definition is emitted below. This creates
undesired ordering effects and makes it impossible for two or more
interfaces to return instances of each other. Change the interface
generator to place the implementations of these methods after all
interface classes.

[jpienaar]

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp

293

We no longer need to split post C++ version bump

[mehdi_amini]

Seems like a bit of an ad-hoc solution for multiple interfaces defined in the same file?

[dcaballe]

Thanks, Alex!

[dcaballe]

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp

459

Should we print all the fw-decls at the beginning of the file before any interface declaration so that an interface declaration can refer to another interface?

[dcaballe]

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp

519

I've been looking at the implementation of this file to try to make it work for my use case but I get lost into the details of the autogen engine.
Assuming multiple interfaces per .td file, with this change we are still generating the following for the .h.inc file:

InterfaceA Fw-Decls
InterfaceA Decls (using InterfaceB, InterfaceB fw-decls are needed)
InterfaceA Defs (using InterfaceB, InterfaceB decls are needed)

InterfaceB Fw-Decls
InterfaceB Decls (using InterfaceA, InterfaceA fw-decls are needed)
InterfaceB Defs (using InterfaceA, InterfaceA decls are needed)

The main issue here is not the fw-decl dependency (that has an easy fix by adding the fw-decls by hand to the interface's .h file) but the decl dependency. InterfaceA needs a full declaration of InterfaceB for its definitions and vice versa. That means we would need to generate something like:

InterfaceA Fw-Decls
InterfaceB Fw-Decls

InterfaceA Decls (using InterfaceB, InterfaceB fw-decls are needed)
InterfaceB Decls (using InterfaceA, InterfaceA fw-decls are needed)

InterfaceA Defs (using InterfaceB, InterfaceB decls are needed)
InterfaceB Defs (using InterfaceA, InterfaceA decls are needed)

I don't see a simple way to generate the code in this order as it looks like each interface is processed independently? I'm missing context here but perhaps there is a way to process/register all the interfaces and only once they are all processed then generate all the code?

As a workaround I guess I can copy paste the generated code to the interface .h file and reorder it by hand. Hopefully we find a way to address this limitation.
Thanks!

[dcaballe]

Sorry, I accidentally committed this diff! I had it in my local branch. Should I revert it? I think it's a step in the right direction and I was able to workaround the cyclic dependency that had so this should be good enough for now if we don't want to address the more complex cases right now.

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

def DeclareMethodsWithDefaultOp : Op<TestDialect, "declare_methods_op",
       [DeclareOpInterfaceMethods<TestOpInterface, ["default_foo"]>]>;
 
 // DECL-LABEL: TestOpInterfaceInterfaceTraits
 // DECL: class TestOpInterface : public ::mlir::OpInterface<TestOpInterface, detail::TestOpInterfaceInterfaceTraits>
 
 // DECL: /// some function comment
 // DECL: int foo(int input);
 
-// DECL: template<typename ConcreteOp>
-// DECL: int detail::TestOpInterfaceInterfaceTraits::Model<ConcreteOp>::foo
-
 // DECL-LABEL: struct TestOpInterfaceVerifyTrait
 // DECL: verifyTrait
 
 // DECL-LABEL: struct TestOpInterfaceVerifyRegionTrait
 // DECL: verifyRegionTrait
 
+// Method implementations come last, after all class definitions.
+// DECL: template<typename ConcreteOp>
+// DECL: int detail::TestOpInterfaceInterfaceTraits::Model<ConcreteOp>::foo
+
 // OP_DECL-LABEL: class DeclareMethodsOp : public
 // OP_DECL: int foo(int input);
 // OP_DECL-NOT: int default_foo(int input);
 
 // OP_DECL-LABEL: class DeclareMethodsWithDefaultOp : public
 // OP_DECL: int foo(int input);
 // OP_DECL: int default_foo(int input);

mlir/tools/mlir-tblgen/OpInterfacesGen.cpp

for (auto &method : interface.getMethods()) {
     if (!method.isStatic())
       os << " const";
     os << ";\n";
   }
   os << "  };\n";
 }
 
 void InterfaceGenerator::emitModelMethodsDef(const Interface &interface) {
+  llvm::SmallVector<StringRef, 2> namespaces;
+  llvm::SplitString(interface.getCppNamespace(), namespaces, "::");
+  for (StringRef ns : namespaces)
+    os << "namespace " << ns << " {\n";
+
   for (auto &method : interface.getMethods()) {
     os << "template<typename " << valueTemplate << ">\n";
     emitCPPType(method.getReturnType(), os);
     os << "detail::" << interface.getName() << "InterfaceTraits::Model<"
        << valueTemplate << ">::";
     emitMethodNameAndArgs(method, os, valueType,
                           /*addThisArg=*/!method.isStatic(),
                           /*addConst=*/false);
for (auto &method : interface.getMethods()) {
     os << " {\n";
 
     // Use the empty context for static methods.
     tblgen::FmtContext ctx;
     os << tblgen::tgfmt(method.getDefaultImplementation()->trim(),
                         method.isStatic() ? &ctx : &nonStaticMethodFmt);
     os << "\n}\n";
   }
+
+  for (StringRef ns : llvm::reverse(namespaces))
+    os << "} // namespace " << ns << "\n";
 }
 
 void InterfaceGenerator::emitTraitDecl(const Interface &interface,
                                        StringRef interfaceName,
                                        StringRef interfaceTraitsName) {
   os << llvm::formatv("  template <typename {3}>\n"
                       "  struct {0}Trait : public ::mlir::{2}<{0},"
                       " detail::{1}>::Trait<{3}> {{\n",
void InterfaceGenerator::emitInterfaceDecl(const Interface &interface) {
   for (StringRef ns : namespaces)
     os << "namespace " << ns << " {\n";
 
   StringRef interfaceName = interface.getName();
   auto interfaceTraitsName = (interfaceName + "InterfaceTraits").str();
 
   // Emit a forward declaration of the interface class so that it becomes usable
   // in the signature of its methods.
   os << "class " << interfaceName << ";\n";
 
   // Emit the traits struct containing the concept and model declarations.
   os << "namespace detail {\n"
      << "struct " << interfaceTraitsName << " {\n";
   emitConceptDecl(interface);
   emitModelDecl(interface);
   os << "};";
 
if (Optional<StringRef> extraDecls =
     os << tblgen::tgfmt(*extraDecls, &extraDeclsFmt);
 
   os << "};\n";
 
   os << "namespace detail {\n";
   emitTraitDecl(interface, interfaceName, interfaceTraitsName);
   os << "}// namespace detail\n";
 
-  emitModelMethodsDef(interface);
-
   for (StringRef ns : llvm::reverse(namespaces))
     os << "} // namespace " << ns << "\n";
 }
 
 bool InterfaceGenerator::emitInterfaceDecls() {
   llvm::emitSourceFileHeader("Interface Declarations", os);
 
-  for (const auto *def : defs)
+  for (const llvm::Record *def : defs)
     emitInterfaceDecl(Interface(def));
+  for (const llvm::Record *def : defs)
+    emitModelMethodsDef(Interface(def));
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // GEN: Interface documentation
 //===----------------------------------------------------------------------===//
 
 static void emitInterfaceDoc(const llvm::Record &interfaceDef,