Re: [PATCH] Clean up early inliner

[Jan Hubicka <hubicka at ucw dot cz>]

> > 
> > But we can indeed declare those just invalid and warn (as well as warn for always
> > inline externally visible functions that are sort of insane too).
> 
> Indeed.

Note that there is also code to silence the sorry message for uninlined calls
to always_inlines that were previously indirect calls.  I guess if we go this
route, we need to check that kernel builds (or is going to be fixed for that)
as that is major consumer of this attribute as far as I can tell.

With externally visible always_inlines and without always_inline code in the
main inliner, we will end up sorrying on those with LTO.

> > Not really. Early inliner is basically inlining for size (modulo the
> > early_insns fuzz) so it would end up inlining C into all functions wher B was
> > inlined if it was performed again.
> 
> Of course.  But you have the may-grow-factor and the optimize out
> predicates.  I can think of unfortunate inlining decisions here if
> we don't eliminate always-inline edges first (which is really what
> the topological sort adjustment tries to do by breaking cycles at
> the right point).

Hmm, if the inliner thinks that inlining C into B is going to increase B's size
by less than early_inlining_insns, it will think so about inlining of C into A
after inlining B. The way those predicates are computed is to first evaluate
benefits of inlining C into B and later benefits of inlining of B into A so they
are transitive.

There is LARGE_FUNCTION_GROW limit and yes, theoretically early inliner may hit
this one.  This is bad case anyway - early inliner makes no effort to
prioritize the more important inlines so it will end up with abirtrary
solution.  Also always_inlines really should not be accounted into this limit
(they are partly in current implementation) since otherwise inlining large
always_inline will make the function to hit the limit and fail in further
automatic inlining.

In practice LARGE_FUNCTION_GROW does not hit for early inlining except for
artifical testcases I would say (because EARLY_INLINING_INSNS is small and
thus the code growth is limited)
> 
> > (if we are serious about early_insns fuzz, I guess early inliner ought
> > to then ignore all always_inline functions and handle inlining of C by
> > iteration after deciding to inline B into A).
> > 
> > > the sorting we really want is unordered (B, C), A.  Note that the
> > > breaking of cycles at the right points is required to do a sane
> > > and complete job with always-inlines.
> > 
> > Yep, one needs to break cycles, but since most of callgraph are acyclic, it is
> > IMO bad idea to pesimize those.  We already have code to find strongly
> > connected components, so we can probably ignore the callees only for always
> > inline functions participating in those.
> 
> Well, it was you who wanted to avoid computing SCCs ;)  But anyway,
> what we want is given the above example A -> B -> C where the
> edge A -> B is always-inline is for purpose of computing the postorder
> virtually do the inline expansion of the always-inline node, thus
> consider A -> B plus A -> (all callees of B) and disregard B -> C
> (B is not really there, but inlined).  Of course that needs to be
> done recursively for A -> B -> C -> D with the first two being
> always-inline edges.  It doesn't fit the postorder computation

Yep, that would work too (and give better solution for SCCs). But indeed such
clusterization would be more work.

> too easy, so my patch makes it work reasonably (and I really doubt
> the situation that you have always-inline function calling a
> function that you still want to be inlined is common).

Yep, it is all nitpicking for sure ;)
I honestly have problem of deciding whether it is more common to have always
inlines in recursive callgraphs or always_inline functions that calls functions
that are candidates for normal inlining.  I guess both are relatively side
cases as the always_inline itself...

Well, I guess we can go with the patch as it is and play with topological sorting
improvements incrementally.  It would also be nice to get the warnings at place so
we see how commonly indirect calls to always_inline and such appear.

Honza
> 
> Richard.