[PATCH] D98103: [NPM] Do not run function simplification pipeline unnecessarily

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks accepted this revision.
aeubanks added a comment.

looks good, just some nits on comments



================
Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:558-559
+    // The assumption here is that CGSCC passes are well-behaved, in that, upon
+    // changing a function, they call updateCGAndAnalysisManagerForCGSCCPass,
+    // updateCGAndAnalysisManagerForFunctionPass, or otherwise need to
+    // invalidate FunctionStatusAnalysis explicitly (or clear the AM because the
----------------
this is no longer relevant


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1033
+    FSP.addPass(RequireAnalysisPass<FunctionStatusAnalysis, Function>());
+  MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FSP)));
 
----------------
mtrofin wrote:
> aeubanks wrote:
> > mtrofin wrote:
> > > aeubanks wrote:
> > > > do we need to add a pass to invalidate FunctionStatusAnalysis after the inliner pipeline so that CGSCC pipelines after the inliner pipeline aren't skipped?
> > > done
> > We already have `InvalidateAnalysisPass<FunctionStatusAnalysis>` for that.
> > It'd be nice if we could add it inside `buildInlinerPipeline()` since it's logically part of the inliner pipeline, but we only use it in one place so maybe it doesn't matter so much
> done for the InvalidateAnalysisPass, thanks for pointing it out!
> 
> I want to rationalize a bit better things in the module inliner wrapper pass, so the invalidation of the status analysis should fall in in that patch.
actually, maybe having a bool param in `createCGSCCToFunctionPassAdaptor` on whether to check for FunctionStatusAnalysis is better than adding the invalidation pass
maybe we could even have the adaptor run the RequireAnalysisPass (or some manual version of it)

just something to consider, not blocking


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98103