From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17641 invoked by alias); 25 Jun 2014 08:14:25 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 17628 invoked by uid 89); 25 Jun 2014 08:14:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-we0-f177.google.com Received: from mail-we0-f177.google.com (HELO mail-we0-f177.google.com) (74.125.82.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 25 Jun 2014 08:14:22 +0000 Received: by mail-we0-f177.google.com with SMTP id u56so1553453wes.36 for ; Wed, 25 Jun 2014 01:14:17 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.90.207 with SMTP id by15mr39013989wib.51.1403684057789; Wed, 25 Jun 2014 01:14:17 -0700 (PDT) Received: by 10.195.11.202 with HTTP; Wed, 25 Jun 2014 01:14:17 -0700 (PDT) In-Reply-To: <20140624201933.GB32150@virgil.suse> References: <20140620114418.GB24436@virgil.suse> <20140624201933.GB32150@virgil.suse> Date: Wed, 25 Jun 2014 08:14:00 -0000 Message-ID: Subject: Re: [PATCH] Change default for --param allow-...-data-races to off From: Richard Biener To: Bernd Edlinger , Richard Biener , "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-06/txt/msg01976.txt.bz2 On Tue, Jun 24, 2014 at 10:19 PM, 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? 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 > > * 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 does not touch either or . > 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 does not touch either or . > 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 > 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 > 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]; >