[PATCH] Change default for --param allow-...-data-races to off

[PATCH] Change default for --param allow-...-data-races to off

[Bernd Edlinger]

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

Hi,

from a recent discussion on gcc@gcc.gnu.org I have learned that the default of
--param allow-store-data-races is still 1, and it is causing problems.
Therefore I would like to suggest to change the default of this option to 0.

Boot-strapped and regression tested on x86_64-linux-gnu.
Ok for trunk?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: changelog-allow-races.txt --]
[-- Type: text/plain, Size: 1239 bytes --]

gcc/ChangeLog:
2014-06-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Set default for --param allow-...-data-races to off.
	* params.def (PARAM_ALLOW_LOAD_DATA_RACES,
	PARAM_ALLOW_STORE_DATA_RACES, PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
	PARAM_ALLOW_PACKED_STORE_DATA_RACES): Set default to off.

testsuite/ChangeLog:
2014-06-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Adjust to new default for --param allow-...-data-races.
	* c-c++-common/cxxbitfields-3.c: Adjust.
	* c-c++-common/cxxbitfields-6.c: Adjust.
	* c-c++-common/simulate-thread/bitfields-1.c: Adjust.
	* c-c++-common/simulate-thread/bitfields-2.c: Adjust.
	* c-c++-common/simulate-thread/bitfields-3.c: Adjust.
	* c-c++-common/simulate-thread/bitfields-4.c: Adjust.
	* g++.dg/simulate-thread/bitfields.C: Adjust.
	* g++.dg/simulate-thread/bitfields-2.C: Adjust.
	* gcc.dg/lto/pr52097_0.c: Adjust.
	* gcc.dg/simulate-thread/speculative-store.c: Adjust.
	* gcc.dg/simulate-thread/speculative-store-2.c: Adjust.
	* gcc.dg/simulate-thread/speculative-store-3.c: Adjust.
	* gcc.dg/simulate-thread/speculative-store-4.c: Adjust.
	* gcc.dg/simulate-thread/strict-align-global.c: Adjust.
	* gcc.dg/simulate-thread/subfields.c: Adjust.
	* gcc.dg/tree-ssa/20050314-1.c: Adjust.


[-- Attachment #3: patch-allow-races.diff --]
[-- Type: application/octet-stream, Size: 9333 bytes --]

Index: gcc/params.def
===================================================================
--- gcc/params.def	(revision 211799)
+++ gcc/params.def	(working copy)
@@ -1005,22 +1005,22 @@
 DEFPARAM (PARAM_ALLOW_LOAD_DATA_RACES,
 	  "allow-load-data-races",
 	  "Allow new data races on loads to be introduced",
-	  1, 0, 1)
+	  0, 0, 1)
 
 DEFPARAM (PARAM_ALLOW_STORE_DATA_RACES,
 	  "allow-store-data-races",
 	  "Allow new data races on stores to be introduced",
-	  1, 0, 1)
+	  0, 0, 1)
 
 DEFPARAM (PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
 	  "allow-packed-load-data-races",
 	  "Allow new data races on packed data loads to be introduced",
-	  1, 0, 1)
+	  0, 0, 1)
 
 DEFPARAM (PARAM_ALLOW_PACKED_STORE_DATA_RACES,
 	  "allow-packed-store-data-races",
 	  "Allow new data races on packed data stores to be introduced",
-	  1, 0, 1)
+	  0, 0, 1)
 
 /* Reassociation width to be used by tree reassoc optimization.  */
 DEFPARAM (PARAM_TREE_REASSOC_WIDTH,
Index: gcc/testsuite/c-c++-common/cxxbitfields-3.c
===================================================================
--- gcc/testsuite/c-c++-common/cxxbitfields-3.c	(revision 211799)
+++ gcc/testsuite/c-c++-common/cxxbitfields-3.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile { target i?86-*-* x86_64-*-* } } */
-/* { dg-options "-O2 --param allow-store-data-races=0" } */
+/* { dg-options "-O2" } */
 
 /* Make sure we don't narrow down to a QI or HI to store into VAR.J,
    but instead use an SI.  */
Index: gcc/testsuite/c-c++-common/cxxbitfields-6.c
===================================================================
--- gcc/testsuite/c-c++-common/cxxbitfields-6.c	(revision 211799)
+++ gcc/testsuite/c-c++-common/cxxbitfields-6.c	(working copy)
@@ -1,6 +1,6 @@
 /* PR middle-end/50141 */
 /* { dg-do compile } */
-/* { dg-options "-O2 --param allow-store-data-races=0" } */
+/* { dg-options "-O2" } */
 
 struct S
 {
Index: gcc/testsuite/c-c++-common/simulate-thread/bitfields-1.c
===================================================================
--- gcc/testsuite/c-c++-common/simulate-thread/bitfields-1.c	(revision 211799)
+++ gcc/testsuite/c-c++-common/simulate-thread/bitfields-1.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/c-c++-common/simulate-thread/bitfields-2.c
===================================================================
--- gcc/testsuite/c-c++-common/simulate-thread/bitfields-2.c	(revision 211799)
+++ gcc/testsuite/c-c++-common/simulate-thread/bitfields-2.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link { target { ! int16 } } } */
-/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/c-c++-common/simulate-thread/bitfields-3.c
===================================================================
--- gcc/testsuite/c-c++-common/simulate-thread/bitfields-3.c	(revision 211799)
+++ gcc/testsuite/c-c++-common/simulate-thread/bitfields-3.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/c-c++-common/simulate-thread/bitfields-4.c
===================================================================
--- gcc/testsuite/c-c++-common/simulate-thread/bitfields-4.c	(revision 211799)
+++ gcc/testsuite/c-c++-common/simulate-thread/bitfields-4.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/g++.dg/simulate-thread/bitfields.C
===================================================================
--- gcc/testsuite/g++.dg/simulate-thread/bitfields.C	(revision 211799)
+++ gcc/testsuite/g++.dg/simulate-thread/bitfields.C	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-load-data-races=0 --param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 /* Test that setting <var.a> does not touch either <var.b> or <var.c>.
Index: gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
===================================================================
--- gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C	(revision 211799)
+++ gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-load-data-races=0 --param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 /* Test that setting <var.a> does not touch either <var.b> or <var.c>.
Index: gcc/testsuite/gcc.dg/lto/pr52097_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr52097_0.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/lto/pr52097_0.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-lto-do link } */
-/* { dg-lto-options { { -O -flto -fexceptions -fnon-call-exceptions --param allow-store-data-races=0 } } } */
+/* { dg-lto-options { { -O -flto -fexceptions -fnon-call-exceptions } } } */
 
 typedef struct { unsigned int e0 : 16; } s1;
 typedef struct { unsigned int e0 : 16; } s2;
