[Mlir-commits] [mlir] [mlir][pass] Allow reregistration of pass with same typeids (PR #72067)

[Oleksandr Alex Zinenko]

================
@@ -181,4 +182,48 @@ TEST(PassManagerTest, PassInitialization) {
   EXPECT_TRUE(succeeded(pm.run(module.get())));
 }
 
+struct ReRegisterPass
+    : public PassWrapper<ReRegisterPass, OperationPass<ModuleOp>> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(ReRegisterPass)
+
+  ReRegisterPass(std::string annotation) : annotation(annotation) {}
+
+  std::string annotation;
+
+  void runOnOperation() override {
+    ModuleOp op = this->getOperation();
+    Builder builder(op);
+    op->walk([this, &builder](Operation *op) {
+      op->setAttr(annotation, builder.getUnitAttr());
+    });
+  }
+};
+
+TEST(PassManagerTest, PassReRegistration) {
+  MLIRContext context;
+  context.allowUnregisteredDialects();
+
+  std::string moduleStr = R"mlir(
+    module {
+      "custom.op1"() : () -> ()
+      "custom.op2"() : () -> ()
+    }
+  )mlir";
+
+  OwningOpRef<ModuleOp> module =
+      parseSourceString<ModuleOp>(moduleStr, &context);
+
+  // Instantiate and run pass in first configuration.
+  auto pm = PassManager::on<ModuleOp>(&context);
+  pm.addPass(std::make_unique<ReRegisterPass>("custom.first"));
+  EXPECT_TRUE(succeeded(pm.run(module.get())));
+  module->walk([](Operation *op) { EXPECT_TRUE(op->hasAttr("custom.first")); });
+
+  // Adding a "reconfiguration" of the pass, i.e., with a different annotation.
+  pm.addPass(std::make_unique<ReRegisterPass>("custom.second"));
----------------
ftynse wrote:

It looks like the problem this is trying to solve is this:

> The current behavior is counter-intuitive (or a bug) in that re-registering is a no-op but there's no warming/error/message that it's a no-op;

Do I understand correctly that registering a different constructor function for the same pass name + TypeID is currently allowed? This sounds undesirable to me, because it may lead to surprises, for example having two `PassRegistration` objects that register the same pass with different configuration may end up having the pass behavior depend on the order of construction of these objects, which may in turn depend on the order of linking.

That being said, I don't think this PR really solves that problem. Instead of prioritizing the first registered constructor, it will prioritize the last registered one. But ordering is still an issue.

I would rather consider removing the custom constructor callback for the pass. All pass classes could provide a (C++) default constructor, so re-registering a pass always points to the same constructor. This may be quite brutal to downstreams though...

https://github.com/llvm/llvm-project/pull/72067