Re: [PATCH] Clean up early inliner

[Richard Guenther <rguenther at suse dot de>]

On Wed, 7 Apr 2010, Jan Hubicka wrote:

> > > 
> > > 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.

I think the idea was once the early inliner catches all cases we support
by design, emit diagnostics for all remaining call edges to
always-inline functions (basically, try to reclaim their bodies and
complain if that's not possible).  Thus you'd never write their
body to the LTO IL.

> > > 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)

Yeah, I guess the case I am worried about is as important as
the case you are worrying about ;)  I just claim it shouldn't make
any difference if in A -> B -> C we first inline C into B and then
B into A or if we go and inline B into A first.

> > 
> > > (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.

Maybe, I'm not sure ;)  The code as is is a little unobvious and
undocumented ;)

> > 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...

Yes.  But if we want to be sure to handle all always-inlines we have
to support during early inlining we have to get the cycle breaking 
correct.  The main aim of the patch is to simplify the inliner code,
untwisting incremental and IPA inlining modes.

> 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.

I'll see if I can implement the warnings - I have to look at diagnostic
regression of the patch anyway it seems.

Thanks,
Richard.

> Honza
> > 
> > Richard.
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex