From patchwork Tue Jun 24 20:19:33 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 363665 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4521E1400B5 for ; Wed, 25 Jun 2014 06:19:57 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=DCXER39r0/PxvH6pm FXy4TfArdV4OwLQxXyxZKp3VK3vEXDw+Dzz9liZa5D7dg4pbiLU+GA8kjuMrQ2N7 tEfyxOqc8b4McBeHZ2Jr4jos/BCnali5vsJOml30oagC3oSlbSA7GtjbJGGeMb0D RBWnhHzNd8mYjoCJbPAf7upOI0= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=fbp3S5tOdUCq3NcTRfKLH1O l/xU=; b=ptzlTW5zccOBr8TYJlNAm6rNnqqd51MR67qRdF+cETyu+qiSylitL7+ WtYph+/p93ZAf6x0tnIrb7BbojsYUEA1+WQsKgnXE4GBseuh8at3+/BPtC875Pny Ub9GaDMh3TWP3IAb/oIMnXSfbuoO2NLIgMzSNBb4iejhPyjCmDpo= Received: (qmail 23043 invoked by alias); 24 Jun 2014 20:19:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 22898 invoked by uid 89); 24 Jun 2014 20:19:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Tue, 24 Jun 2014 20:19:37 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 1AD0AAD7C; Tue, 24 Jun 2014 20:19:34 +0000 (UTC) Date: Tue, 24 Jun 2014 22:19:33 +0200 From: Martin Jambor To: Bernd Edlinger Cc: Richard Biener , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH] Change default for --param allow-...-data-races to off Message-ID: <20140624201933.GB32150@virgil.suse> Mail-Followup-To: Bernd Edlinger , Richard Biener , "gcc-patches@gcc.gnu.org" References: <20140620114418.GB24436@virgil.suse> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 * 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];