[avr-gcc-list] Re: gcc 4.3.3 vs inline

[Szikra Istvan]

Thanks David for the explanation. As you can see I already included a
workaround:
#define extIntSetEnable_gcc_4_3_3_aail_workaround
__attribute__((always_inline))
I use it with #if (GCC_VERSION == 40303UL) in my library :)
In this case it's clearly better to inline, but the function might be
called with variable parameters in other cases, so it would be nice
not to force the compiler...

My next question might prompt a RTFM answer in some ppl, but pls
resist the urge.
Anyone know if this new behavior can be disabled by command line switch?
I looked through the options that had something to do with inlining at
http://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html but did not
find how to get the old behavior without always inline attribute. But
maybe I just missed it.

Thanks Eric but I already know about
http://www.nongnu.org/avr-libc/bugs.html and also
https://savannah.nongnu.org/bugs/?group=avr-libc ,
http://gcc.gnu.org/bugzilla/ . But I thought it was indexed by google.
The entire point of me writing this mail was that I did not find this
reported. (which does not mean it's not, I might just not used the
right search phrase)

So I guess this should not be reported as a bug, because it would be
hard to fix so wouldn't happen anyway

Regards, Istvan

On Sun, Mar 28, 2010 at 12:15 AM, David Brown <address@hidden> wrote:
> Szikra Istvan wrote:
>>
>> /**
>> Category: miss optimization
>> affects: avr-gcc 4.3.3
>> Size optimization inline check generates bigger code
>>
>> I get this warning: "inlining failed in call to 'extIntSetEnable':
>> optimizing for size and code size would grow"
>> NO! It would not!
>>
>> Is this a known bug, or something that should be reported?
>> (tried google 'avr optimizing for size and code size would grow' with no
>> joy)
>>
>
> The algorithms for estimating code growth size on inlining are not perfect -
> it's a very hard problem to solve accurately while keeping a sensible
> compile time.  So much is done based on the complexity of the inline
> candidate by itself, not within the context of where it is used.  In cases
> like this where constant folding will reduce the inlined function to a few
> statements, you should use __attribute__((always_inline)) to force inlining.
>  Then you'll get the code you want.
>
> mvh.,
>
> David
>
>
>> See the test case for an example:
>> (sorry if it's still a bit much, but I tried to strip everything
>> that's not relevant.
>> I spent a lot of time on it, and it's a fraction of the original code.)
>> In this case NOT inlining generates more then 5 times as much
>> instructions than inlining!
>> */
>>
>> #define extIntSetEnable_gcc_4_3_3_aail_workaround
>> //#define extIntSetEnable_gcc_4_3_3_aail_workaround
>> __attribute__((always_inline))
>>
>> #include <avr/io.h>
>> #include <avr/interrupt.h>
>>
>> #define BIT(x) (0x01 << (x))
>> #define bit_set(p,b) ((p) |= BIT(b))
>> #define bit_clear(p,b) ((p) &= ~BIT(b))
>> //#define bit_write(p,b,c) (c ? bit_set(p,b) : bit_clear(p,b))
>> #define bit_write(p,b,c) if (c) { bit_set(p,b);}else bit_clear(p,b)
>>
>> static inline
>> void extIntSetEnable_gcc_4_3_3_aail_workaround extIntSetEnable(char
>> channel, char enabled)
>> {
>>  switch (channel) { /// enable / disable int
>>    case 0: {
>>      bit_write(GICR,INT0,enabled);
>>    } break;
>>    case 1: {
>>      bit_write(GICR,INT1,enabled);
>>    } break;
>>    case 2: {
>>      bit_write(GICR,INT2,enabled);
>>    } break;
>>  }
>> }
>> int main (void)
>> {
>>  extIntSetEnable(0,0);
>>  ///...
>>  extIntSetEnable(0,1);
>>  for (;;)
>>  {
>>  }
>> }
>>
>> /**
>> avr-gcc (GCC) 3.4.6
>> avr-gcc -c -mmcu=atmega16 -I. -gdwarf-2 -DF_CPU=16000000UL
>> -DBOOTSIZE=1024  -Os -funsigned-char -funsigned-bitfields
>> -fpack-struct -fshort-enums -fno-strict-aliasing -Wall -Winline
>> -Wstrict-prototypes -Wa,-adhlns=test_il.lst -ID:/Lib/ -std=gnu99 -MD
>> -MP -MF .dep/test_il.o.d test_il.c -o test_il.o
>> avr-gcc -mmcu=atmega16 -I. -gdwarf-2 -DF_CPU=16000000UL
>> -DBOOTSIZE=1024  -Os -funsigned-char -funsigned-bitfields
>> -fpack-struct -fshort-enums -fno-strict-aliasing -Wall -Winline
>> -Wstrict-prototypes -Wa,-adhlns=test_il.o -ID:/Lib/ -std=gnu99 -MD -MP
>> -MF .dep/test_il.elf.d test_il.o --output test_il.elf
>> -Wl,-Map=test_il.map,--cref    -lm
>> -Wl,--section-start=.bootloader=0x3800
>> Size after:
>> .text              184      0
>>
>> 00000054 <__ctors_end>:
>>  58:   cf e5           ldi     r28, 0x5F       ; 95
>>  5a:   d4 e0           ldi     r29, 0x04       ; 4
>>  5c:   de bf           out     0x3e, r29       ; 62
>>  5e:   cd bf           out     0x3d, r28       ; 61
>> ...
>> 0000008e <main>:
>>  8e:   cf e5           ldi     r28, 0x5F       ; 95  <<< again? ... gcc
>> 3.4.6 has
>> it's own faults
>>  90:   d4 e0           ldi     r29, 0x04       ; 4
>>  92:   de bf           out     0x3e, r29       ; 62
>>  94:   cd bf           out     0x3d, r28       ; 61
>>
>>  extIntSetEnable(0,0);
>>  96:   8b b7           in      r24, 0x3b       ; 59
>>  98:   8f 7b           andi    r24, 0xBF       ; 191
>>  9a:   8b bf           out     0x3b, r24       ; 59
>>  extIntSetEnable(0,1);
>>  9c:   8b b7           in      r24, 0x3b       ; 59
>>  9e:   80 64           ori     r24, 0x40       ; 64
>>  a0:   8b bf           out     0x3b, r24       ; 59
>>
>> */
>>
>> /**
>> avr-gcc (WinAVR 20090313) 4.3.2
>> avr-gcc -c -mmcu=atmega16 -I. -gdwarf-2 -DF_CPU=16000000UL
>> -DBOOTSIZE=1024  -Os -funsigned-char -funsigned-bitfields
>> -fpack-struct -fshort-enums -fno-strict-aliasing
>> -fno-inline-small-functions  -save-temps  -Wall -Winline
>> -Wstrict-prototypes -Wa,-adhlns=test_il.lst -ID:/Lib/ -std=gnu99 -MD
>> -MP -MF .dep/test_il.o.d test_il.c -o test_il.o
>> avr-gcc -mmcu=atmega16 -I. -gdwarf-2 -DF_CPU=16000000UL
>> -DBOOTSIZE=1024  -Os -funsigned-char -funsigned-bitfields
>> -fpack-struct -fshort-enums -fno-strict-aliasing
>> -fno-inline-small-functions  -save-temps  -Wall -Winline
>> -Wstrict-prototypes -Wa,-adhlns=test_il.o -ID:/Lib/ -std=gnu99 -MD -MP
>> -MF .dep/test_il.elf.d test_il.o --output test_il.elf
>> -Wl,-Map=test_il.map,--cref    -lm
>> -Wl,--section-start=.bootloader=0x3800
>> Size after:
>> .text              146      0
>>
>> 0000006c <main>:
>>      bit_write(GICR,INT0,enabled);
>>  6c:   8b b7           in      r24, 0x3b       ; 59
>>  6e:   8f 7b           andi    r24, 0xBF       ; 191
>>  70:   8b bf           out     0x3b, r24       ; 59
>>
>>  72:   8b b7           in      r24, 0x3b       ; 59
>>  74:   80 64           ori     r24, 0x40       ; 64
>>  76:   8b bf           out     0x3b, r24       ; 59
>>
>> */
>>
>> /**
>> avr-gcc (WinAVR 20100110) 4.3.3
>> avr-gcc -c -mmcu=atmega16 -I. -gdwarf-2 -DF_CPU=16000000UL
>> -DBOOTSIZE=1024  -Os -funsigned-char -funsigned-bitfields
>> -fpack-struct -fshort-enums -fno-strict-aliasing
>> -fno-inline-small-functions  -save-temps  -Wall -Winline
>> -Wstrict-prototypes -Wa,-adhlns=test_il.lst -ID:/Lib/ -std=gnu99 -MD
>> -MP -MF .dep/test_il.o.d test_il.c -o test_il.o
>> test_il.c: In function 'main':
>> test_il.c:32: warning: inlining failed in call to 'extIntSetEnable':
>> optimizing for size and code size would grow
>> test_il.c:49: warning: called from here
>> test_il.c:32: warning: inlining failed in call to 'extIntSetEnable':
>> optimizing for size and code size would grow
>> test_il.c:54: warning: called from here
>> avr-gcc -mmcu=atmega16 -I. -gdwarf-2 -DF_CPU=16000000UL
>> -DBOOTSIZE=1024  -Os -funsigned-char -funsigned-bitfields
>> -fpack-struct -fshort-enums -fno-strict-aliasing
>> -fno-inline-small-functions  -save-temps  -Wall -Winline
>> -Wstrict-prototypes -Wa,-adhlns=test_il.o -ID:/Lib/ -std=gnu99 -MD -MP
>> -MF .dep/test_il.elf.d test_il.o --output test_il.elf
>> -Wl,-Map=test_il.map,--cref    -lm
>> -Wl,--section-start=.bootloader=0x3800
>> Size after:
>> .text              216      0
>>
>>
>> 0000006c <extIntSetEnable>:
>>    switch (channel) { /// enable / disable int
>>  6c:   81 30           cpi     r24, 0x01       ; 1
>>  6e:   69 f0           breq    .+26            ; 0x8a
>> <extIntSetEnable+0x1e>
>>  70:   81 30           cpi     r24, 0x01       ; 1
>>  72:   18 f0           brcs    .+6             ; 0x7a
>> <extIntSetEnable+0xe>
>>  74:   82 30           cpi     r24, 0x02       ; 2
>>  76:   e1 f4           brne    .+56            ; 0xb0
>> <extIntSetEnable+0x44>
>>  78:   10 c0           rjmp    .+32            ; 0x9a
>> <extIntSetEnable+0x2e>
>>    case 0: {
>>      bit_write(GICR,INT0,enabled);
>>  7a:   66 23           and     r22, r22
>>  7c:   19 f0           breq    .+6             ; 0x84
>> <extIntSetEnable+0x18>
>>  7e:   8b b7           in      r24, 0x3b       ; 59
>>  80:   80 64           ori     r24, 0x40       ; 64
>>  82:   0f c0           rjmp    .+30            ; 0xa2
>> <extIntSetEnable+0x36>
>>  84:   8b b7           in      r24, 0x3b       ; 59
>>  86:   8f 7b           andi    r24, 0xBF       ; 191
>>  88:   0c c0           rjmp    .+24            ; 0xa2
>> <extIntSetEnable+0x36>
>>    } break;
>>    case 1: {
>>      bit_write(GICR,INT1,enabled);
>>  8a:   66 23           and     r22, r22
>>  8c:   19 f0           breq    .+6             ; 0x94
>> <extIntSetEnable+0x28>
>>  8e:   8b b7           in      r24, 0x3b       ; 59
>>  90:   80 68           ori     r24, 0x80       ; 128
>>  92:   07 c0           rjmp    .+14            ; 0xa2
>> <extIntSetEnable+0x36>
>>  94:   8b b7           in      r24, 0x3b       ; 59
>>  96:   8f 77           andi    r24, 0x7F       ; 127
>>  98:   04 c0           rjmp    .+8             ; 0xa2
>> <extIntSetEnable+0x36>
>>    } break;
>>    case 2: {
>>      bit_write(GICR,INT2,enabled);
>>  9a:   66 23           and     r22, r22
>>  9c:   21 f0           breq    .+8             ; 0xa6
>> <extIntSetEnable+0x3a>
>>  9e:   8b b7           in      r24, 0x3b       ; 59
>>  a0:   80 62           ori     r24, 0x20       ; 32
>>  a2:   8b bf           out     0x3b, r24       ; 59
>>  a4:   08 95           ret
>>  a6:   8b b7           in      r24, 0x3b       ; 59
>>  a8:   8f 7d           andi    r24, 0xDF       ; 223
>>  aa:   8b bf           out     0x3b, r24       ; 59
>>  ac:   08 95           ret
>>
>> 000000ae <main>:
>>    extIntSetEnable(0,0);
>>  ae:   80 e0           ldi     r24, 0x00       ; 0
>>  b0:   60 e0           ldi     r22, 0x00       ; 0
>>  b2:   0e 94 36 00     call    0x6c    ; 0x6c <extIntSetEnable>
>>    extIntSetEnable(0,1);
>>  b6:   80 e0           ldi     r24, 0x00       ; 0
>>  b8:   61 e0           ldi     r22, 0x01       ; 1
>>  ba:   0e 94 36 00     call    0x6c    ; 0x6c <extIntSetEnable>
>>
>> */
>>
>> SIGNAL(SIG_INTERRUPT0)
>> {
>> }
>>