[patch,avr] PR70677: Use -fno-caller-saves for avr

[patch,avr] PR70677: Use -fno-caller-saves for avr

[Georg-Johann Lay]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

Problem with -fcaller-saves is that there are situations where it triggers an 
expensive frame just to store a variable around a function call even though 
there are plenty of call-saved registers.

Example:

typedef __UINT8_TYPE__ uint8_t;

extern uint8_t uart0_getc (void);

void foo (uint8_t *buffer, uint8_t cnt)
{
   while (--cnt)
     {
       *buffer++ = uart0_getc();
     }
}

$ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c

$ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c && avr-size 
loop-buf.o
    text    data     bss     dec     hex filename
      50       0       0      50      32 loop-buf.o

$ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves && 
avr-size loop-buf.o
    text    data     bss     dec     hex filename
      32       0       0      32      20 loop-buf.o

I actually came never across a situation where -fcaller-saves improved the code 
performance, hence this patch proposes to switch off -fcaller-saved per default.

I can test the patch without regressions, but what bothers me is the following 
lines in ira-color.c:allocno_reload_assign()

       if (ALLOCNO_CALLS_CROSSED_NUM (a) != 0
	  && ira_hard_reg_set_intersection_p (hard_regno, ALLOCNO_MODE (a),
					      call_used_reg_set))
	{
	  ira_assert (flag_caller_saves);
	  caller_save_needed = 1;
	}

What's not clear is whether this assertion is about the inner working of IRA as 
alloc depends on caller-saves in other places of IRA, or if caller-saves is 
needed because otherwise IRA cannot resolve complicated reload situations and 
hence the proposed change might trigger ICEs for complex programs.

Therefore CCed Vladimir who added the assertion to IRA.

Ok to apply if IRA can do without caller-saves?


Johann


	PR 70677
	* common/config/avr/avr-common.c (avr_option_optimization_table)
	[OPT_LEVELS_ALL]: Turn off -fcaller-saves.



