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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D91567#2398637 <https://reviews.llvm.org/D91567#2398637>, @mtrofin wrote:

> In D91567#2398623 <https://reviews.llvm.org/D91567#2398623>, @dblaikie wrote:
>
>> In D91567#2398461 <https://reviews.llvm.org/D91567#2398461>, @mtrofin wrote:
>>
>>> In D91567#2398440 <https://reviews.llvm.org/D91567#2398440>, @dblaikie wrote:
>>>
>>>>> Performing the mandatory inlinings first simplifies the problem the full inliner needs to solve
>>>>
>>>> That confuses me a bit - is that suggesting that we don't run the AlwaysInliner when we are running the Inliner (ie: we only run the AlwaysInliner at -O0, and use the Inliner at higher optimization levels and let the Inliner do always inlining too)?
>>>> & sounds like this is suggesting that would change? That we would now perform always inlining separately from inlining? Maybe that's an orthogonal/separate change from one implementing the always inlining using the Inliner being run in a separate mode?
>>>
>>> In the NPM, we didn't run the AlwaysInliner until D86988 <https://reviews.llvm.org/D86988>. See also the discussion there. The normal inliner pass was, and still is, taking care of the mandatory inlinings if it finds them. Of course, if we completely upfronted those (which this patch can do), then the normal inliner wouldn't need to. I'm not suggesting changing that - meaning, it's straightforward for the normal inliner to take care of mandatory and policy-driven inlinings. The idea, though, is that if we upfront the mandatory inlinings, the shape of the call graph the inliner operates over is simpler and the effects of inlining probably more easy to glean by the decision making policy. There are trade-offs, though - we can increase that "ease of gleaning" by performing more function simplification passes between the mandatory inlinings and the full inliner.
>>
>> OK, so if I understand correctly with the old Pass Manager there were two separate passes (always inliner and inliner - they share some code though, yeah?)
>
> AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.

The NPM always inliner doesn't share any code with the NPM non-always inliner? (though this ( https://reviews.llvm.org/D86988 ) is the patch that added a separate always inliner to the NPM, right? And that patch doesn't look like it adds a whole new pass implementation - so looks like it's sharing some code with something?)

>> and they were run in the pass pipeline but potentially (definitely?) not adjacent?
>
> From what I can see, the legacy one was used only in the O0/O1 <https://reviews.llvm.org/owners/package/1/> cases, see clang/lib/CodeGen/BackendUtil,cpp:643. The full inliner isn't.

The full inliner isn't.. isn't run at -O0/-O1? So with the Legacy Pass Manager one inliner (always or non-always) was used in a given compilation, not both? (so I guess then the non-always inliner did the always-inlining in -O2 and above in the old pass manager? But didn't have the same recursive always inlining miss that the NPM non-always inliner had?)

> New pass manager survived for quite a while with only one inlining pass, that included a mandatorily strong preference for inlining always-inline functions? But still missed some recursive cases. So D86988 <https://reviews.llvm.org/D86988> made the always inliner run right next to/before the inliner in the NPM.
>
>> Now there's tihs patch, to implement the AlwaysInliner using the inliner - but is also changing the order of passes to improve optimization opportunities by doing some cleanup after always inlining?
>
> It doesn't quite change the order D86988 <https://reviews.llvm.org/D86988> introduced. Specifically, D86988 <https://reviews.llvm.org/D86988> ran AlwaysInliner (a module pass) first, then let the Inliner and function optimizations happen.
> This patch keeps the order between doing mandatory inlinings and inlinings. But, in addition, if in the future we want to also perform some of the function passes that happen in the inliner case, to help the full inliner, we can more easily do so.

I'm still a bit confused/trying to understand better - am I understanding correctly when I say: D86988 <https://reviews.llvm.org/D86988> added always inlining (for the NPM) as a separate process within the non-always inliner? And this patch you're proposing. breaks always inlining out into a separate pass proper, so that at some point, if someone wanted to (but not being done in this patch) they could put some passes in between the two runs of inlining (always and non-always)?

(I guess one thing I might be especially confused about is the "reuse X to do Y" would, to me, immediately lead me to think about "so I expect to see a bunch of deleted code because X presumably was doing a bunch of stuff itself that it now doesn't have to" - but I guess that's not the case here? (at least I don't see a large bunch of deletion I'd expect to see if some kind of inlining implementation was being deleted))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567