mtrofin added a comment. In D91567#2401021 <https://reviews.llvm.org/D91567#2401021>, @dblaikie wrote:
> 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?) There was already an AlwaysInliner for the NPM, just wasn't used. So D86966 <https://reviews.llvm.org/D86966> hooked that up in the NPM, basically. The implementation of that AlwaysInliner is separate from the Inliner pass. See Transforms/IPO/AlwaysInliner.cpp lines 36 - 114, vs Inliner.cpp, from 687 onwards. >>> 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?) Yup, see BackendUtil.cpp:634. Can't comment on the latter problem. >> 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)? Yes. Nit on the first sentence: it's not "a separate process *within* the non-always inliner". It's a separate module pass part of the module pass manager that wraps the full inliner and related passes. > (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)) See the note to @aeubanks' - indeed, we can remove the NPM AlwaysInliner as result of this change. 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
