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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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:
> Meinersbur wrote:
> > 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?
> @Meinersbur the whole *Changed* stuff is not state-based, so I naturally choose that path. And I'd favor it anyway, as it's more explicit. But that's not a strong opinion.
Thanks @Meinersbur ; this is great explanation!
@serge-sans-paille  can you capture this in the comment describing the getAnalysis() interface you changed? (or point to a doc if this is documented somewhere)

Also it wasn't obvious to me that this was a module pass (the method is called  `runOnFunction` here, and I didn't check the declaration in the class to see that it wasn't an override).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81236