Re: PR tree-optimization/51680 (not inlining comdats)

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sun, Jan 8, 2012 at 12:38 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this patch fixes first half of the PR. ?The PR is about a function that is supposed
> to be inlined at -O2. ?The function contains two calls, one indirect and inlining
> the indirect call makes things a lot better. GCC however things that replacing call
> by two calls at -O2 is a bad idea.
>
> This patch solves first part of it. ?The function in question is COMDAT. We already
> know that C++ coding style makes COMDATs quite rarely to be shared and at -Os we
> already handle them sort of similarly to static ufnctions. ?We however don't do that
> at -O2. ?This patch makes it happen in both cases.
>
> Bootstrapped/regtested x86_64-linux, comitted.
> Will commit the testcase with the followup fix.

Err ...

> ? ? ? ?PR tree-optimization/51680
> ? ? ? ?* ipa-inline.c (want_inline_small_function_p): Be more lax on functions
> ? ? ? ?whose inlining reduce unit size.
>
> Index: ipa-inline.c
> ===================================================================
> --- ipa-inline.c ? ? ? ?(revision 182949)
> +++ ipa-inline.c ? ? ? ?(working copy)
> @@ -482,21 +482,13 @@ want_inline_small_function_p (struct cgr
> ? ? ? ? ? e->inline_failed = CIF_MAX_INLINE_INSNS_SINGLE_LIMIT;
> ? ? ? ? ?want_inline = false;
> ? ? ? ?}
> - ? ? ?else if (!DECL_DECLARED_INLINE_P (callee->decl)
> - ? ? ? ? ? ? ?&& !flag_inline_functions)
> - ? ? ? {
> - ? ? ? ? ?e->inline_failed = CIF_NOT_DECLARED_INLINED;
> - ? ? ? ? want_inline = false;
> - ? ? ? }
> - ? ? ?else if (!DECL_DECLARED_INLINE_P (callee->decl)
> - ? ? ? ? ? ? ?&& growth >= MAX_INLINE_INSNS_AUTO)
> - ? ? ? {
> - ? ? ? ? ?e->inline_failed = CIF_MAX_INLINE_INSNS_AUTO_LIMIT;
> - ? ? ? ? want_inline = false;
> - ? ? ? }
> - ? ? ?/* If call is cold, do not inline when function body would grow.
> - ? ? ? ?Still inline when the overall unit size will shrink because the offline
> - ? ? ? ?copy of function being eliminated.
> + ? ? ?/* Before giving up based on fact that caller size will grow, allow
> + ? ? ? ? functions that are called few times and eliminating the offline
> + ? ? ? ?copy will lead to overall code size reduction.
> + ? ? ? ?Not all of these will be handled by subsequent inlining of functions
> + ? ? ? ?called once: in particular weak functions are not handled or funcitons
> + ? ? ? ?that inline to multiple calls but a lot of bodies is optimized out.
> + ? ? ? ?Finally we want to inline earlier to allow inlining of callbacks.
>
> ? ? ? ? This is slightly wrong on aggressive side: ?it is entirely possible
> ? ? ? ? that function is called many times with a context where inlining
> @@ -509,24 +501,39 @@ want_inline_small_function_p (struct cgr
> ? ? ? ? first, this situation is not a problem at all: after inlining all
> ? ? ? ? "good" calls, we will realize that keeping the function around is
> ? ? ? ? better. ?*/
> - ? ? ?else if (!cgraph_maybe_hot_edge_p (e)
> - ? ? ? ? ? ? ?&& (DECL_EXTERNAL (callee->decl)
> + ? ? ?else if (growth <= MAX_INLINE_INSNS_SINGLE

That surely should be MAX_INLINE_INSNS_AUTO instead.  At _least_.
You are triggering very much more inlining during early inlining now -
I don't see
where we did this for -Os already as you claim.  In fact it is totally
against the spirit of early inlining now :(

Why does IPA inlining not figure out that inlining the indirect call is good?

> + ? ? ? ? ? ? ?/* Unlike for functions called once, we play unsafe with
> + ? ? ? ? ? ? ? ? COMDATs. ?We can allow that since we know functions
> + ? ? ? ? ? ? ? ? in consideration are small (and thus risk is small) and
> + ? ? ? ? ? ? ? ? moreover grow estimates already accounts that COMDAT
> + ? ? ? ? ? ? ? ? functions may or may not disappear when eliminated from
> + ? ? ? ? ? ? ? ? current unit. With good probability making aggressive
> + ? ? ? ? ? ? ? ? choice in all units is going to make overall program
> + ? ? ? ? ? ? ? ? smaller.
> +
> + ? ? ? ? ? ? ? ? Consequently we ask cgraph_can_remove_if_no_direct_calls_p
> + ? ? ? ? ? ? ? ? instead of
> + ? ? ? ? ? ? ? ? cgraph_will_be_removed_from_program_if_no_direct_calls ?*/
> + ? ? ? ? ? ? ? && !DECL_EXTERNAL (callee->decl)
> + ? ? ? ? ? ? ? && cgraph_can_remove_if_no_direct_calls_p (callee)
> + ? ? ? ? ? ? ? && estimate_growth (callee) <= 0)
> + ? ? ? ;
> + ? ? ?else if (!DECL_DECLARED_INLINE_P (callee->decl)
> + ? ? ? ? ? ? ?&& !flag_inline_functions)
> + ? ? ? {
> + ? ? ? ? ?e->inline_failed = CIF_NOT_DECLARED_INLINED;
> + ? ? ? ? want_inline = false;
> + ? ? ? }
> + ? ? ?else if (!DECL_DECLARED_INLINE_P (callee->decl)
> + ? ? ? ? ? ? ?&& growth >= MAX_INLINE_INSNS_AUTO)
> + ? ? ? {
> + ? ? ? ? ?e->inline_failed = CIF_MAX_INLINE_INSNS_AUTO_LIMIT;
> + ? ? ? ? want_inline = false;
> + ? ? ? }
> + ? ? ?/* If call is cold, do not inline when function body would grow. */
> + ? ? ?else if (!cgraph_maybe_hot_edge_p (e))
>
> - ? ? ? ? ? ? ? ? ?/* Unlike for functions called once, we play unsafe with
> - ? ? ? ? ? ? ? ? ? ? COMDATs. ?We can allow that since we know functions
> - ? ? ? ? ? ? ? ? ? ? in consideration are small (and thus risk is small) and
> - ? ? ? ? ? ? ? ? ? ? moreover grow estimates already accounts that COMDAT
> - ? ? ? ? ? ? ? ? ? ? functions may or may not disappear when eliminated from
> - ? ? ? ? ? ? ? ? ? ? current unit. With good probability making aggressive
> - ? ? ? ? ? ? ? ? ? ? choice in all units is going to make overall program
> - ? ? ? ? ? ? ? ? ? ? smaller.
> -
> - ? ? ? ? ? ? ? ? ? ? Consequently we ask cgraph_can_remove_if_no_direct_calls_p
> - ? ? ? ? ? ? ? ? ? ? instead of
> - ? ? ? ? ? ? ? ? ? ? cgraph_will_be_removed_from_program_if_no_direct_calls ?*/
>
> - ? ? ? ? ? ? ? ? ?|| !cgraph_can_remove_if_no_direct_calls_p (callee)
> - ? ? ? ? ? ? ? ? ?|| estimate_growth (callee) > 0))
> ? ? ? ?{
> ? ? ? ? ? e->inline_failed = CIF_UNLIKELY_CALL;
> ? ? ? ? ?want_inline = false;