[avr] Restore default value of PARAM_ALLOW_STORE_DATA_RACES to 1

Submitted by Senthil Kumar Selvaraj on Feb. 1, 2016, 1:56 p.m.

Details

Message ID 87egcwedqn.fsf@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj Feb. 1, 2016, 1:56 p.m.
Hi,

  This patch sets PARAM_ALLOW_STORE_DATA_RACES to 1 (the default until
  a year back), to avoid code size regressions in trunk (and probably
  5.x )for the AVR target.

  Consider the following piece of code
  
volatile int z;
void foo(int x)
{
    static char i;
    for (i=0; i< 4; ++i)
    {
        if (x > 2)
            z = 1;
        else
            z = 2;
    }
}

Unmodified gcc trunk generates this

        movw r20,r24
        sts i.1495,__zero_reg__
        ldi r25,0
        ldi r18,0
        ldi r22,lo8(2)
        ldi r23,0
        ldi r30,lo8(1)
        ldi r31,0
.L2:
        cpi r25,lo8(4)
        brne .L5
        cpse r18,__zero_reg__
        sts i.1495,r25
.L1:
        ret
.L5:
        cpi r20,3
        cpc r21,__zero_reg__
        brlt .L3
        sts z+1,r31
        sts z,r30
.L4:
        subi r25,lo8(-(1))
        ldi r18,lo8(1)
        rjmp .L2
.L3:
        sts z+1,r23
        sts z,r22
        rjmp .L4
        .size   foo, .-foo
        .local  i.1495
        .comm   i.1495,1,1
        .comm   z,2,1
        .ident  "GCC: (GNU) 6.0.0 20160201 (experimental)"

Note the usage of an extra reg (r18) that is used as a flag to
record loop entry (in .L4), and the conditional store of r25 to i in .L2.

In 4.x, there is no extra reg usage - only a single unconditional set of
i to r25 at the end of the loop.

Digging into the code, I found that LIM checks
PARAM_ALLOW_STORE_DATA_RACES and introduces the flag to avoid store data
races - see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52558. The
default value of the param was set to zero a year and a half back - see
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01548.html.

For AVR, I guess assuming any store can cause a data race is too
pessimistic for the general case. Globals shared with interrupts will
need special handling for atomic access anyway, so I thought we should
revert the default back to allow store data races.

If this is ok, could someone commit please? I don't have commit access.

Regards
Senthil

gcc/ChangeLog

2016-02-01  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	* config/avr/avr.c (avr_option_override): Set
    PARAM_ALLOW_STORE_DATA_RACES to 1.

Comments

Denis Chertykov Feb. 2, 2016, 4:02 p.m.
2016-02-01 16:56 GMT+03:00 Senthil Kumar Selvaraj
<senthil_kumar.selvaraj@atmel.com>:
>
> Hi,
>
>   This patch sets PARAM_ALLOW_STORE_DATA_RACES to 1 (the default until
>   a year back), to avoid code size regressions in trunk (and probably
>   5.x )for the AVR target.
>
>   Consider the following piece of code
>
> volatile int z;
> void foo(int x)
> {
>     static char i;
>     for (i=0; i< 4; ++i)
>     {
>         if (x > 2)
>             z = 1;
>         else
>             z = 2;
>     }
> }
>
> Unmodified gcc trunk generates this
>
>         movw r20,r24
>         sts i.1495,__zero_reg__
>         ldi r25,0
>         ldi r18,0
>         ldi r22,lo8(2)
>         ldi r23,0
>         ldi r30,lo8(1)
>         ldi r31,0
> .L2:
>         cpi r25,lo8(4)
>         brne .L5
>         cpse r18,__zero_reg__
>         sts i.1495,r25
> .L1:
>         ret
> .L5:
>         cpi r20,3
>         cpc r21,__zero_reg__
>         brlt .L3
>         sts z+1,r31
>         sts z,r30
> .L4:
>         subi r25,lo8(-(1))
>         ldi r18,lo8(1)
>         rjmp .L2
> .L3:
>         sts z+1,r23
>         sts z,r22
>         rjmp .L4
>         .size   foo, .-foo
>         .local  i.1495
>         .comm   i.1495,1,1
>         .comm   z,2,1
>         .ident  "GCC: (GNU) 6.0.0 20160201 (experimental)"
>
> Note the usage of an extra reg (r18) that is used as a flag to
> record loop entry (in .L4), and the conditional store of r25 to i in .L2.
>
> In 4.x, there is no extra reg usage - only a single unconditional set of
> i to r25 at the end of the loop.
>
> Digging into the code, I found that LIM checks
> PARAM_ALLOW_STORE_DATA_RACES and introduces the flag to avoid store data
> races - see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52558. The
> default value of the param was set to zero a year and a half back - see
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01548.html.
>
> For AVR, I guess assuming any store can cause a data race is too
> pessimistic for the general case. Globals shared with interrupts will
> need special handling for atomic access anyway, so I thought we should
> revert the default back to allow store data races.
>
> If this is ok, could someone commit please? I don't have commit access.
>
> Regards
> Senthil
>
> gcc/ChangeLog
>
> 2016-02-01  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
>         * config/avr/avr.c (avr_option_override): Set
>     PARAM_ALLOW_STORE_DATA_RACES to 1.
>

Committed.

Denis.

Patch hide | download patch | download mbox

diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index e557772..a7728e3 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -43,6 +43,7 @@ 
 #include "expr.h"
 #include "langhooks.h"
 #include "cfgrtl.h"
+#include "params.h"
 #include "builtins.h"
 #include "context.h"
 #include "tree-pass.h"
@@ -410,6 +411,15 @@  avr_option_override (void)
   if (avr_strict_X)
     flag_caller_saves = 0;
 
+  /* Allow optimizer to introduce store data races. This used to be the
+     default - it was changed because bigger targets did not see any
+     performance decrease. For the AVR though, disallowing data races
+     introduces additional code in LIM and increases reg pressure.  */
+
+  maybe_set_param_value (PARAM_ALLOW_STORE_DATA_RACES, 1,
+      global_options.x_param_values,
+      global_options_set.x_param_values);
+
   /* Unwind tables currently require a frame pointer for correctness,
      see toplev.c:process_options().  */