Created attachment 28476 [details]
pixman-combine-float.i.gz - the original full preprocessed source

Applying the changes from http://gcc.gnu.org/git/?p=gcc.git;a=commit;h=526b36a8a249c8c8698ca48ffeb8bff552f5a6fd to 'passes.c' in GCC 4.6 branch "fixes" the reduced testcase. But pixman still can't be compiled successfully, so also attaching the original full preprocessed source.

void combine_conjoint_xor_ca_float ()
{
    combine_channel_t j = pd_combine_conjoint_xor, k = pd_combine_conjoint_xor;
    a[0] = k (0, b, 0, a[0]);
    a[0] = k (0, b, 0, a[0]);
    a[0] = k (0, b, 0, a[0]);
    a[0] = j (0, c[0], 0, a[0]);
    a[0] = k (0, c[0], 0, a[0]);
    a[0] = k (0, c[0], 0, a[0]);
    a[0] = k (0, c[0], 0, a[0]);

you are using indirect function calls here, GCC in 4.6 is not smart enough
to transform them to direct calls before inlining.  Inlining of
always-inline indirect function calls is not going to work reliably.

Don't use always-inline or don't use indirect function calls to always-inline
functions.  It makes always-inline function calls survive until IPA inlining
where we seem to honor limits even though we say we should disregard them.

Honza?

Considering pd_combine_conjoint_xor with 383 size
 to be inlined into combine_conjoint_xor_ca_float in t.i:53
 Estimated growth after inlined into all callees is +748 insns.
 Estimated badness is -2147483648, frequency 1.00.
Processing frequency pd_combine_conjoint_xor
  Called by combine_conjoint_xor_ca_float that is normal or hot
 Inlined into combine_conjoint_xor_ca_float which now has time 1402 and size 2342,net change of +374.

Considering pd_combine_conjoint_xor with 383 size
 to be inlined into combine_conjoint_xor_ca_float in t.i:54
 Estimated growth after inlined into all callees is +374 insns.
 Estimated badness is -2147483648, frequency 1.00.
 Not inlining into combine_conjoint_xor_ca_float:--param large-function-growth limit reached.

(In reply to comment #2)
> void combine_conjoint_xor_ca_float ()
> {
>     combine_channel_t j = pd_combine_conjoint_xor, k = pd_combine_conjoint_xor;
>     a[0] = k (0, b, 0, a[0]);
>     a[0] = k (0, b, 0, a[0]);
>     a[0] = k (0, b, 0, a[0]);
>     a[0] = j (0, c[0], 0, a[0]);
>     a[0] = k (0, c[0], 0, a[0]);
>     a[0] = k (0, c[0], 0, a[0]);
>     a[0] = k (0, c[0], 0, a[0]);
> 
> you are using indirect function calls here, GCC in 4.6 is not smart enough
> to transform them to direct calls before inlining.  Inlining of
> always-inline indirect function calls is not going to work reliably.

Does this only apply to GCC 4.6?

> Don't use always-inline or don't use indirect function calls to always-inline
> functions.

This looks like it might be really inconvenient. Pixman relies on this functionality in a number of places by doing something like this:

void always_inline per_pixel_operation_a(...)
{
    ...
}

void always_inline per_pixel_operation_b(...)
{
    ...
}

void always_inline big_function_template(..., per_pixel_operation_ptr foo)
{
    ...
    /* do some calls to foo() in an inner loop */
    ...
}

void big_function_a(...)
{
    big_function_template(..., per_pixel_operation_a);
}

void big_function_b(...)
{
    big_function_template(..., per_pixel_operation_b);
}

Needless to say that we want to be absolutely sure that per-pixel operations are always inlined. Otherwise the performance gets really bad if the compiler ever makes a bad inlining decision.

The same functionality can be probably achieved by replacing always_inline functions with macros. But the code becomes less readable, more error prone and somewhat more difficult to maintain.

On Thu, 18 Oct 2012, siarhei.siamashka at gmail dot com wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54965
> 
> --- Comment #3 from Siarhei Siamashka <siarhei.siamashka at gmail dot com> 2012-10-18 10:47:51 UTC ---
> (In reply to comment #2)
> > void combine_conjoint_xor_ca_float ()
> > {
> >     combine_channel_t j = pd_combine_conjoint_xor, k = pd_combine_conjoint_xor;
> >     a[0] = k (0, b, 0, a[0]);
> >     a[0] = k (0, b, 0, a[0]);
> >     a[0] = k (0, b, 0, a[0]);
> >     a[0] = j (0, c[0], 0, a[0]);
> >     a[0] = k (0, c[0], 0, a[0]);
> >     a[0] = k (0, c[0], 0, a[0]);
> >     a[0] = k (0, c[0], 0, a[0]);
> > 
> > you are using indirect function calls here, GCC in 4.6 is not smart enough
> > to transform them to direct calls before inlining.  Inlining of
> > always-inline indirect function calls is not going to work reliably.
> 
> Does this only apply to GCC 4.6?

No, that applies in general.  If GCC isn't able to figure out which
function is called it cannot make sure always-inline functions are
always inlined.  always-inline is an attribute that should be used
if it is incorrect to not inline, not if that's just good for 
optimziation.

> > Don't use always-inline or don't use indirect function calls to always-inline
> > functions.
> 
> This looks like it might be really inconvenient. Pixman relies on this
> functionality in a number of places by doing something like this:
> 
> void always_inline per_pixel_operation_a(...)
> {
>     ...
> }
> 
> void always_inline per_pixel_operation_b(...)
> {
>     ...
> }
> 
> void always_inline big_function_template(..., per_pixel_operation_ptr foo)
> {
>     ...
>     /* do some calls to foo() in an inner loop */
>     ...
> }
> 
> void big_function_a(...)
> {
>     big_function_template(..., per_pixel_operation_a);
> }
> 
> void big_function_b(...)
> {
>     big_function_template(..., per_pixel_operation_b);
> }
> 
> Needless to say that we want to be absolutely sure that per-pixel operations
> are always inlined. Otherwise the performance gets really bad if the compiler
> ever makes a bad inlining decision.
> 
> The same functionality can be probably achieved by replacing always_inline
> functions with macros. But the code becomes less readable, more error prone and
> somewhat more difficult to maintain.

In the above case you probably want big_function_a to have all
calls inlined.  You can then conveniently use the flatten attribute:

void __attribute__((flatten)) big_function_b (...)
{
  big_function_template(..., per_pixel_operation_b);
}

GCC will then inline all calls in that function but not ICE
when it fails to inline one case for some weird reason.

Richard.

(In reply to comment #4)
> In the above case you probably want big_function_a to have all
> calls inlined.  You can then conveniently use the flatten attribute:
> 
> void __attribute__((flatten)) big_function_b (...)
> {
>   big_function_template(..., per_pixel_operation_b);
> }
> 
> GCC will then inline all calls in that function but not ICE
> when it fails to inline one case for some weird reason.

That's nice, but "flatten" attribute does not seem to be widely supported by the compilers. For example, clang-3.1 does not support it yet and the enhancement request is still open since 2010 - http://llvm.org/bugs/show_bug.cgi?id=7559

As far as I know, a few different compilers are currently in real use for building pixman for various systems: GCC, Clang, Solaris Studio and MSVC. All of them have some sort of "always_inline" attribute support, which makes it more universal than "flatten".

> Don't use always-inline or don't use indirect function calls to
> always-inline functions.  It makes always-inline function calls
> survive until IPA inlining where we seem to honor limits even
> though we say we should disregard them.

Is it too intrusive to fix GCC so that it would disregard limits in this case? Or maybe introduce one more attribute which would be a strong inlining hint, but still not cause compilation failure if some function can't be really inlined?

(In reply to comment #5)
> (In reply to comment #4)
> > In the above case you probably want big_function_a to have all
> > calls inlined.  You can then conveniently use the flatten attribute:
> > 
> > void __attribute__((flatten)) big_function_b (...)
> > {
> >   big_function_template(..., per_pixel_operation_b);
> > }
> > 
> > GCC will then inline all calls in that function but not ICE
> > when it fails to inline one case for some weird reason.
> 
> That's nice, but "flatten" attribute does not seem to be widely supported by
> the compilers. For example, clang-3.1 does not support it yet and the
> enhancement request is still open since 2010 -
> http://llvm.org/bugs/show_bug.cgi?id=7559
> 
> As far as I know, a few different compilers are currently in real use for
> building pixman for various systems: GCC, Clang, Solaris Studio and MSVC. All
> of them have some sort of "always_inline" attribute support, which makes it
> more universal than "flatten".
> 
> > Don't use always-inline or don't use indirect function calls to
> > always-inline functions.  It makes always-inline function calls
> > survive until IPA inlining where we seem to honor limits even
> > though we say we should disregard them.
> 
> Is it too intrusive to fix GCC so that it would disregard limits in this case?
> Or maybe introduce one more attribute which would be a strong inlining hint,
> but still not cause compilation failure if some function can't be really
> inlined?

I think the particular case is a bug in GCC, I was just mentioning that
using indirect function calls to always-inline functions is always prone
to this kind of error.

> you are using indirect function calls here, GCC in 4.6 is not smart enough
> to transform them to direct calls before inlining.  Inlining of
> always-inline indirect function calls is not going to work reliably.
> 
> Don't use always-inline or don't use indirect function calls to always-inline
> functions.  It makes always-inline function calls survive until IPA inlining
> where we seem to honor limits even though we say we should disregard them.

Yes, we do. It was actually your change to move always_inline function handling
out of the small functions inlining into inline_always_inline_functions.  The
motivation was that when done as part of small function inlining other inlining
may close cycles in the callgraph making always_inline inlining impossible.

In the presence of indirect calls, I do not think it is possible to honnor
always_inline completely because of the ordering issue.

Honza

The 4.6 branch has been closed, fixed in GCC 4.7.0.