[mlir][ODS] Fix copy ctor for generate Pass classes

[vinograd47]

Summary

Redirect the copy ctor to the actual class instead of
overwriting it with TypeID based ctor.

This allows the final Pass classes to have extra fields and logic for their copy.

[lattner]

Whoa, great catch. Only request: please merge the unit test into the existing test/lib/Pass stuff. Thank you!

[vinograd47]

In D104302#2823022, @lattner wrote:

Only request: please merge the unit test into the existing test/lib/Pass stuff.

I'm not sure. As far as I see, test/ folder contains LIT-based tests and test/lib contains auxiliary libraries for those tests. unittests/ folder contains GTest-based tests for some components, like mlir-tblgen. And the new tests relates to mlir-tblgen pass generation functionality. That's why I've put it into unittests/TableGen folder.

[lattner]

Do we need a unit test at all here? If I understand the patch correctly, you're saying that the canonicalize options aren't being copied over correctly when a pass is cloned. If that is true, then we can exercise this with a standard filecheck test of the canonicalize pass.

Unit tests impose an outsized burden on the project because they increase link and build time when doing check-mlir. Filecheck tests of existing tools are much cheaper.

[vinograd47]

Removed confusing comment about Canonicalization pass.

[vinograd47]

@lattner Sorry, my comment about Canonicalization pass was wrong and confusing (removed it).

We faced with this issue in our internal project. We have pass, which has extra fields not configurable from command line options. Those fields are used in production code, but are not needed in testing/debugging tools like mlir-opt. Due to the issue with copy ctors, those fields were lost during pass cloning.

Initially I thought that the Canonicalization pass will have the same issue. But after more careful look I've found that the the config options for greedy pattern driver are available via command line options and the pass constructor initializes GreedyRewriteConfig object from them.

Nevertheless, I still think that the missing copy ctor call is an issue. The mlir-opt tools and command line options are not mandatory and external projects might not use them.

[lattner]

Ok, LGTM, thanks!

mlir/include/mlir/Pass/Pass.h

 ///
 /// Derived function passes are expected to provide the following:
 ///   - A 'void runOnOperation()' method.
 ///   - A 'StringRef getName() const' method.
 ///   - A 'std::unique_ptr<Pass> clonePass() const' method.
 template <typename OpT = void> class OperationPass : public Pass {
 protected:
   OperationPass(TypeID passID) : Pass(passID, OpT::getOperationName()) {}
+  OperationPass(const OperationPass &) = default;
 
   /// Support isa/dyn_cast functionality.
   static bool classof(const Pass *pass) {
     return pass->getOpName() == OpT::getOperationName();
   }
 
   /// Return the current operation being transformed.
   OpT getOperation() { return cast<OpT>(Pass::getOperation()); }

 ///
 /// Derived function passes are expected to provide the following:
 ///   - A 'void runOnOperation()' method.
 ///   - A 'StringRef getName() const' method.
 ///   - A 'std::unique_ptr<Pass> clonePass() const' method.
 template <> class OperationPass<void> : public Pass {
 protected:
   OperationPass(TypeID passID) : Pass(passID) {}
+  OperationPass(const OperationPass &) = default;
 };
 
 /// A model for providing function pass specific utilities.
 ///
 /// Derived function passes are expected to provide the following:
 ///   - A 'void runOnFunction()' method.
 ///   - A 'StringRef getName() const' method.
 ///   - A 'std::unique_ptr<Pass> clonePass() const' method.

 public:
   /// Support isa/dyn_cast functionality for the derived pass class.
   static bool classof(const Pass *pass) {
     return pass->getTypeID() == TypeID::get<PassT>();
   }
 
 protected:
   PassWrapper() : BaseT(TypeID::get<PassT>()) {}
+  PassWrapper(const PassWrapper &) = default;
 
   /// Returns the derived pass name.
   StringRef getName() const override { return llvm::getTypeName<PassT>(); }
 
   /// A clone method to create a copy of this pass.
   std::unique_ptr<Pass> clonePass() const override {
     return std::make_unique<PassT>(*static_cast<const PassT *>(this));
   }
 };
 
 } // end namespace mlir
 
 #endif // MLIR_PASS_PASS_H

