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

[Richard Biener <richard dot guenther at gmail dot com>]

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];
>