Index: gcc/testsuite/gcc.dg/simulate-thread/speculative-store.c
===================================================================
--- gcc/testsuite/gcc.dg/simulate-thread/speculative-store.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/simulate-thread/speculative-store.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
@@ -6,7 +5,7 @@
 #include "simulate-thread.h"
 
 /* This file tests that speculative store movement out of a loop doesn't 
-   happen.  This is disallowed when --param allow-store-data-races is 0.  */
+   happen.  */
 
 int global = 100;
 
Index: gcc/testsuite/gcc.dg/simulate-thread/speculative-store-2.c
===================================================================
--- gcc/testsuite/gcc.dg/simulate-thread/speculative-store-2.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/simulate-thread/speculative-store-2.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0 -O2" } */
+/* { dg-options "-O2" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
@@ -7,8 +7,7 @@
 
 #include "simulate-thread.h"
 
-/* Test that speculative stores do not happen for --param
-   allow-store-data-races=0.  */
+/* Test that speculative stores do not happen.  */
 
 int count, insns;
 
Index: gcc/testsuite/gcc.dg/simulate-thread/speculative-store-3.c
===================================================================
--- gcc/testsuite/gcc.dg/simulate-thread/speculative-store-3.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/simulate-thread/speculative-store-3.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0 -O2" } */
+/* { dg-options "-O2" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/gcc.dg/simulate-thread/speculative-store-4.c
===================================================================
--- gcc/testsuite/gcc.dg/simulate-thread/speculative-store-4.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/simulate-thread/speculative-store-4.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
@@ -8,8 +7,7 @@
 #include "simulate-thread.h"
 
 /* PR 54139 */
-/* Test that speculative stores do not happen for --param
-   allow-store-data-races=0.  */
+/* Test that speculative stores do not happen.  */
 
 int g_13=1, insns=1;
 
Index: gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
===================================================================
--- gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-packed-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/gcc.dg/simulate-thread/subfields.c
===================================================================
--- gcc/testsuite/gcc.dg/simulate-thread/subfields.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/simulate-thread/subfields.c	(working copy)
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-packed-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
Index: gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c	(revision 211799)
+++ gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c	(working copy)
@@ -15,7 +15,7 @@
 }
 
 /* Store motion may be applied to the assignment to a[k], since sinf
-   cannot read nor write the memory.  */
+   cannot read nor write the memory.  Add one for the store flag.  */
 
-/* { dg-final { scan-tree-dump-times "Moving statement" 1 "lim1" } } */
+/* { dg-final { scan-tree-dump-times "Moving statement" 2 "lim1" } } */
 /* { dg-final { cleanup-tree-dump "lim1" } } */

Re: [PATCH] Change default for --param allow-...-data-races to off

[Martin Jambor]

Hi,

On Thu, Jun 19, 2014 at 06:18:47PM +0200, Bernd Edlinger wrote:
> Hi,
> 
> from a recent discussion on gcc@gcc.gnu.org I have learned that the default of
> --param allow-store-data-races is still 1, and it is causing problems.
> Therefore I would like to suggest to change the default of this option to 0.

I was about to propose a similar patch but I intended to leave the
parameter set to one when -Ofast is specified so that benchmarks are
not hurt by this and as a nice pointer for people exploring our
options to really squeeze out 100% performance (which would of course
mean documenting it too).

Thanks,

Martin

> 
> Boot-strapped and regression tested on x86_64-linux-gnu.
> Ok for trunk?
> 
> 
> Thanks
> Bernd.
>  		 	   		  

> gcc/ChangeLog:
> 2014-06-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	Set default for --param allow-...-data-races to off.
> 	* params.def (PARAM_ALLOW_LOAD_DATA_RACES,
> 	PARAM_ALLOW_STORE_DATA_RACES, PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
> 	PARAM_ALLOW_PACKED_STORE_DATA_RACES): Set default to off.
> 
> testsuite/ChangeLog:
> 2014-06-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	Adjust to new default for --param allow-...-data-races.
> 	* c-c++-common/cxxbitfields-3.c: Adjust.
> 	* c-c++-common/cxxbitfields-6.c: Adjust.
> 	* c-c++-common/simulate-thread/bitfields-1.c: Adjust.
> 	* c-c++-common/simulate-thread/bitfields-2.c: Adjust.
> 	* c-c++-common/simulate-thread/bitfields-3.c: Adjust.
> 	* c-c++-common/simulate-thread/bitfields-4.c: Adjust.
> 	* g++.dg/simulate-thread/bitfields.C: Adjust.
> 	* g++.dg/simulate-thread/bitfields-2.C: Adjust.
> 	* gcc.dg/lto/pr52097_0.c: Adjust.
> 	* gcc.dg/simulate-thread/speculative-store.c: Adjust.
> 	* gcc.dg/simulate-thread/speculative-store-2.c: Adjust.
> 	* gcc.dg/simulate-thread/speculative-store-3.c: Adjust.
> 	* gcc.dg/simulate-thread/speculative-store-4.c: Adjust.
> 	* gcc.dg/simulate-thread/strict-align-global.c: Adjust.
> 	* gcc.dg/simulate-thread/subfields.c: Adjust.
> 	* gcc.dg/tree-ssa/20050314-1.c: Adjust.
> 

RE: [PATCH] Change default for --param allow-...-data-races to off

[Bernd Edlinger]

Hi,


On Fri, 20 Jun 2014 13:44:18, Martin Jambor wrote:
>
> Hi,
>
> On Thu, Jun 19, 2014 at 06:18:47PM +0200, Bernd Edlinger wrote:
>> Hi,
>>
>> from a recent discussion on gcc@gcc.gnu.org I have learned that the default of
>> --param allow-store-data-races is still 1, and it is causing problems.
>> Therefore I would like to suggest to change the default of this option to 0.
>
> I was about to propose a similar patch but I intended to leave the
> parameter set to one when -Ofast is specified so that benchmarks are
> not hurt by this and as a nice pointer for people exploring our
> options to really squeeze out 100% performance (which would of course
> mean documenting it too).
>

