[PATCH] D23299: [PM] Port the always inliner to the new pass manager in a much more minimal and boring form than the old pass manager's version.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D23299#512879, @davidxl wrote:

> yes, I understand the module pass is quite simple and straightforward, but it seems to me the legacy always inliner is even simpler -- as it is simply one 'instantiation' of a common inliner implementation with one inline cost hook provided :) Have we compared the pros-and-cons of the two approaches?


I mean, yes. ;] I didn't do this without some careful thought.

> The pro of using module pass include : avoid building CG so it might be a compile time win (depending on how expensive GlobalDCE is).




- Avoid coupling the always inliner which needs no cost model to an inliner built entirely around cost modeling
- Avoid abstractions between the always cost model (or lack there of) and a concrete cost model
- Avoid the complexity of rigging up and potentially emitting remarks when the decision of whether or not to inline is completely predictable from source.

> The cons I can see:

> 

> 1. need to insert life time marker at O0 and depend on stack coloring to be turned on at O0 (which can be expensive)


If we need to do this at all. It isn't clear that we do. I've specifically said we can add alloca merging as a follow-up if it proves necessary.

> 2. need to run GlobalDCE


This adds no complexity though. The goal is to de-couple things which should make it overall more simple.

> 3. It is likely in the future CG is needed at O0 for other reasons, then the benefit of avoiding CG will be gone.


I don't see how this can be a con... It seems circular.

> 4. less code sharing.


I disagree. This shares all relevant logic with the existing inliner via the InlineFunction routine.

> Did I miss others?


I outlined all of the ones I saw in the patch description already (and it does include a couple you didn't mention) but I also described why I don't find them compelling to use a more complex approach.

It is probably worth highlighting that this new pass requires *less code* than the old version does even if we don't count any of the common inliner code or logic shared with other inliners. Despite the fact that some of that code only exists to support this pass's use case.

In https://reviews.llvm.org/D23299#512903, @davidxl wrote:

> For the record, I am all for a more general Module-pass based inliner.  The motivation is that it allows us to do a very quick round of priority based inlining (e.g, wrapper call inliing or other top-down heuristics) to avoid the limitation of bottom-up inlining.   However the general framework still needs CG (and update) though.


I think that is a completely different discussion. My goal is for the always inliner to be the simplest thing possible that merely inlines function bodies based on a single signal: the alwaysinline attribute.

In https://reviews.llvm.org/D23299#512909, @eraman wrote:

> > In https://reviews.llvm.org/D23299#512749, @davidxl wrote:
>
> > 
>
> > >
>
> > 
>
> > 
>
> > Did I miss others?
>
>
> When used in the -O1 pipeline, we might end up inlining less with the new AlwaysInliner.


The fact that -O1 uses the always inliner ... doesn't make much sense IMO. In fact, I suspect most don't realize that this is the case. It dates from r89464 in 2009 when this logic was added as part of some code dump of option parsing -- I suspect ported from the python driver wrapper. I don't see any particular logic for this model. I strongly suspect we should do something closer to -Os's inlining logic at -O1.

And I think we have the flexibility to do exactly this with the new PM. I'm not changing the old pass manager in any way.

> This is because the isInlineViable call scans the callee for conditions that disallow inlining and it matters whether some function pass gets rid of them (if they are in unreachable paths, for example). In the case of existing CGSCC AlwaysInliner pass, since we optimize a CGSCC node with function passes before moving to its parent, isInlineViable could get more precise result. No such cleanup happens in the module pass (unless we run some cleanup passes before the module pass). I don't think this difference matters much in practice though.


Yea, this is a difference, but I strongly agree it doesn't matter in practice.

Notably, we'd really like the alwaysinline to not be a hint but a guarantee. As such, it seems like a much bigger problem if some code has this attribute and relies on DCE or something else to be viable for inlining.

In https://reviews.llvm.org/D23299#513854, @davidxl wrote:

> I don't know why it is a bad thing to use a super simple cost model for always-inliner (as done by Old pass manager).
>
> Anyway, I don't think this patch should be held because of the difference in opinions here: it is not yet on by default and if we see problems, it can always be revisited (assuming implementing this as regular CG based inliner in new PM is possible) later.
>
> I do think a follow up is needed to handle always-inline callsite attribute.
>
> lgtm


Thanks. I'll try to re-invigorate the always-inline refinement that was already underway.


https://reviews.llvm.org/D23299