This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[Patch, avr] Restore default value of PARAM_ALLOW_STORE_DATA_RACES to 1
- From: Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>
- To: GCC-Patches-ML <gcc-patches at gcc dot gnu dot org>
- Cc: Georg-Johann Lay <avr at gjlay dot de>, Denis Chertykov <chertykov at gmail dot com>
- Date: Mon, 1 Feb 2016 19:26:00 +0530
- Subject: [Patch, avr] Restore default value of PARAM_ALLOW_STORE_DATA_RACES to 1
- Authentication-results: sourceware.org; auth=none
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.
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(). */