This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Change default for --param allow-...-data-races to off
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- Cc: Martin Jambor <mjambor at suse dot cz>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 23 Jun 2014 10:50:16 +0200
- Subject: Re: [PATCH] Change default for --param allow-...-data-races to off
- Authentication-results: sourceware.org; auth=none
- References: <DUB118-W318F2CAA5BBAC891FBC6D6E4130 at phx dot gbl> <20140620114418 dot GB24436 at virgil dot suse> <DUB118-W4683B0ED69FBB39F01AB5DE41F0 at phx dot gbl>
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.
>>>
>>
>>
>