Re: [PATCH] Clean up early inliner

[Richard Guenther <rguenther at suse dot de>]

On Wed, 7 Apr 2010, Jan Hubicka wrote:

> > I don't think we are losing a feature that worked reliably.  You still
> > can be lucky with the patch, though.  In fact I think we should warn
> > whenever the address of an always-inline function is taken.
> 
> For the case of always inline callback of always inline function, this
> shouhd've worked reliably. I can imagine this would even make sense for
> stuff like for_each_rtx if that was not recursive and cheap enough.  I think
> Martin added a testcase to testsuite for this too, but I guess it is now
> the lucky one.
> 
> Another case where we can now fail is when always inline is passed as callback:
> __attribute__ ((always_inline)) int a(void)
> {
> }
> 
> int
> t(int (*a)(void))
> {
>   return a();
> }
> 
> with some extra effort needed to make topological sorter to put t before a.
> 
> 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.

> 
> > > I am leaving soon, so give me some time to thing about the cyclic graphs.
> > > I am not completely happy about the fact that topological sorter now ignore
> > > all callers of always inline functions, at least that it does so unconditionally.
> > > It will lead to some pesimization of those.
> > > (i.e. if you have A that calls always inline B that calls C, topological sorter
> > > will no longer make C early optimized before A that will result in early inliner
> > > not inlining the whole chaing A->B->C.  This is probably something you don't want
> > > when you care about performance enough to play with always inlines)
> > 
> > Well.  With that situation you absolutely want to inline B into all
> > callers first (otherwise you effectively make inline copy of C in B
> > always-inline, disregarding any costs), only then consider C.  Thus,
> 
> 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).

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

Richard.