mlir/tools/mlir-tblgen/PassGen.cpp

 //===----------------------------------------------------------------------===//
 
 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 &) : {1}(::mlir::TypeID::get<DerivedT>()) {{}
+  {0}Base(const {0}Base &other) : {1}(other) {{}
 
   /// Returns the command-line argument attached to this pass.
   static constexpr ::llvm::StringLiteral getArgumentName() {
     return ::llvm::StringLiteral("{2}");
   }
   ::llvm::StringRef getArgument() const override { return "{2}"; }
 
   ::llvm::StringRef getDescription() const override { return "{3}"; }

mlir/unittests/TableGen/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS enums.td)
 mlir_tablegen(EnumsGenTest.h.inc -gen-enum-decls)
 mlir_tablegen(EnumsGenTest.cpp.inc -gen-enum-defs)
 add_public_tablegen_target(MLIRTableGenEnumsIncGen)
 
 set(LLVM_TARGET_DEFINITIONS structs.td)
 mlir_tablegen(StructAttrGenTest.h.inc -gen-struct-attr-decls)
 mlir_tablegen(StructAttrGenTest.cpp.inc -gen-struct-attr-defs)
 add_public_tablegen_target(MLIRTableGenStructAttrIncGen)
 
+set(LLVM_TARGET_DEFINITIONS passes.td)
+mlir_tablegen(PassGenTest.h.inc -gen-pass-decls -name TableGenTest)
+add_public_tablegen_target(MLIRTableGenTestPassIncGen)
+
 add_mlir_unittest(MLIRTableGenTests
   EnumsGenTest.cpp
   StructsGenTest.cpp
   FormatTest.cpp
   OpBuildGen.cpp
+  PassGenTest.cpp
 )
 
 add_dependencies(MLIRTableGenTests MLIRTableGenEnumsIncGen)
 add_dependencies(MLIRTableGenTests MLIRTableGenStructAttrIncGen)
+add_dependencies(MLIRTableGenTests MLIRTableGenTestPassIncGen)
 add_dependencies(MLIRTableGenTests MLIRTestDialect)
 
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../test/lib/Dialect/Test)
 include_directories(${CMAKE_CURRENT_BINARY_DIR}/../../test/lib/Dialect/Test)
 
 target_link_libraries(MLIRTableGenTests
   PRIVATE MLIRTableGen MLIRIR
   PUBLIC MLIRTestDialect
 )

mlir/unittests/TableGen/PassGenTest.cpp

+//===- PassGenTest.cpp - TableGen PassGen Tests ---------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Pass/Pass.h"
+
+#include "gmock/gmock.h"
+
+std::unique_ptr<mlir::Pass> createTestPass(int v = 0);
+
+#define GEN_PASS_REGISTRATION
+#include "PassGenTest.h.inc"
+
+#define GEN_PASS_CLASSES
+#include "PassGenTest.h.inc"
+
+struct TestPass : public TestPassBase<TestPass> {
+  explicit TestPass(int v) : extraVal(v) {}
+
+  void runOnOperation() override {}
+
+  std::unique_ptr<mlir::Pass> clone() const {
+    return TestPassBase<TestPass>::clone();
+  }
+
+  int extraVal;
+};
+
+std::unique_ptr<mlir::Pass> createTestPass(int v) {
+  return std::make_unique<TestPass>(v);
+}
+
+TEST(PassGenTest, PassClone) {
+  mlir::MLIRContext context;
+
+  const auto unwrap = [](const std::unique_ptr<mlir::Pass> &pass) {
+    return static_cast<const TestPass *>(pass.get());
+  };
+
+  const auto origPass = createTestPass(10);
+  const auto clonePass = unwrap(origPass)->clone();
+
+  EXPECT_EQ(unwrap(origPass)->extraVal, unwrap(clonePass)->extraVal);
+}

mlir/unittests/TableGen/passes.td

+//===-- passes.td - PassGen test definition file -----------*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+include "mlir/Pass/PassBase.td"
+include "mlir/Pass/PassBase.td"
+include "mlir/Rewrite/PassUtil.td"
+
+def TestPass : Pass<"test"> {
+  let summary = "Test pass";
+
+  let constructor = "::createTestPass()";
+
+  let options = RewritePassUtils.options;
+}