[Inliner] Handle multiple `run` invocation of `ModuleInlinerWrapperPass` (NFC).

[michele.scandale]

Summary

The current implementation ModuleInlinerWrapperPass uses three pass
managers. In the run function two of the three pass manager are
always moved into the third one. This lead to undefined behavior (e.g.
crash) if the ModuleInlinerWrapperPass::run is executed more than
once on the same instance of the pass because additional run perform
again the move operations on object whose state is unspecified.
This commit addresses the issue by ensuring that the move operations are
performed only on the first invocation of ModuleInlinerWrapperPass::run.

[mtrofin]

asking to learn - what's the motivation for running ::run more than once / why not have different instances of ModuleInlinerWrapperPass?

[michele.scandale]

In D130952#3692584, @mtrofin wrote:

asking to learn - what's the motivation for running ::run more than once / why not have different instances of ModuleInlinerWrapperPass?

The use case I'm looking into is a system processing several modules with a custom optimization pipeline where I'd like to pay the setup cost of building the pass manager only once rather than for each module being processed.
This is something that used to work (modulo bugs once in a while) with the legacy pass manager. I'm trying this with the new pass manager infrastructure, and found few issues while experimenting (the issue addressed here, D130954 and D130955).

[mtrofin]

In D130952#3693697, @michele.scandale wrote:

In D130952#3692584, @mtrofin wrote:

asking to learn - what's the motivation for running ::run more than once / why not have different instances of ModuleInlinerWrapperPass?

The use case I'm looking into is a system processing several modules with a custom optimization pipeline where I'd like to pay the setup cost of building the pass manager only once rather than for each module being processed.
This is something that used to work (modulo bugs once in a while) with the legacy pass manager. I'm trying this with the new pass manager infrastructure, and found few issues while experimenting (the issue addressed here, D130954 and D130955).

Wondering, is this something that's intentionally supported though?

Setting that question aside, maybe it'd make more sense we moved the finalization stuff in a ModuleInlinerWrapperPass::finish() or something like that, and then we just assert(Finalized) in ::run; that would address a current oddity with the wrapper, in that the pass composition is somewhat deferred until the run.

wdyt?

[michele.scandale]

Wondering, is this something that's intentionally supported though?

Based on the discussion in D130954, it seems that currently there is no intention to support the use case I was experimenting with, therefore this change is not required.
From the initial performance analysis rebuilding the pass manager is much cheaper with the new pass manager than the legacy one, and for now I can use such alternative approach for my use case.

Setting that question aside, maybe it'd make more sense we moved the finalization stuff in a ModuleInlinerWrapperPass::finish() or something like that, and then we just assert(Finalized) in ::run; that would address a current oddity with the wrapper, in that the pass composition is somewhat deferred until the run.

wdyt?

Clients of ModuleInlinerWrapperPass would need to call finish before using it. I don't know if that acceptable or not as there is no finish for regular pass managers.
Perhaps it would be better to find a way to avoid the pass managers moves inside run. It seems that explicitly allocating the PassModel for a CGSCCPassManager would allow to keep a pointer to the CGSCCPassManager that would be returned via getPM, and still be able to prepare the module pass manager with the ModuleToPostOrderCGSCCPassAdaptor directly in the constructor of ModuleInlinerWrapperPass.

[mtrofin]

In D130952#3698544, @michele.scandale wrote:

Wondering, is this something that's intentionally supported though?

Based on the discussion in D130954, it seems that currently there is no intention to support the use case I was experimenting with, therefore this change is not required.
From the initial performance analysis rebuilding the pass manager is much cheaper with the new pass manager than the legacy one, and for now I can use such alternative approach for my use case.

Setting that question aside, maybe it'd make more sense we moved the finalization stuff in a ModuleInlinerWrapperPass::finish() or something like that, and then we just assert(Finalized) in ::run; that would address a current oddity with the wrapper, in that the pass composition is somewhat deferred until the run.

wdyt?

Clients of ModuleInlinerWrapperPass would need to call finish before using it. I don't know if that acceptable or not as there is no finish for regular pass managers.
Perhaps it would be better to find a way to avoid the pass managers moves inside run. It seems that explicitly allocating the PassModel for a CGSCCPassManager would allow to keep a pointer to the CGSCCPassManager that would be returned via getPM, and still be able to prepare the module pass manager with the ModuleToPostOrderCGSCCPassAdaptor directly in the constructor of ModuleInlinerWrapperPass.

Right, so then the lines 1160-1166 would be the responsibility of the user, and the main responsibility of the ModuleInlinerWrapperPass is to set up / clear out correctly the advisor. SGTM.

llvm/include/llvm/Transforms/IPO/Inliner.h

private:
   const InlineParams Params;
   const InlineContext IC;
   const InliningAdvisorMode Mode;
   const unsigned MaxDevirtIterations;
   // TODO: Clean this up so we only have one ModulePassManager.
   CGSCCPassManager PM;
   ModulePassManager MPM;
   ModulePassManager AfterCGMPM;
+  bool MPMFinalized;
 };
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_IPO_INLINER_H

llvm/lib/Transforms/IPO/Inliner.cpp

 }
 
 ModuleInlinerWrapperPass::ModuleInlinerWrapperPass(InlineParams Params,
                                                    bool MandatoryFirst,
                                                    InlineContext IC,
                                                    InliningAdvisorMode Mode,
                                                    unsigned MaxDevirtIterations)
     : Params(Params), IC(IC), Mode(Mode),
-      MaxDevirtIterations(MaxDevirtIterations) {
+      MaxDevirtIterations(MaxDevirtIterations), MPMFinalized(false) {
   // Run the inliner first. The theory is that we are walking bottom-up and so
   // the callees have already been fully optimized, and we want to inline them
   // into the callers so that our optimizations can reflect that.
   // For PreLinkThinLTO pass, we disable hot-caller heuristic for sample PGO
   // because it makes profile annotation in the backend inaccurate.
   if (MandatoryFirst) {
     PM.addPass(InlinerPass(/*OnlyMandatory*/ true));
     if (EnablePostSCCAdvisorPrinting)
if (!IAA.tryCreate(Params, Mode,
                       {CGSCCInlineReplayFormat}},
                      IC)) {
     M.getContext().emitError(
         "Could not setup Inlining Advisor for the requested "
         "mode and/or options");
     return PreservedAnalyses::all();
   }
 
+  if (!MPMFinalized) {
     // We wrap the CGSCC pipeline in a devirtualization repeater. This will try
     // to detect when we devirtualize indirect calls and iterate the SCC passes
     // in that case to try and catch knock-on inlining or function attrs
     // opportunities. Then we add it to the module pipeline by walking the SCCs
     // in postorder (or bottom-up).
     // If MaxDevirtIterations is 0, we just don't use the devirtualization
     // wrapper.
     if (MaxDevirtIterations == 0)
       MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(PM)));
     else
       MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
           createDevirtSCCRepeatedPass(std::move(PM), MaxDevirtIterations)));
 
     MPM.addPass(std::move(AfterCGMPM));
+    MPMFinalized = true;
+  }
   MPM.run(M, MAM);
 
   // Discard the InlineAdvisor, a subsequent inlining session should construct
   // its own.
   auto PA = PreservedAnalyses::all();
   if (!KeepAdvisorForPrinting)
     PA.abandon<InlineAdvisorAnalysis>();
   return PA;