Well actually, I am not sure if we ever wanted to have a race condition here.
Have you seen any impact of --param allow-store-data-races on any benchmark?


Thanks
Bernd.

> Thanks,
>
> Martin
>
>>
>> Boot-strapped and regression tested on x86_64-linux-gnu.
>> Ok for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>
>> gcc/ChangeLog:
>> 2014-06-19 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> Set default for --param allow-...-data-races to off.
>> * params.def (PARAM_ALLOW_LOAD_DATA_RACES,
>> PARAM_ALLOW_STORE_DATA_RACES, PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
>> PARAM_ALLOW_PACKED_STORE_DATA_RACES): Set default to off.
>>
>> testsuite/ChangeLog:
>> 2014-06-19 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> Adjust to new default for --param allow-...-data-races.
>> * c-c++-common/cxxbitfields-3.c: Adjust.
>> * c-c++-common/cxxbitfields-6.c: Adjust.
>> * c-c++-common/simulate-thread/bitfields-1.c: Adjust.
>> * c-c++-common/simulate-thread/bitfields-2.c: Adjust.
>> * c-c++-common/simulate-thread/bitfields-3.c: Adjust.
>> * c-c++-common/simulate-thread/bitfields-4.c: Adjust.
>> * g++.dg/simulate-thread/bitfields.C: Adjust.
>> * g++.dg/simulate-thread/bitfields-2.C: Adjust.
>> * gcc.dg/lto/pr52097_0.c: Adjust.
>> * gcc.dg/simulate-thread/speculative-store.c: Adjust.
>> * gcc.dg/simulate-thread/speculative-store-2.c: Adjust.
>> * gcc.dg/simulate-thread/speculative-store-3.c: Adjust.
>> * gcc.dg/simulate-thread/speculative-store-4.c: Adjust.
>> * gcc.dg/simulate-thread/strict-align-global.c: Adjust.
>> * gcc.dg/simulate-thread/subfields.c: Adjust.
>> * gcc.dg/tree-ssa/20050314-1.c: Adjust.
>>
>
>
 		 	   		  

Re: [PATCH] Change default for --param allow-...-data-races to off

[Richard Biener]

On Mon, Jun 23, 2014 at 10:02 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
>
> On Fri, 20 Jun 2014 13:44:18, Martin Jambor wrote:
>>
>> Hi,
>>
>> On Thu, Jun 19, 2014 at 06:18:47PM +0200, Bernd Edlinger wrote:
>>> Hi,
>>>
>>> from a recent discussion on gcc@gcc.gnu.org I have learned that the default of
>>> --param allow-store-data-races is still 1, and it is causing problems.
>>> Therefore I would like to suggest to change the default of this option to 0.
>>
>> I was about to propose a similar patch but I intended to leave the
>> parameter set to one when -Ofast is specified so that benchmarks are
>> not hurt by this and as a nice pointer for people exploring our
>> options to really squeeze out 100% performance (which would of course
>> mean documenting it too).
>>
>
> Well actually, I am not sure if we ever wanted to have a race condition here.
> Have you seen any impact of --param allow-store-data-races on any benchmark?

It's trivially to write one.   The only pass that checks the param is
tree loop invariant motion and it does that when it applies store-motion.
Register pressure increase is increased by a factor of two.

So I'd agree that we might want to disable this again for -Ofast.

As nothing tests for the PACKED variants nor for the LOAD variant
I'd rather remove those.  Claiming we don't create races for those
when you disable it via the param is simply not true.

Thanks,
Richard.

>
> Thanks
> Bernd.
>
>> Thanks,
>>
>> Martin
>>
>>>
>>> Boot-strapped and regression tested on x86_64-linux-gnu.
>>> Ok for trunk?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>
>>> gcc/ChangeLog:
>>> 2014-06-19 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>
>>> Set default for --param allow-...-data-races to off.
>>> * params.def (PARAM_ALLOW_LOAD_DATA_RACES,
>>> PARAM_ALLOW_STORE_DATA_RACES, PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
>>> PARAM_ALLOW_PACKED_STORE_DATA_RACES): Set default to off.
>>>
>>> testsuite/ChangeLog:
>>> 2014-06-19 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>
>>> Adjust to new default for --param allow-...-data-races.
>>> * c-c++-common/cxxbitfields-3.c: Adjust.
>>> * c-c++-common/cxxbitfields-6.c: Adjust.
>>> * c-c++-common/simulate-thread/bitfields-1.c: Adjust.
>>> * c-c++-common/simulate-thread/bitfields-2.c: Adjust.
>>> * c-c++-common/simulate-thread/bitfields-3.c: Adjust.
>>> * c-c++-common/simulate-thread/bitfields-4.c: Adjust.
>>> * g++.dg/simulate-thread/bitfields.C: Adjust.
>>> * g++.dg/simulate-thread/bitfields-2.C: Adjust.
>>> * gcc.dg/lto/pr52097_0.c: Adjust.
>>> * gcc.dg/simulate-thread/speculative-store.c: Adjust.
>>> * gcc.dg/simulate-thread/speculative-store-2.c: Adjust.
>>> * gcc.dg/simulate-thread/speculative-store-3.c: Adjust.
>>> * gcc.dg/simulate-thread/speculative-store-4.c: Adjust.
>>> * gcc.dg/simulate-thread/strict-align-global.c: Adjust.
>>> * gcc.dg/simulate-thread/subfields.c: Adjust.
>>> * gcc.dg/tree-ssa/20050314-1.c: Adjust.
>>>
>>
>>
>

RE: [PATCH] Change default for --param allow-...-data-races to off

[Bernd Edlinger]

Hi Martin,

>>
>> Well actually, I am not sure if we ever wanted to have a race condition here.
>> Have you seen any impact of --param allow-store-data-races on any benchmark?
>
> It's trivially to write one. The only pass that checks the param is
> tree loop invariant motion and it does that when it applies store-motion.
> Register pressure increase is increased by a factor of two.
>
> So I'd agree that we might want to disable this again for -Ofast.
>
> As nothing tests for the PACKED variants nor for the LOAD variant
> I'd rather remove those. Claiming we don't create races for those
> when you disable it via the param is simply not true.
>
> Thanks,
> Richard.
>

