mtrofin added a comment. In D91567#2403173 <https://reviews.llvm.org/D91567#2403173>, @aeubanks wrote:
> In D91567#2400699 <https://reviews.llvm.org/D91567#2400699>, @mtrofin wrote: > >> In D91567#2400207 <https://reviews.llvm.org/D91567#2400207>, @aeubanks wrote: >> >>> What about removing the existing AlwaysInlinerPass and replacing it with >>> this one? Or is that something you were planning to do in a follow-up >>> change? >> >> That's the plan, yes. >> >>>> open the opportunity to help the full inliner by performing additional >>>> function passes after the mandatory inlinings, but before the full inliner >>> >>> This change doesn't run the function simplification pipeline between the >>> mandatory and full inliner though, only >>> >>> if (AttributorRun & AttributorRunOption::CGSCC) >>> MainCGPipeline.addPass(AttributorCGSCCPass()); >>> >>> if (PTO.Coroutines) >>> MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0)); >>> >>> // Now deduce any function attributes based in the current code. >>> MainCGPipeline.addPass(PostOrderFunctionAttrsPass()); >> >> Right - my point was that we could more easily explore doing so by having >> this new AlwaysInliner separate. In this patch I included those additional >> passes because I thought they may be necessary or beneficial, but I can >> remove them as a first step if they are not necessary. @jdoerfert - should >> the Attributor be run post-always inlining, or is it fine to not be run? > > I'd say start off without running any passes and keeping the status quo (i.e. > an NFC patch), then explore adding passes between the inliners in a future > patch. Done >>> And is there any evidence that running the function simplification pipeline >>> between the mandatory and full inliner is helpful? It could affect compile >>> times. >> >> In the ML-driven -Oz case, we saw some marginal improvement. I haven't in >> -O3 cases using the "non-ml" inliner. I suspect that it helps in the ML >> policy case (both -Oz and -O3, on which we're currently working), because: >> 1) the current -Oz takes some global (module-wide) features, so probably >> simplifying out the trivial cases helps; and 2) we plan on taking into >> consideration regions of the call graph, and the intuition is that >> eliminating the trivial cases (mandatory cases) would rise the visibility >> (for a training algorithm) of the non-trivial cases. >> >>> I'd think that adding the mandatory inliner right before the full inliner >>> in the same CGSCC pass manager would do the job. e.g. add it in >>> `ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before >>> `PM.addPass(InlinerPass());` >> >> It would, and what I'm proposing here is equivalent to that, but the >> proposal here helps with these other explorations, with (arguably) not much >> of a difference cost-wise in itself (meaning, of course, if we discover >> there's benefit in running those additional passes, we pay with compile >> time, but in of itself, factoring the always inliner in its own wrapper, or >> in the same wrapper as the inliner, doesn't really come at much of a cost). >> >> Now, if we determine there is no value, we can bring it back easily - wdyt? > > I'll run this through llvm-compile-time-tracker to see what the compile time > implications are. You mean for the variant where we ran some of the function passes, or you'd try running all of them? Probably the latter would be quite interesting as a 'worst case'. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91567/new/ https://reviews.llvm.org/D91567 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
