Fix issue with running the same PassManager twice

[modiking]

Fixes #58939 by re-initializing the moved from passmanagers in the inliner

Testing:
ninja check with new test

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (modiking)

Changes

Fix by re-initializing the moved from passmanagers in the inliner

Testing:
ninja check with new test

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/IPO/Inliner.cpp (+5)
• (modified) llvm/unittests/CodeGen/PassManagerTest.cpp (+24)

diff --git a/llvm/lib/Transforms/IPO/Inliner.cpp b/llvm/lib/Transforms/IPO/Inliner.cpp
index a9747aebf67bb..33aeee4a0d100 100644
--- a/llvm/lib/Transforms/IPO/Inliner.cpp
+++ b/llvm/lib/Transforms/IPO/Inliner.cpp
@@ -628,6 +628,11 @@ PreservedAnalyses ModuleInlinerWrapperPass::run(Module &M,
         createDevirtSCCRepeatedPass(std::move(PM), MaxDevirtIterations)));
 
   MPM.addPass(std::move(AfterCGMPM));
+
+  // If this is run again (e.g. same PassManager invoked twice)
+  // these need to be re-initialized after being moved.
+  PM = CGSCCPassManager();
+  AfterCGMPM = ModulePassManager();
   MPM.run(M, MAM);
 
   // Discard the InlineAdvisor, a subsequent inlining session should construct
diff --git a/llvm/unittests/CodeGen/PassManagerTest.cpp b/llvm/unittests/CodeGen/PassManagerTest.cpp
index d3a410f5450cc..8905ee7e1b338 100644
--- a/llvm/unittests/CodeGen/PassManagerTest.cpp
+++ b/llvm/unittests/CodeGen/PassManagerTest.cpp
@@ -264,4 +264,28 @@ TEST_F(PassManagerTest, DiagnosticHandler) {
               std::string::npos);
 }
 
+TEST_F(PassManagerTest, RunTwice) {
+  if (!TM)
+    GTEST_SKIP();
+
+  M->setDataLayout(TM->createDataLayout());
+
+  LoopAnalysisManager LAM;
+  FunctionAnalysisManager FAM;
+  CGSCCAnalysisManager CGAM;
+  ModuleAnalysisManager MAM;
+  PassBuilder PB(TM.get());
+  PB.registerModuleAnalyses(MAM);
+  PB.registerCGSCCAnalyses(CGAM);
+  PB.registerFunctionAnalyses(FAM);
+  PB.registerLoopAnalyses(LAM);
+  PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
+
+  ModulePassManager MPM;
+  MPM.addPass(PB.buildPerModuleDefaultPipeline(OptimizationLevel::O2));
+
+  MPM.run(*M, MAM);
+  MPM.run(*M, MAM);
+}
+
 } // namespace

[aeubanks]

this isn't the right solution. if we're calling ModuleInlinerWrapperPass::run() more than once, we're going to keep adding random passes/sub-pass managers to MPM and running those.

we really shouldn't be creating and running pass managers within a run(). could you see if we could somehow make it so we do all the pass management once? will probably involve modifying PassBuilderPipelines.cpp

[aeubanks]

put this in llvm/unittests/IR/PassManagerTest.cpp instead

[modiking]

this isn't the right solution. if we're calling ModuleInlinerWrapperPass::run() more than once, we're going to keep adding random passes/sub-pass managers to MPM and running those.

we really shouldn't be creating and running pass managers within a run(). could you see if we could somehow make it so we do all the pass management once? will probably involve modifying PassBuilderPipelines.cpp

Yeah it really is strange that a pass contains pass managers. I don't have too much context here but reading through what MIWP does it seems to be a pass manager in every aspect expect being a PassManager. It looks like a distinction is that MIWP executes a PassManager at pass runtime which depending on it's internal state:

  if (MaxDevirtIterations == 0)
    MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(PM)));
  else
    MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
        createDevirtSCCRepeatedPass(std::move(PM), MaxDevirtIterations)));

But given MaxDevirtIterations is const this isn't really a runtime decision. It seems like MIWP can be replaced with a regular MPM and the decision above be made at pipeline construction. Is that the direction you're suggesting?

[aeubanks]

But given MaxDevirtIterations is const this isn't really a runtime decision. It seems like MIWP can be replaced with a regular MPM and the decision above be made at pipeline construction. Is that the direction you're suggesting?

Yes, exactly