[PATCH] D81236: Improve LegacyPassManager API to correctly report modified status

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/LoopExtractor.cpp:135
+  bool Changed = false;
+  LoopInfo &LI = getAnalysis<LoopInfoWrapperPass>(F, &Changed).getLoopInfo();
 
----------------
serge-sans-paille wrote:
> ekatz wrote:
> > mehdi_amini wrote:
> > > ekatz wrote:
> > > > serge-sans-paille wrote:
> > > > > ekatz wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ekatz wrote:
> > > > > > > > Maybe I am missing something, but isn't `LoopExtractor` dependent on `BreakCriticalEdges`? I mean, what are the transformations that the `LoopInfo` analysis depends on?
> > > > > > > > what are the transformations that the LoopInfo analysis depends on?
> > > > > > > 
> > > > > > > No transformations:
> > > > > > > 
> > > > > > > ```
> > > > > > >   void LoopInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {                                                                                                                                              
> > > > > > >     AU.setPreservesAll();
> > > > > > >     AU.addRequiredTransitive<DominatorTreeWrapperPass>();
> > > > > > >   }
> > > > > > >   
> > > > > > > ```
> > > > > > So why would the `Changed` ever be set to `true`?
> > > > > It's set to True because of the on the fly pass evaluation: asking for `LoopInfo` triggers the whole dependencies, including `BreakCriticalEdges`. That's why the API change I propose only impacts on the fly passes.
> > > > I see.
> > > What is "on the fl pass evaluation" in this context? I'm missing why `BreakCriticalEdges` isn't triggered before `LoopExtractor::runOnFunction` invocation?
> > To add to your question, just to make sure I understand it correctly- if `getAnalysis` was never called, then `BreakCriticalEdges` wasn't either?
> That's what I observed, yes.
@mehdi_amini "On the fly" is a workaround for the legacy pass manager where coarser grain passes (here: ModulePass) cannot depend on finer-grain passes (here: FunctionPass). 

Usually, the legacy pass manager would call all dependency passes before the pass itself, such that dependency pass'es runOnFunction is called be the PassManager, and receives the Changed status of that dependency. 
When a ModulePass requests a FunctionPass analysis, the PassManager will not have called it in advance, but it will call it during getAnalsysis(). If that analysis modifies the IR, the changed status was not propagated up the PassManager stack, in this case the MPPassManager.

Since the getOnTheFlyPass is actually a method of MPPassManager, `Changed` in `MPPassManager::runOnModule` could also me made a field which `getOnTheFlyPass` sets to true if `FPP->run(F)` returns and we would not have to propagate the Changes status though method arguments.

@serge-sans-paille Did you actually consider such an alternative design?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81236/new/

https://reviews.llvm.org/D81236