[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

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