[testsuite] Fix bogus uninit-19.c failure for avr

Message ID 874m2y8qt5.fsf@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj Nov. 23, 2016, 9:54 a.m.
Hi,

  The below patch fixes uninit-19.c for avr by adding
  -finline-small-functions for avr.

  The test fails for avr because fn1 does not get inlined into fn2. Inlining
  occurs for x86_64 because fn1's computed size equals call_stmt_size. For
  avr, 32 bit memory moves are more expensive, and b[3] = p10[a] results in 
  a bigger size for fn1, resulting in estimate_growth > 0 and no inlining.

  Adding -finline-small-functions forces early inliner to inline fn1,
  making the test pass.

  Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-11-23  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

        * gcc.dg/uninit-19.c: Add -finline-small-functions for avr.

Comments

Jeff Law Nov. 23, 2016, 7:14 p.m. | #1
On 11/23/2016 02:54 AM, Senthil Kumar Selvaraj wrote:
> Hi,
>
>   The below patch fixes uninit-19.c for avr by adding
>   -finline-small-functions for avr.
>
>   The test fails for avr because fn1 does not get inlined into fn2. Inlining
>   occurs for x86_64 because fn1's computed size equals call_stmt_size. For
>   avr, 32 bit memory moves are more expensive, and b[3] = p10[a] results in
>   a bigger size for fn1, resulting in estimate_growth > 0 and no inlining.
>
>   Adding -finline-small-functions forces early inliner to inline fn1,
>   making the test pass.
>
>   Committed to trunk.
>
> Regards
> Senthil
>
> gcc/testsuite/ChangeLog
>
> 2016-11-23  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
>         * gcc.dg/uninit-19.c: Add -finline-small-functions for avr.
How about instead forcing inlining via the always_inline attribute?  If 
inlining is indeed expected and necessary, that seems better than 
-finline-small-functions.

Jeff
Senthil Kumar Selvaraj Nov. 24, 2016, 7:24 a.m. | #2
Jeff Law writes:

> On 11/23/2016 02:54 AM, Senthil Kumar Selvaraj wrote:
>> Hi,
>>
>>   The below patch fixes uninit-19.c for avr by adding
>>   -finline-small-functions for avr.
>>
>>   The test fails for avr because fn1 does not get inlined into fn2. Inlining
>>   occurs for x86_64 because fn1's computed size equals call_stmt_size. For
>>   avr, 32 bit memory moves are more expensive, and b[3] = p10[a] results in
>>   a bigger size for fn1, resulting in estimate_growth > 0 and no inlining.
>>
>>   Adding -finline-small-functions forces early inliner to inline fn1,
>>   making the test pass.
>>
>>   Committed to trunk.
>>
>> Regards
>> Senthil
>>
>> gcc/testsuite/ChangeLog
>>
>> 2016-11-23  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>>
>>         * gcc.dg/uninit-19.c: Add -finline-small-functions for avr.
> How about instead forcing inlining via the always_inline attribute?  If 
> inlining is indeed expected and necessary, that seems better than 
> -finline-small-functions.

I considered it, but a previous discussion in the mailing list backed
off from a similar proposal
(https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00517.html), and I wasn't
sure if always_inline would break the pic/non-pic distinction somehow.

Should I revert this and mark the function always_inline/static?

>
> Jeff
Jeff Law Nov. 28, 2016, 9:47 p.m. | #3
On 11/24/2016 12:24 AM, Senthil Kumar Selvaraj wrote:
>
> Jeff Law writes:
>
>> On 11/23/2016 02:54 AM, Senthil Kumar Selvaraj wrote:
>>> Hi,
>>>
>>>   The below patch fixes uninit-19.c for avr by adding
>>>   -finline-small-functions for avr.
>>>
>>>   The test fails for avr because fn1 does not get inlined into fn2. Inlining
>>>   occurs for x86_64 because fn1's computed size equals call_stmt_size. For
>>>   avr, 32 bit memory moves are more expensive, and b[3] = p10[a] results in
>>>   a bigger size for fn1, resulting in estimate_growth > 0 and no inlining.
>>>
>>>   Adding -finline-small-functions forces early inliner to inline fn1,
>>>   making the test pass.
>>>
>>>   Committed to trunk.
>>>
>>> Regards
>>> Senthil
>>>
>>> gcc/testsuite/ChangeLog
>>>
>>> 2016-11-23  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>>>
>>>         * gcc.dg/uninit-19.c: Add -finline-small-functions for avr.
>> How about instead forcing inlining via the always_inline attribute?  If
>> inlining is indeed expected and necessary, that seems better than
>> -finline-small-functions.
>
> I considered it, but a previous discussion in the mailing list backed
> off from a similar proposal
> (https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00517.html), and I wasn't
> sure if always_inline would break the pic/non-pic distinction somehow.
>
> Should I revert this and mark the function always_inline/static?
It's often not obvious what the right thing to do is.  I think it'll 
ultimately come down to trying to make an educated guess what the test 
is really trying to do and whether or not it it inherently depends on 
inlining.

Your call whether to go back and use the always_inline attribute on that 
particular test.

Jeff

Patch

Index: testsuite/gcc.dg/uninit-19.c
===================================================================
--- testsuite/gcc.dg/uninit-19.c	(revision 242741)
+++ testsuite/gcc.dg/uninit-19.c	(working copy)
@@ -1,5 +1,6 @@ 
 /* { dg-do compile } */
 /* { dg-options "-O -Wuninitialized" } */
+/* { dg-additional-options "-finline-small-functions" { target avr-*-* } } */
 
 int a, l, m;
 float *b;
@@ -10,7 +11,7 @@ 
      unsigned char *c2, float *p10)
 {
   if (p1 & 8)
-    b[3] = p10[a];  /* 13.  */
+    b[3] = p10[a];  /* 14.  */
 }
 
 void
@@ -19,8 +20,8 @@ 
   float *n;
   if (l & 6)
     n = &c + m;
-  fn1 (l, &d, &e, &g, &i, &h, &k, n);  /* 22.  */
+  fn1 (l, &d, &e, &g, &i, &h, &k, n);  /* 23.  */
 }
 
-/* { dg-warning "may be used uninitialized" "" { target { { nonpic } || { hppa*64*-*-* } } } 13 } */
-/* { dg-warning "may be used uninitialized" "" { target { ! { { nonpic } || { hppa*64*-*-* } } } } 22 } */
+/* { dg-warning "may be used uninitialized" "" { target { { nonpic } || { hppa*64*-*-* } } } 14 } */
+/* { dg-warning "may be used uninitialized" "" { target { ! { { nonpic } || { hppa*64*-*-* } } } } 23 } */