[-- Attachment #2: no-caller-saves.diff --]
[-- Type: text/x-patch, Size: 600 bytes --]

Index: common/config/avr/avr-common.c
===================================================================
--- common/config/avr/avr-common.c	(revision 238849)
+++ common/config/avr/avr-common.c	(working copy)
@@ -28,6 +28,9 @@
 static const struct default_options avr_option_optimization_table[] =
   {
     { OPT_LEVELS_1_PLUS, OPT_fomit_frame_pointer, NULL, 1 },
+    // The only effect of -fcaller-saves might be that it triggers
+    // a frame without need when it tries to be smart around calls.
+    { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 },
     { OPT_LEVELS_NONE, 0, NULL, 0 }
   };
 

Re: [patch,avr] PR70677: Use -fno-caller-saves for avr

[Denis Chertykov]

2016-08-01 15:17 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
> Problem with -fcaller-saves is that there are situations where it triggers
> an expensive frame just to store a variable around a function call even
> though there are plenty of call-saved registers.
>
> Example:
>
> typedef __UINT8_TYPE__ uint8_t;
>
> extern uint8_t uart0_getc (void);
>
> void foo (uint8_t *buffer, uint8_t cnt)
> {
>   while (--cnt)
>     {
>       *buffer++ = uart0_getc();
>     }
> }
>
> $ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c
>
> $ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c &&
> avr-size loop-buf.o
>    text    data     bss     dec     hex filename
>      50       0       0      50      32 loop-buf.o
>
> $ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves
> && avr-size loop-buf.o
>    text    data     bss     dec     hex filename
>      32       0       0      32      20 loop-buf.o
>
> I actually came never across a situation where -fcaller-saves improved the
> code performance, hence this patch proposes to switch off -fcaller-saved per
> default.
>
> I can test the patch without regressions, but what bothers me is the
> following lines in ira-color.c:allocno_reload_assign()
>
>       if (ALLOCNO_CALLS_CROSSED_NUM (a) != 0
>           && ira_hard_reg_set_intersection_p (hard_regno, ALLOCNO_MODE (a),
>                                               call_used_reg_set))
>         {
>           ira_assert (flag_caller_saves);
>           caller_save_needed = 1;
>         }
>
> What's not clear is whether this assertion is about the inner working of IRA
> as alloc depends on caller-saves in other places of IRA, or if caller-saves
> is needed because otherwise IRA cannot resolve complicated reload situations
> and hence the proposed change might trigger ICEs for complex programs.
>
> Therefore CCed Vladimir who added the assertion to IRA.
>
> Ok to apply if IRA can do without caller-saves?


Ok.



>
> Johann
>
>
>         PR 70677
>         * common/config/avr/avr-common.c (avr_option_optimization_table)
>         [OPT_LEVELS_ALL]: Turn off -fcaller-saves.
>
>

Re: [patch,avr] PR70677: Use -fno-caller-saves for avr

[Senthil Kumar Selvaraj]

Denis Chertykov writes:

> 2016-08-01 15:17 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
>> Problem with -fcaller-saves is that there are situations where it triggers
>> an expensive frame just to store a variable around a function call even
>> though there are plenty of call-saved registers.
>>
>> Example:
>>
>> typedef __UINT8_TYPE__ uint8_t;
>>
>> extern uint8_t uart0_getc (void);
>>
>> void foo (uint8_t *buffer, uint8_t cnt)
>> {
>>   while (--cnt)
>>     {
>>       *buffer++ = uart0_getc();
>>     }
>> }
>>
>> $ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c
>>
>> $ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c &&
>> avr-size loop-buf.o
>>    text    data     bss     dec     hex filename
>>      50       0       0      50      32 loop-buf.o
>>
>> $ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves
>> && avr-size loop-buf.o
>>    text    data     bss     dec     hex filename
>>      32       0       0      32      20 loop-buf.o
>>
>> I actually came never across a situation where -fcaller-saves improved the
>> code performance, hence this patch proposes to switch off -fcaller-saved per
>> default.

Like you mentioned in the bug report, would fixing the costs be a better
way to fix this rather than a blanket disabling of the option?

Regards
Senthil

Re: [patch,avr] PR70677: Use -fno-caller-saves for avr

[Georg-Johann Lay]

On 02.08.2016 06:50, Senthil Kumar Selvaraj wrote:
>
> Denis Chertykov writes:
>
>> 2016-08-01 15:17 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
>>> Problem with -fcaller-saves is that there are situations where it triggers
>>> an expensive frame just to store a variable around a function call even
>>> though there are plenty of call-saved registers.
>>>
>>> Example:
>>>
>>> typedef __UINT8_TYPE__ uint8_t;
>>>
>>> extern uint8_t uart0_getc (void);
>>>
>>> void foo (uint8_t *buffer, uint8_t cnt)
>>> {
>>>   while (--cnt)
>>>     {
>>>       *buffer++ = uart0_getc();
>>>     }
>>> }
>>>
>>> $ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c
>>>
>>> $ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c &&
>>> avr-size loop-buf.o
>>>    text    data     bss     dec     hex filename
>>>      50       0       0      50      32 loop-buf.o
>>>
>>> $ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves
>>> && avr-size loop-buf.o
>>>    text    data     bss     dec     hex filename
>>>      32       0       0      32      20 loop-buf.o
>>>
>>> I actually came never across a situation where -fcaller-saves improved the
>>> code performance, hence this patch proposes to switch off -fcaller-saved per
>>> default.
>
> Like you mentioned in the bug report, would fixing the costs be a better
> way to fix this rather than a blanket disabling of the option?

What costs specifically?  Where could the costs of different epilogues / 
prologues be described?

Johann

>
> Regards
> Senthil
>

Re: [patch,avr] PR70677: Use -fno-caller-saves for avr

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> On 02.08.2016 06:50, Senthil Kumar Selvaraj wrote:
>>
>> Denis Chertykov writes:
>>
>>> 2016-08-01 15:17 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
>>>> Problem with -fcaller-saves is that there are situations where it triggers
>>>> an expensive frame just to store a variable around a function call even
>>>> though there are plenty of call-saved registers.
>>>>
>>>> Example:
>>>>
>>>> typedef __UINT8_TYPE__ uint8_t;
>>>>
>>>> extern uint8_t uart0_getc (void);
>>>>
>>>> void foo (uint8_t *buffer, uint8_t cnt)
>>>> {
>>>>   while (--cnt)
>>>>     {
>>>>       *buffer++ = uart0_getc();
>>>>     }
>>>> }
>>>>
>>>> $ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c
>>>>
>>>> $ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c &&
>>>> avr-size loop-buf.o
>>>>    text    data     bss     dec     hex filename
>>>>      50       0       0      50      32 loop-buf.o
>>>>
>>>> $ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves
>>>> && avr-size loop-buf.o
>>>>    text    data     bss     dec     hex filename
>>>>      32       0       0      32      20 loop-buf.o
>>>>
>>>> I actually came never across a situation where -fcaller-saves improved the
>>>> code performance, hence this patch proposes to switch off -fcaller-saved per
>>>> default.
>>
>> Like you mentioned in the bug report, would fixing the costs be a better
>> way to fix this rather than a blanket disabling of the option?
>
> What costs specifically?  Where could the costs of different epilogues / 
> prologues be described?

I don't know either - I'd have to dig in to find that out too. Let me
check that out.

Regards
Senthil