[MLIR] Fix autogenerated pass constructors linkage

[mscuttari]

Summary

The patch addresses the linkage of the new autogenerated pass constructors, which, being declared as friend functions, resulted in having an inline nature and thus their implementations not being exported.

[mscuttari]

Compilation log (a bit trimmed, for clearness):

====================[ Build | check-mlir | Debug ]==============================
/Applications/CLion.app/Contents/bin/cmake/mac/bin/cmake --build /Users/mscuttari/Desktop/llvm-project/llvm/cmake-build-debug --target check-mlir -j 8
[127/139] Linking CXX executable tools/mlir/unittests/ExecutionEngine/MLIRExecutionEngineTests

Undefined symbols for architecture arm64:
  "mlir::createConvertArithmeticToLLVMPass()", referenced from:
      lowerToLLVMDialect(mlir::ModuleOp) in Invoke.cpp.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[130/139] Linking CXX executable bin/mlir-opt

Undefined symbols for architecture arm64:
  "mlir::createConvertArithmeticToLLVMPass()", referenced from:
      mlir::registerConvertArithmeticToLLVMPass()::'lambda'()::operator()() const in mlir-opt.cpp.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[131/139] Linking CXX executable bin/mlir-capi-ir-test

Undefined symbols for architecture arm64:
  "mlir::createConvertArithmeticToLLVMPass()", referenced from:
      mlir::registerConvertArithmeticToLLVMPass()::'lambda'()::operator()() const in libMLIRCAPIRegisterEverything.a(RegisterEverything.cpp.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[132/139] Linking CXX executable bin/mlir-reduce

Undefined symbols for architecture arm64:
  "mlir::createConvertArithmeticToLLVMPass()", referenced from:
      mlir::registerConvertArithmeticToLLVMPass()::'lambda'()::operator()() const in mlir-reduce.cpp.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[133/139] Linking CXX executable bin/mlir-capi-execution-engine-test

Undefined symbols for architecture arm64:
  "mlir::createConvertArithmeticToLLVMPass()", referenced from:
      _mlirCreateConversionConvertArithmeticToLLVMPass in libMLIRCAPIConversion.a(Passes.cpp.o)
      mlir::registerConvertArithmeticToLLVMPass()::'lambda'()::operator()() const in libMLIRCAPIConversion.a(Passes.cpp.o)
      mlir::registerConvertArithmeticToLLVMPass()::'lambda'()::operator()() const in libMLIRCAPIRegisterEverything.a(RegisterEverything.cpp.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[134/139] Linking CXX executable bin/mlir-cpu-runner
ninja: build stopped: subcommand failed.

[rriddle]

mlir/lib/Conversion/ArithmeticToLLVM/ArithmeticToLLVM.cpp

21 ↗

(On Diff #455260)

Is the base pass class being generated in the mlir namespace? It needs to be (the same as the namespace the create method is in).

[mscuttari]

mlir/lib/Conversion/ArithmeticToLLVM/ArithmeticToLLVM.cpp

21 ↗

(On Diff #455260)

Already tried (but uploaded the version without, as per original design intentions), didn't make any difference.

[mscuttari]

Wrapping the base class in mlir namespace. It is indeed the correct way to go, but still the
problem persists.

[mehdi_amini]

The issue is that the friend function in the class gets an inline linkage: it only gets emitted if it used in the translation unit.
I'm not sure if it can be made "not inline", usually you have to define it outside of the class but I don't know what the syntax would be here...

[mscuttari]

In D132572#3747742, @mehdi_amini wrote:

The issue is that the friend function in the class gets an inline linkage: it only gets emitted if it used in the translation unit.
I'm not sure if it can be made "not inline", usually you have to define it outside of the class but I don't know what the syntax would be here...

I've made a few tries but none of them solved the problem. What if we just go for the splitting of the macro? It seems to be we are trying to push the standard to its limits, with all the consequent risks.

[rriddle]

In D132572#3748287, @mscuttari wrote:

In D132572#3747742, @mehdi_amini wrote:

The issue is that the friend function in the class gets an inline linkage: it only gets emitted if it used in the translation unit.
I'm not sure if it can be made "not inline", usually you have to define it outside of the class but I don't know what the syntax would be here...

I've made a few tries but none of them solved the problem. What if we just go for the splitting of the macro? It seems to be we are trying to push the standard to its limits, with all the consequent risks.

The inline linkage is specified in the standard it seems (I think): https://en.cppreference.com/w/cpp/language/friend

Defines a non-member function, and makes it a friend of this class at the same time. Such non-member function is always inline

If that's the case the easy fix would be to use an "Impl" function for the friend that is local, and just dispatch to that in the main create method:

std::unique_ptr<Pass> createFooImpl();

template <typename T>
class FooBase ... {
  friend std::unique_ptr<Pass> createFooImpl(...) {
    return std::make_unique<T>(...);
  }
};

std::unique_ptr<Pass> createFoo() {
  return createFooImpl();
}

That worked when I did it locally. I think if we really need to we should switch to 3 macros, but I think we should try as much as possible to limit the complexity necessary to define a single pass.

[mscuttari]

Add createPassNameImpl methods

[mscuttari]

@rriddle thanks for the suggestion, working flawlessly. I have uploaded your fix and removed the changes I was doing to the passes, which I will instead upload in a separate patch in order to avoid a single commit mixing different stuff.

[rriddle]

Nice! Do you think we should add a namespace field to the tablegen pass def now? Seems like that would be consistent with everything else (and would remove potential user foot guns as well).

[mscuttari]

In D132572#3751139, @rriddle wrote:

Nice! Do you think we should add a namespace field to the tablegen pass def now? Seems like that would be consistent with everything else (and would remove potential user foot guns as well).

Sounds reasonable. Maybe a implnamespace for both the base class and the createPassNameImplmethods? At that point we can even rename createPassNameImplto just createPassName, as it would live in the implnamespace.

[mscuttari]

impl namespace for pass base class and friend create constructors

[mscuttari]

@rriddle I'm sorry for the early ping, but I'm fixing the big patch regarding all the passes and I would like to know if the current namespace separation looks good. I'd like to avoid reiterating over all the passes just for this :D

[rriddle]

The impl namespace seems fine to me, though we would need to update the docs to describe this. The docs should describe the ideal way to do things, and we can update the old uses in the codebase in the meantime.

[mscuttari]

Update docs

[mscuttari]

I've updated the docs. Feel free to comment on them if you feel them not to be clear enough.
In the meanwhile, I've uploaded a first draft regarding the patches to the pass declarations, so we can start discussing about them: D132838

mlir/docs/PassManagement.md

 def MyPass : Pass<"my-pass", "ModuleOp"> {
   let summary = "My Pass Summary";
   let description = [{
     Here we can now give a much larger description of `MyPass`, including all of
     its various constraints and behavior.
   }];
 
   // A constructor must be provided to specify how to create a default instance
-  // of MyPass.
+  // of MyPass. It can be skipped for this specific example, because both the
+  // constructor and the registration methods live in the same namespace.
   let constructor = "foo::createMyPass()";
 
   // Specify any options.
   let options = [
     Option<"option", "example-option", "bool", /*default=*/"true",
            "An example option">,
     ListOption<"listOption", "example-list", "int64_t",
                "An example list option">

 
 The first is to register the declared passes with the global registry. For
 each pass, the generator produces a `registerPassName` where
 `PassName` is the name of the definition specified in tablegen. It also
 generates a `registerGroupPasses`, where `Group` is the tag provided via the
 `-name` input parameter, that registers all of the passes present.
 
 ```c++
-// gen-pass-decls -name="Example"
+// Tablegen options: -gen-pass-decls -name="Example"
 
+// Passes.h
+
+namespace foo {
 #define GEN_PASS_REGISTRATION
 #include "Passes.h.inc"
+} // namespace foo
 
 void registerMyPasses() {
   // Register all of the passes.
-  registerExamplePasses();
+  foo::registerExamplePasses();
+  
+  // Or
 
   // Register `MyPass` specifically.
-  registerMyPass();
+  foo::registerMyPass();
 }
 ```
 
 The second is to provide a way to configure the pass options. These classes are
 named in the form of `MyPassOptions`, where `MyPass` is the name of the pass
 definition in tablegen. The configurable parameters reflect the options
 declared in the tablegen file. Differently from the registration hooks, these
 classes can be enabled on a per-pass basis by defining the

 #endif // GEN_PASS_DECL_MYPASS
 ```
 
 The `gen-pass-defs` generator produces the definitions to be used for the pass
 implementation.
 
 It generates a base class for each of the passes, containing most of the boiler
 plate related to pass definitions. These classes are named in the form of
-`MyPassBase`, where `MyPass` is the name of the pass definition in tablegen. We
-can update the original C++ pass definition as so:
+`MyPassBase` and are declared inside the `impl` namespace, where `MyPass` is
+the name of the pass definition in tablegen. We can update the original C++
+pass definition as so:
 
 ```c++
+// MyPass.cpp
+
 /// Include the generated base pass class definitions.
+namespace foo {
 #define GEN_PASS_DEF_MYPASS
 #include "Passes.cpp.inc"
+}
 
 /// Define the main class as deriving from the generated base class.
-struct MyPass : MyPassBase<MyPass> {
-  /// The explicit constructor is no longer explicitly necessary when defining
-  /// pass options and statistics, the base class takes care of that
-  /// automatically.
-  ...
+struct MyPass : foo::impl::MyPassBase<MyPass> {
+  using MyPassBase::MyPassBase;
 
   /// The definitions of the options and statistics are now generated within
   /// the base class, but are accessible in the same way.
 };
 ```
 
 Similarly to the previous generator, the definitions can be enabled on a
 per-pass basis by defining the appropriate preprocessor `GEN_PASS_DEF_PASSNAME`

mlir/tools/mlir-tblgen/PassGen.cpp

 //===----------------------------------------------------------------------===//
 
 /// The code snippet used to generate the start of a pass base class.
 ///
 /// {0}: The def name of the pass record.
 /// {1}: The base class for the pass.
 /// {2): The command line argument for the pass.
 /// {3}: The dependent dialects registration.
-const char *const passDeclBegin = R"(
+const char *const baseClassBegin = R"(
 template <typename DerivedT>
 class {0}Base : public {1} {
 public:
   using Base = {0}Base;
 
   {0}Base() : {1}(::mlir::TypeID::get<DerivedT>()) {{}
   {0}Base(const {0}Base &other) : {1}(other) {{}
 

 )";
 
 /// Registration for a single dependent dialect, to be inserted for each
 /// dependent dialect in the `getDependentDialects` above.
 const char *const dialectRegistrationTemplate = R"(
   registry.insert<{0}>();
 )";
 
-const char *const friendDefaultConstructorTemplate = R"(
+const char *const friendDefaultConstructorDeclTemplate = R"(
+namespace impl {{
+  std::unique_ptr<::mlir::Pass> create{0}();
+} // namespace impl
+)";
+
+const char *const friendDefaultConstructorWithOptionsDeclTemplate = R"(
+namespace impl {{
+  std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options);
+} // namespace impl
+)";
+
+const char *const friendDefaultConstructorDefTemplate = R"(
   friend std::unique_ptr<::mlir::Pass> create{0}() {{
     return std::make_unique<DerivedT>();
   }
 )";
 
-const char *const friendDefaultConstructorWithOptionsTemplate = R"(
+const char *const friendDefaultConstructorWithOptionsDefTemplate = R"(
   friend std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
     return std::make_unique<DerivedT>(options);
   }
 )";
 
+const char *const defaultConstructorDefTemplate = R"(
+std::unique_ptr<::mlir::Pass> create{0}() {{
+  return impl::create{0}();
+}
+)";
+
+const char *const defaultConstructorWithOptionsDefTemplate = R"(
+std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
+  return impl::create{0}(options);
+}
+)";
+
 /// Emit the declarations for each of the pass options.
 static void emitPassOptionDecls(const Pass &pass, raw_ostream &os) {
   for (const PassOption &opt : pass.getOptions()) {
     os.indent(2) << "::mlir::Pass::"
                  << (opt.isListOption() ? "ListOption" : "Option");
 
     os << llvm::formatv(R"(<{0}> {1}{{*this, "{2}", ::llvm::cl::desc("{3}"))",
                         opt.getType(), opt.getCppVariableName(),
os << llvm::formatv(
         stat.getCppVariableName(), stat.getName(), stat.getDescription());
   }
 }
 
 /// Emit the code to be used in the implementation of the pass.
 static void emitPassDefs(const Pass &pass, raw_ostream &os) {
   StringRef passName = pass.getDef()->getName();
   std::string enableVarName = "GEN_PASS_DEF_" + passName.upper();
+  bool emitDefaultConstructors = pass.getConstructor().empty();
+  bool emitDefaultConstructorWithOptions = !pass.getOptions().empty();
 
   os << "#ifdef " << enableVarName << "\n";
   os << llvm::formatv(passHeader, passName);
 
+  if (emitDefaultConstructors) {
+    os << llvm::formatv(friendDefaultConstructorDeclTemplate, passName);
+
+    if (emitDefaultConstructorWithOptions)
+      os << llvm::formatv(friendDefaultConstructorWithOptionsDeclTemplate,
+                          passName);
+  }
+
   std::string dependentDialectRegistrations;
   {
     llvm::raw_string_ostream dialectsOs(dependentDialectRegistrations);
     for (StringRef dependentDialect : pass.getDependentDialects())
       dialectsOs << llvm::formatv(dialectRegistrationTemplate,
                                   dependentDialect);
   }
 
-  os << llvm::formatv(passDeclBegin, passName, pass.getBaseClass(),
+  os << "namespace impl {\n";
+  os << llvm::formatv(baseClassBegin, passName, pass.getBaseClass(),
                       pass.getArgument(), pass.getSummary(),
                       dependentDialectRegistrations);
 
   if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty()) {
     os.indent(2) << llvm::formatv(
         "{0}Base(const {0}Options &options) : {0}Base() {{\n", passName);
 
     for (const PassOption &opt : pass.getOptions())
       os.indent(4) << llvm::formatv("{0} = options.{0};\n",
                                     opt.getCppVariableName());
 
     os.indent(2) << "}\n";
   }
 
   // Protected content
   os << "protected:\n";
   emitPassOptionDecls(pass, os);
   emitPassStatisticDecls(pass, os);
 
   // Private content
   os << "private:\n";
 
-  if (pass.getConstructor().empty()) {
-    os << llvm::formatv(friendDefaultConstructorTemplate, passName);
+  if (emitDefaultConstructors) {
+    os << llvm::formatv(friendDefaultConstructorDefTemplate, passName);
 
     if (!pass.getOptions().empty())
-      os << llvm::formatv(friendDefaultConstructorWithOptionsTemplate,
+      os << llvm::formatv(friendDefaultConstructorWithOptionsDefTemplate,
                           passName);
   }
 
   os << "};\n";
+  os << "} // namespace impl\n";
+
+  if (emitDefaultConstructors) {
+    os << llvm::formatv(defaultConstructorDefTemplate, passName);
+
+    if (emitDefaultConstructorWithOptions)
+      os << llvm::formatv(defaultConstructorWithOptionsDefTemplate, passName);
+  }
 
   os << "#undef " << enableVarName << "\n";
   os << "#endif // " << enableVarName << "\n";
 }
 
 static void emitDefs(const llvm::RecordKeeper &recordKeeper, raw_ostream &os) {
   std::vector<Pass> passes = getPasses(recordKeeper);
   os << "/* Autogenerated by mlir-tblgen; don't manually edit */\n";

mlir/unittests/TableGen/PassGenTest.cpp

 #define GEN_PASS_REGISTRATION
 #include "PassGenTest.h.inc"
 
 #define GEN_PASS_DEF_TESTPASS
 #define GEN_PASS_DEF_TESTPASSWITHOPTIONS
 #define GEN_PASS_DEF_TESTPASSWITHCUSTOMCONSTRUCTOR
 #include "PassGenTest.cpp.inc"
 
-struct TestPass : public TestPassBase<TestPass> {
+struct TestPass : public impl::TestPassBase<TestPass> {
   using TestPassBase::TestPassBase;
 
   void runOnOperation() override {}
 
   std::unique_ptr<mlir::Pass> clone() const {
     return TestPassBase<TestPass>::clone();
   }
 };
TEST(PassGenTest, PassClone) {
   const auto origPass = createTestPass();
   const auto clonePass = unwrap(origPass)->clone();
 
   EXPECT_TRUE(clonePass.get() != nullptr);
   EXPECT_TRUE(origPass.get() != clonePass.get());
 }
 
 struct TestPassWithOptions
-    : public TestPassWithOptionsBase<TestPassWithOptions> {
+    : public impl::TestPassWithOptionsBase<TestPassWithOptions> {
   using TestPassWithOptionsBase::TestPassWithOptionsBase;
 
   void runOnOperation() override {}
 
   std::unique_ptr<mlir::Pass> clone() const {
     return TestPassWithOptionsBase<TestPassWithOptions>::clone();
   }
 
TEST(PassGenTest, PassOptions) {
   const auto pass = createTestPassWithOptions(options);
 
   EXPECT_EQ(unwrap(pass)->getTestOption(), 57);
   EXPECT_EQ(unwrap(pass)->getTestListOption()[0], 1);
   EXPECT_EQ(unwrap(pass)->getTestListOption()[1], 2);
 }
 
 struct TestPassWithCustomConstructor
-    : public TestPassWithCustomConstructorBase<TestPassWithCustomConstructor> {
+    : public impl::TestPassWithCustomConstructorBase<
+          TestPassWithCustomConstructor> {
   explicit TestPassWithCustomConstructor(int v) : extraVal(v) {}
 
   void runOnOperation() override {}
 
   std::unique_ptr<mlir::Pass> clone() const {
     return TestPassWithCustomConstructorBase<
         TestPassWithCustomConstructor>::clone();
   }