OK, please go ahead with your patch.

Thanks
Bernd.
 		 	   		  

Re: [PATCH] Change default for --param allow-...-data-races to off

[Martin Jambor]

On Mon, Jun 23, 2014 at 03:35:01PM +0200, Bernd Edlinger wrote:
> Hi Martin,
> 
> >>
> >> Well actually, I am not sure if we ever wanted to have a race condition here.
> >> Have you seen any impact of --param allow-store-data-races on any benchmark?
> >
> > It's trivially to write one. The only pass that checks the param is
> > tree loop invariant motion and it does that when it applies store-motion.
> > Register pressure increase is increased by a factor of two.
> >
> > So I'd agree that we might want to disable this again for -Ofast.
> >
> > As nothing tests for the PACKED variants nor for the LOAD variant
> > I'd rather remove those. Claiming we don't create races for those
> > when you disable it via the param is simply not true.
> >
> > Thanks,
> > Richard.
> >
> 
> OK, please go ahead with your patch.

Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
and tested on x86_64-linux.  OK for trunk?

Thanks,

Martin


2014-06-24  Martin Jambor  <mjambor@suse.cz>

	* params.def (PARAM_ALLOW_LOAD_DATA_RACES)
	(PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
	(PARAM_ALLOW_PACKED_STORE_DATA_RACES): Removed.
	(PARAM_ALLOW_STORE_DATA_RACES): Set default to zero.
	* opts.c (default_options_optimization): Set
	PARAM_ALLOW_STORE_DATA_RACES to one at -Ofast.
	* doc/invoke.texi (allow-load-data-races)
	(allow-packed-load-data-races, allow-packed-store-data-races):
	Removed.
	(allow-store-data-races): Document the new default.

testsuite/
	* g++.dg/simulate-thread/bitfields-2.C: Remove allow-load-data-races
	parameter.
	* g++.dg/simulate-thread/bitfields.C: Likewise.
	* gcc.dg/simulate-thread/strict-align-global.c: Remove
	allow-packed-store-data-races parameter.
	* gcc.dg/simulate-thread/subfields.c: Likewise.
	* gcc.dg/tree-ssa/20050314-1.c: Set parameter allow-store-data-races
	to one.

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 0d4bd00..027b6fb 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10176,25 +10176,10 @@ The maximum number of conditional stores paires that can be sunk.  Set to 0
 if either vectorization (@option{-ftree-vectorize}) or if-conversion
 (@option{-ftree-loop-if-convert}) is disabled.  The default is 2.
 
-@item allow-load-data-races
-Allow optimizers to introduce new data races on loads.
-Set to 1 to allow, otherwise to 0.  This option is enabled by default
-unless implicitly set by the @option{-fmemory-model=} option.
-
 @item allow-store-data-races
 Allow optimizers to introduce new data races on stores.
 Set to 1 to allow, otherwise to 0.  This option is enabled by default
-unless implicitly set by the @option{-fmemory-model=} option.
-
-@item allow-packed-load-data-races
-Allow optimizers to introduce new data races on packed data loads.
-Set to 1 to allow, otherwise to 0.  This option is enabled by default
-unless implicitly set by the @option{-fmemory-model=} option.
-
-@item allow-packed-store-data-races
-Allow optimizers to introduce new data races on packed data stores.
-Set to 1 to allow, otherwise to 0.  This option is enabled by default
-unless implicitly set by the @option{-fmemory-model=} option.
+at optimization level @option{-Ofast}.
 
 @item case-values-threshold
 The smallest number of different values for which it is best to use a
diff --git a/gcc/opts.c b/gcc/opts.c
index 3ab06c6..19203dc 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -620,6 +620,13 @@ default_options_optimization (struct gcc_options *opts,
      opt2 ? default_param_value (PARAM_LOOP_INVARIANT_MAX_BBS_IN_LOOP) : 1000,
      opts->x_param_values, opts_set->x_param_values);
 
+  /* At -Ofast, allow store motion to introduce potential race conditions.  */
+  maybe_set_param_value
+    (PARAM_ALLOW_STORE_DATA_RACES,
+     opts->x_optimize_fast ? 1
+     : default_param_value (PARAM_ALLOW_STORE_DATA_RACES),
+     opts->x_param_values, opts_set->x_param_values);
+
   if (opts->x_optimize_size)
     /* We want to crossjump as much as possible.  */
     maybe_set_param_value (PARAM_MIN_CROSSJUMP_INSNS, 1,
diff --git a/gcc/params.def b/gcc/params.def
index 28ef79a..aa1e88d 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1002,25 +1002,10 @@ DEFPARAM (PARAM_CASE_VALUES_THRESHOLD,
           0, 0, 0)
 
 /* Data race flags for C++0x memory model compliance.  */
-DEFPARAM (PARAM_ALLOW_LOAD_DATA_RACES,
-	  "allow-load-data-races",
-	  "Allow new data races on loads to be introduced",
-	  1, 0, 1)
-
 DEFPARAM (PARAM_ALLOW_STORE_DATA_RACES,
 	  "allow-store-data-races",
 	  "Allow new data races on stores to be introduced",
-	  1, 0, 1)
-
-DEFPARAM (PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
-	  "allow-packed-load-data-races",
-	  "Allow new data races on packed data loads to be introduced",
-	  1, 0, 1)
-
-DEFPARAM (PARAM_ALLOW_PACKED_STORE_DATA_RACES,
-	  "allow-packed-store-data-races",
-	  "Allow new data races on packed data stores to be introduced",
-	  1, 0, 1)
+	  0, 0, 1)
 
 /* Reassociation width to be used by tree reassoc optimization.  */
 DEFPARAM (PARAM_TREE_REASSOC_WIDTH,
diff --git a/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C b/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
index 077514a..be5d1ea 100644
--- a/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
+++ b/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
@@ -1,5 +1,5 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-load-data-races=0 --param allow-store-data-races=0" } */
+/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 /* Test that setting <var.a> does not touch either <var.b> or <var.c>.
diff --git a/gcc/testsuite/g++.dg/simulate-thread/bitfields.C b/gcc/testsuite/g++.dg/simulate-thread/bitfields.C
index 3acf21f..b829587 100644
--- a/gcc/testsuite/g++.dg/simulate-thread/bitfields.C
+++ b/gcc/testsuite/g++.dg/simulate-thread/bitfields.C
@@ -1,5 +1,5 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-load-data-races=0 --param allow-store-data-races=0" } */
+/* { dg-options "--param allow-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 /* Test that setting <var.a> does not touch either <var.b> or <var.c>.
diff --git a/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c b/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
index fdcd7f4..f8b88ad 100644
--- a/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
+++ b/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-packed-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
diff --git a/gcc/testsuite/gcc.dg/simulate-thread/subfields.c b/gcc/testsuite/gcc.dg/simulate-thread/subfields.c
index 2d93117..70e38a1 100644
--- a/gcc/testsuite/gcc.dg/simulate-thread/subfields.c
+++ b/gcc/testsuite/gcc.dg/simulate-thread/subfields.c
@@ -1,5 +1,4 @@
 /* { dg-do link } */
-/* { dg-options "--param allow-packed-store-data-races=0" } */
 /* { dg-final { simulate-thread } } */
 
 #include <stdio.h>
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c b/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
index 1082973..8f07781 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O1 -fdump-tree-lim1-details" } */
+/* { dg-options "-O1 -fdump-tree-lim1-details --param allow-store-data-races=1" } */
 
 float a[100];
 

Re: [PATCH] Change default for --param allow-...-data-races to off

[Richard Biener]

On Tue, Jun 24, 2014 at 10:19 PM, Martin Jambor <mjambor@suse.cz> wrote:
> On Mon, Jun 23, 2014 at 03:35:01PM +0200, Bernd Edlinger wrote:
>> Hi Martin,
>>
>> >>
>> >> Well actually, I am not sure if we ever wanted to have a race condition here.
>> >> Have you seen any impact of --param allow-store-data-races on any benchmark?
>> >
>> > It's trivially to write one. The only pass that checks the param is
>> > tree loop invariant motion and it does that when it applies store-motion.
>> > Register pressure increase is increased by a factor of two.
>> >
>> > So I'd agree that we might want to disable this again for -Ofast.
>> >
>> > As nothing tests for the PACKED variants nor for the LOAD variant
>> > I'd rather remove those. Claiming we don't create races for those
>> > when you disable it via the param is simply not true.
>> >
>> > Thanks,
>> > Richard.
>> >
>>
>> OK, please go ahead with your patch.
>
> Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
> and tested on x86_64-linux.  OK for trunk?

Ok - please give the C++/atomics folks a chance to comment.

This change of default behavior should also be documented in
gcc-4.10/changes.html.

Thanks,
Richard.

> Thanks,
>
> Martin
>
>
> 2014-06-24  Martin Jambor  <mjambor@suse.cz>
>
>         * params.def (PARAM_ALLOW_LOAD_DATA_RACES)
>         (PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
>         (PARAM_ALLOW_PACKED_STORE_DATA_RACES): Removed.
>         (PARAM_ALLOW_STORE_DATA_RACES): Set default to zero.
>         * opts.c (default_options_optimization): Set
>         PARAM_ALLOW_STORE_DATA_RACES to one at -Ofast.
>         * doc/invoke.texi (allow-load-data-races)
>         (allow-packed-load-data-races, allow-packed-store-data-races):
>         Removed.
>         (allow-store-data-races): Document the new default.
>
> testsuite/
>         * g++.dg/simulate-thread/bitfields-2.C: Remove allow-load-data-races
>         parameter.
>         * g++.dg/simulate-thread/bitfields.C: Likewise.
>         * gcc.dg/simulate-thread/strict-align-global.c: Remove
>         allow-packed-store-data-races parameter.
>         * gcc.dg/simulate-thread/subfields.c: Likewise.
>         * gcc.dg/tree-ssa/20050314-1.c: Set parameter allow-store-data-races
>         to one.
>
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 0d4bd00..027b6fb 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -10176,25 +10176,10 @@ The maximum number of conditional stores paires that can be sunk.  Set to 0
>  if either vectorization (@option{-ftree-vectorize}) or if-conversion
>  (@option{-ftree-loop-if-convert}) is disabled.  The default is 2.
>
> -@item allow-load-data-races
> -Allow optimizers to introduce new data races on loads.
> -Set to 1 to allow, otherwise to 0.  This option is enabled by default
> -unless implicitly set by the @option{-fmemory-model=} option.
> -
>  @item allow-store-data-races
>  Allow optimizers to introduce new data races on stores.
>  Set to 1 to allow, otherwise to 0.  This option is enabled by default
> -unless implicitly set by the @option{-fmemory-model=} option.
> -
> -@item allow-packed-load-data-races
> -Allow optimizers to introduce new data races on packed data loads.
> -Set to 1 to allow, otherwise to 0.  This option is enabled by default
> -unless implicitly set by the @option{-fmemory-model=} option.
> -
> -@item allow-packed-store-data-races
> -Allow optimizers to introduce new data races on packed data stores.
> -Set to 1 to allow, otherwise to 0.  This option is enabled by default
> -unless implicitly set by the @option{-fmemory-model=} option.
> +at optimization level @option{-Ofast}.
>
>  @item case-values-threshold
>  The smallest number of different values for which it is best to use a
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 3ab06c6..19203dc 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -620,6 +620,13 @@ default_options_optimization (struct gcc_options *opts,
>       opt2 ? default_param_value (PARAM_LOOP_INVARIANT_MAX_BBS_IN_LOOP) : 1000,
>       opts->x_param_values, opts_set->x_param_values);
>
> +  /* At -Ofast, allow store motion to introduce potential race conditions.  */
> +  maybe_set_param_value
> +    (PARAM_ALLOW_STORE_DATA_RACES,
> +     opts->x_optimize_fast ? 1
> +     : default_param_value (PARAM_ALLOW_STORE_DATA_RACES),
> +     opts->x_param_values, opts_set->x_param_values);
> +
>    if (opts->x_optimize_size)
>      /* We want to crossjump as much as possible.  */
>      maybe_set_param_value (PARAM_MIN_CROSSJUMP_INSNS, 1,
> diff --git a/gcc/params.def b/gcc/params.def
> index 28ef79a..aa1e88d 100644
> --- a/gcc/params.def
> +++ b/gcc/params.def
> @@ -1002,25 +1002,10 @@ DEFPARAM (PARAM_CASE_VALUES_THRESHOLD,
>            0, 0, 0)
>
>  /* Data race flags for C++0x memory model compliance.  */
> -DEFPARAM (PARAM_ALLOW_LOAD_DATA_RACES,
> -         "allow-load-data-races",
> -         "Allow new data races on loads to be introduced",
> -         1, 0, 1)
> -
>  DEFPARAM (PARAM_ALLOW_STORE_DATA_RACES,
>           "allow-store-data-races",
>           "Allow new data races on stores to be introduced",
> -         1, 0, 1)
> -
> -DEFPARAM (PARAM_ALLOW_PACKED_LOAD_DATA_RACES,
> -         "allow-packed-load-data-races",
> -         "Allow new data races on packed data loads to be introduced",
> -         1, 0, 1)
> -
> -DEFPARAM (PARAM_ALLOW_PACKED_STORE_DATA_RACES,
> -         "allow-packed-store-data-races",
> -         "Allow new data races on packed data stores to be introduced",
> -         1, 0, 1)
> +         0, 0, 1)
>
>  /* Reassociation width to be used by tree reassoc optimization.  */
>  DEFPARAM (PARAM_TREE_REASSOC_WIDTH,
> diff --git a/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C b/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
> index 077514a..be5d1ea 100644
> --- a/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
> +++ b/gcc/testsuite/g++.dg/simulate-thread/bitfields-2.C
> @@ -1,5 +1,5 @@
>  /* { dg-do link } */
> -/* { dg-options "--param allow-load-data-races=0 --param allow-store-data-races=0" } */
> +/* { dg-options "--param allow-store-data-races=0" } */
>  /* { dg-final { simulate-thread } } */
>
>  /* Test that setting <var.a> does not touch either <var.b> or <var.c>.
> diff --git a/gcc/testsuite/g++.dg/simulate-thread/bitfields.C b/gcc/testsuite/g++.dg/simulate-thread/bitfields.C
> index 3acf21f..b829587 100644
> --- a/gcc/testsuite/g++.dg/simulate-thread/bitfields.C
> +++ b/gcc/testsuite/g++.dg/simulate-thread/bitfields.C
> @@ -1,5 +1,5 @@
>  /* { dg-do link } */
> -/* { dg-options "--param allow-load-data-races=0 --param allow-store-data-races=0" } */
> +/* { dg-options "--param allow-store-data-races=0" } */
>  /* { dg-final { simulate-thread } } */
>
>  /* Test that setting <var.a> does not touch either <var.b> or <var.c>.
> diff --git a/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c b/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
> index fdcd7f4..f8b88ad 100644
> --- a/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
> +++ b/gcc/testsuite/gcc.dg/simulate-thread/strict-align-global.c
> @@ -1,5 +1,4 @@
>  /* { dg-do link } */
> -/* { dg-options "--param allow-packed-store-data-races=0" } */
>  /* { dg-final { simulate-thread } } */
>
>  #include <stdio.h>
> diff --git a/gcc/testsuite/gcc.dg/simulate-thread/subfields.c b/gcc/testsuite/gcc.dg/simulate-thread/subfields.c
> index 2d93117..70e38a1 100644
> --- a/gcc/testsuite/gcc.dg/simulate-thread/subfields.c
> +++ b/gcc/testsuite/gcc.dg/simulate-thread/subfields.c
> @@ -1,5 +1,4 @@
>  /* { dg-do link } */
> -/* { dg-options "--param allow-packed-store-data-races=0" } */
>  /* { dg-final { simulate-thread } } */
>
>  #include <stdio.h>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c b/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
> index 1082973..8f07781 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/20050314-1.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O1 -fdump-tree-lim1-details" } */
> +/* { dg-options "-O1 -fdump-tree-lim1-details --param allow-store-data-races=1" } */
>
>  float a[100];
>

Re: [PATCH] Change default for --param allow-...-data-races to off

[Jakub Jelinek]

On Wed, Jun 25, 2014 at 10:14:17AM +0200, Richard Biener wrote:
> > Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
> > and tested on x86_64-linux.  OK for trunk?
> 
> Ok - please give the C++/atomics folks a chance to comment.
> 
> This change of default behavior should also be documented in
> gcc-4.10/changes.html.

Do we want to allow the store data races by default with -Ofast even in strict
conformance modes (-std=c++11, -std=c++14, -std=c11)?

	Jakub

Re: [PATCH] Change default for --param allow-...-data-races to off

[Richard Biener]

On Wed, Jun 25, 2014 at 10:54 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jun 25, 2014 at 10:14:17AM +0200, Richard Biener wrote:
>> > Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
>> > and tested on x86_64-linux.  OK for trunk?
>>
>> Ok - please give the C++/atomics folks a chance to comment.
>>
>> This change of default behavior should also be documented in
>> gcc-4.10/changes.html.
>
> Do we want to allow the store data races by default with -Ofast even in strict
> conformance modes (-std=c++11, -std=c++14, -std=c11)?

I think so.  -Ofast means -Ofast (same issue with fp contraction and other
stuff -ffast-math enables that is not valid in strict conformance mode).

Richard.

Re: [PATCH] Change default for --param allow-...-data-races to off

[Marc Glisse]

On Wed, 25 Jun 2014, Richard Biener wrote:

> On Wed, Jun 25, 2014 at 10:54 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Jun 25, 2014 at 10:14:17AM +0200, Richard Biener wrote:
>>>> Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
>>>> and tested on x86_64-linux.  OK for trunk?
>>>
>>> Ok - please give the C++/atomics folks a chance to comment.
>>>
>>> This change of default behavior should also be documented in
>>> gcc-4.10/changes.html.
>>
>> Do we want to allow the store data races by default with -Ofast even in strict
>> conformance modes (-std=c++11, -std=c++14, -std=c11)?
>
> I think so.  -Ofast means -Ofast (same issue with fp contraction and other
> stuff -ffast-math enables that is not valid in strict conformance mode).

One thing I am missing is a -single-thread option that would allow races, 
remove atomics and thread locals, etc, without breaking conformance as 
long as no second thread is created. I hope that not too many libraries 
use threads internally in a way that would break this.

-- 
Marc Glisse

Re: [PATCH] Change default for --param allow-...-data-races to off

[Jeff Law]

On 06/24/14 14:19, Martin Jambor wrote:
> On Mon, Jun 23, 2014 at 03:35:01PM +0200, Bernd Edlinger wrote:
>> Hi Martin,
>>
>>>>
>>>> Well actually, I am not sure if we ever wanted to have a race condition here.
>>>> Have you seen any impact of --param allow-store-data-races on any benchmark?
>>>
>>> It's trivially to write one. The only pass that checks the param is
>>> tree loop invariant motion and it does that when it applies store-motion.
>>> Register pressure increase is increased by a factor of two.
>>>
>>> So I'd agree that we might want to disable this again for -Ofast.
>>>
>>> As nothing tests for the PACKED variants nor for the LOAD variant
>>> I'd rather remove those. Claiming we don't create races for those
>>> when you disable it via the param is simply not true.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>> OK, please go ahead with your patch.
>
> Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
> and tested on x86_64-linux.  OK for trunk?
>
> Thanks,
>
> Martin
>
>
> 2014-06-24  Martin Jambor  <mjambor@suse.cz>
>
> 	* params.def (PARAM_ALLOW_LOAD_DATA_RACES)
> 	(PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
> 	(PARAM_ALLOW_PACKED_STORE_DATA_RACES): Removed.
> 	(PARAM_ALLOW_STORE_DATA_RACES): Set default to zero.
> 	* opts.c (default_options_optimization): Set
> 	PARAM_ALLOW_STORE_DATA_RACES to one at -Ofast.
> 	* doc/invoke.texi (allow-load-data-races)
> 	(allow-packed-load-data-races, allow-packed-store-data-races):
> 	Removed.
> 	(allow-store-data-races): Document the new default.
>
> testsuite/
> 	* g++.dg/simulate-thread/bitfields-2.C: Remove allow-load-data-races
> 	parameter.
> 	* g++.dg/simulate-thread/bitfields.C: Likewise.
> 	* gcc.dg/simulate-thread/strict-align-global.c: Remove
> 	allow-packed-store-data-races parameter.
> 	* gcc.dg/simulate-thread/subfields.c: Likewise.
> 	* gcc.dg/tree-ssa/20050314-1.c: Set parameter allow-store-data-races
> 	to one.
Don't we want to deprecate, not remove the dead options?


Jeff

Re: [PATCH] Change default for --param allow-...-data-races to off

[Martin Jambor]

Hi,

On Wed, Jun 25, 2014 at 03:14:31PM -0600, Jeff Law wrote:
> On 06/24/14 14:19, Martin Jambor wrote:
> >On Mon, Jun 23, 2014 at 03:35:01PM +0200, Bernd Edlinger wrote:
> >>Hi Martin,
> >>
> >>>>
> >>>>Well actually, I am not sure if we ever wanted to have a race condition here.
> >>>>Have you seen any impact of --param allow-store-data-races on any benchmark?
> >>>
> >>>It's trivially to write one. The only pass that checks the param is
> >>>tree loop invariant motion and it does that when it applies store-motion.
> >>>Register pressure increase is increased by a factor of two.
> >>>
> >>>So I'd agree that we might want to disable this again for -Ofast.
> >>>
> >>>As nothing tests for the PACKED variants nor for the LOAD variant
> >>>I'd rather remove those. Claiming we don't create races for those
> >>>when you disable it via the param is simply not true.
> >>>
> >>>Thanks,
> >>>Richard.
> >>>
> >>
> >>OK, please go ahead with your patch.
> >
> >Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
> >and tested on x86_64-linux.  OK for trunk?
> >
> >Thanks,
> >
> >Martin
> >
> >
> >2014-06-24  Martin Jambor  <mjambor@suse.cz>
> >
> >	* params.def (PARAM_ALLOW_LOAD_DATA_RACES)
> >	(PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
> >	(PARAM_ALLOW_PACKED_STORE_DATA_RACES): Removed.
> >	(PARAM_ALLOW_STORE_DATA_RACES): Set default to zero.
> >	* opts.c (default_options_optimization): Set
> >	PARAM_ALLOW_STORE_DATA_RACES to one at -Ofast.
> >	* doc/invoke.texi (allow-load-data-races)
> >	(allow-packed-load-data-races, allow-packed-store-data-races):
> >	Removed.
> >	(allow-store-data-races): Document the new default.
> >
> >testsuite/
> >	* g++.dg/simulate-thread/bitfields-2.C: Remove allow-load-data-races
> >	parameter.
> >	* g++.dg/simulate-thread/bitfields.C: Likewise.
> >	* gcc.dg/simulate-thread/strict-align-global.c: Remove
> >	allow-packed-store-data-races parameter.
> >	* gcc.dg/simulate-thread/subfields.c: Likewise.
> >	* gcc.dg/tree-ssa/20050314-1.c: Set parameter allow-store-data-races
> >	to one.
> Don't we want to deprecate, not remove the dead options?
> 

Is there a mechanism for deprecating parameters (I could not quickly
find any) or do you mean to leave them there and only document them as
deprecated?

I am not really concerned how we deal with the unused parameters,
removing or any form of deprecating is fine with me.

Thanks,

Martin

Re: [PATCH] Change default for --param allow-...-data-races to off

[Richard Biener]

On June 26, 2014 12:03:21 AM CEST, Martin Jambor <mjambor@suse.cz> wrote:
>Hi,
>
>On Wed, Jun 25, 2014 at 03:14:31PM -0600, Jeff Law wrote:
>> On 06/24/14 14:19, Martin Jambor wrote:
>> >On Mon, Jun 23, 2014 at 03:35:01PM +0200, Bernd Edlinger wrote:
>> >>Hi Martin,
>> >>
>> >>>>
>> >>>>Well actually, I am not sure if we ever wanted to have a race
>condition here.
>> >>>>Have you seen any impact of --param allow-store-data-races on any
>benchmark?
>> >>>
>> >>>It's trivially to write one. The only pass that checks the param
>is
>> >>>tree loop invariant motion and it does that when it applies
>store-motion.
>> >>>Register pressure increase is increased by a factor of two.
>> >>>
>> >>>So I'd agree that we might want to disable this again for -Ofast.
>> >>>
>> >>>As nothing tests for the PACKED variants nor for the LOAD variant
>> >>>I'd rather remove those. Claiming we don't create races for those
>> >>>when you disable it via the param is simply not true.
>> >>>
>> >>>Thanks,
>> >>>Richard.
>> >>>
>> >>
>> >>OK, please go ahead with your patch.
>> >
>> >Perhaps not unsurprisingly, the patch is very similar.  Bootstrapped
>> >and tested on x86_64-linux.  OK for trunk?
>> >
>> >Thanks,
>> >
>> >Martin
>> >
>> >
>> >2014-06-24  Martin Jambor  <mjambor@suse.cz>
>> >
>> >	* params.def (PARAM_ALLOW_LOAD_DATA_RACES)
>> >	(PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
>> >	(PARAM_ALLOW_PACKED_STORE_DATA_RACES): Removed.
>> >	(PARAM_ALLOW_STORE_DATA_RACES): Set default to zero.
>> >	* opts.c (default_options_optimization): Set
>> >	PARAM_ALLOW_STORE_DATA_RACES to one at -Ofast.
>> >	* doc/invoke.texi (allow-load-data-races)
>> >	(allow-packed-load-data-races, allow-packed-store-data-races):
>> >	Removed.
>> >	(allow-store-data-races): Document the new default.
>> >
>> >testsuite/
>> >	* g++.dg/simulate-thread/bitfields-2.C: Remove
>allow-load-data-races
>> >	parameter.
>> >	* g++.dg/simulate-thread/bitfields.C: Likewise.
>> >	* gcc.dg/simulate-thread/strict-align-global.c: Remove
>> >	allow-packed-store-data-races parameter.
>> >	* gcc.dg/simulate-thread/subfields.c: Likewise.
>> >	* gcc.dg/tree-ssa/20050314-1.c: Set parameter
>allow-store-data-races
>> >	to one.
>> Don't we want to deprecate, not remove the dead options?
>> 
>
>Is there a mechanism for deprecating parameters (I could not quickly
>find any) or do you mean to leave them there and only document them as
>deprecated?
>
>I am not really concerned how we deal with the unused parameters,
>removing or any form of deprecating is fine with me.

--params are not a stable interface, so we can just remove those.  Of course this would be the opportunity to introduce a real option for this task and leave the param as an implementation detail.

Richard.

>Thanks,
>
>Martin

RE: [PATCH] Change default for --param allow-...-data-races to off

[Bernd Edlinger]

Hi,

On Thu, 26 Jun 2014 08:43:46, Richard Biener wrote:
>
> On June 26, 2014 12:03:21 AM CEST, Martin Jambor <mjambor@suse.cz> wrote:
>>Hi,
>>
>>On Wed, Jun 25, 2014 at 03:14:31PM -0600, Jeff Law wrote:
>>> On 06/24/14 14:19, Martin Jambor wrote:
>>>>On Mon, Jun 23, 2014 at 03:35:01PM +0200, Bernd Edlinger wrote:
>>>>>Hi Martin,
>>>>>
>>>>>>>
>>>>>>>Well actually, I am not sure if we ever wanted to have a race
>>condition here.
>>>>>>>Have you seen any impact of --param allow-store-data-races on any
>>benchmark?
>>>>>>
>>>>>>It's trivially to write one. The only pass that checks the param
>>is
>>>>>>tree loop invariant motion and it does that when it applies
>>store-motion.
>>>>>>Register pressure increase is increased by a factor of two.
>>>>>>
>>>>>>So I'd agree that we might want to disable this again for -Ofast.
>>>>>>
>>>>>>As nothing tests for the PACKED variants nor for the LOAD variant
>>>>>>I'd rather remove those. Claiming we don't create races for those
>>>>>>when you disable it via the param is simply not true.
>>>>>>
>>>>>>Thanks,
>>>>>>Richard.
>>>>>>
>>>>>
>>>>>OK, please go ahead with your patch.
>>>>
>>>>Perhaps not unsurprisingly, the patch is very similar. Bootstrapped
>>>>and tested on x86_64-linux. OK for trunk?
>>>>
>>>>Thanks,
>>>>
>>>>Martin
>>>>
>>>>
>>>>2014-06-24 Martin Jambor <mjambor@suse.cz>
>>>>
>>>> * params.def (PARAM_ALLOW_LOAD_DATA_RACES)
>>>> (PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
>>>> (PARAM_ALLOW_PACKED_STORE_DATA_RACES): Removed.
>>>> (PARAM_ALLOW_STORE_DATA_RACES): Set default to zero.
>>>> * opts.c (default_options_optimization): Set
>>>> PARAM_ALLOW_STORE_DATA_RACES to one at -Ofast.
>>>> * doc/invoke.texi (allow-load-data-races)
>>>> (allow-packed-load-data-races, allow-packed-store-data-races):
>>>> Removed.
>>>> (allow-store-data-races): Document the new default.
>>>>
>>>>testsuite/
>>>> * g++.dg/simulate-thread/bitfields-2.C: Remove
>>allow-load-data-races
>>>> parameter.
>>>> * g++.dg/simulate-thread/bitfields.C: Likewise.
>>>> * gcc.dg/simulate-thread/strict-align-global.c: Remove
>>>> allow-packed-store-data-races parameter.
>>>> * gcc.dg/simulate-thread/subfields.c: Likewise.
>>>> * gcc.dg/tree-ssa/20050314-1.c: Set parameter
>>allow-store-data-races
>>>> to one.
>>> Don't we want to deprecate, not remove the dead options?
>>>
>>
>>Is there a mechanism for deprecating parameters (I could not quickly
>>find any) or do you mean to leave them there and only document them as
>>deprecated?
>>
>>I am not really concerned how we deal with the unused parameters,
>>removing or any form of deprecating is fine with me.
>
> --params are not a stable interface, so we can just remove those. Of course this would be the opportunity to introduce a real option for this task and leave the param as an implementation detail.
>

well, of course, given the fact that the --param allow-store-data-races=0 is actually used now
by linux kernel makefiles we should keep this parameter.

I'd agree with Richard about the other parameters.

Note however that they are not really a secret any more:

See https://gcc.gnu.org/wiki/Atomic/GCCMM/ExecutiveSummary
where these --params are documented, should this page be adjusted too when
we remove them?


Bernd.

> Richard.
>
>>Thanks,
>>
>>Martin
>
>