diff mbox

Set -fprofile-update=atomic when -pthread is present

Message ID 9aca1f7c-e7eb-e101-249e-8b5edd21cd48@suse.cz
State New
Headers show

Commit Message

Martin Liška Oct. 4, 2016, 12:05 p.m. UTC
On 10/03/2016 02:26 PM, Nathan Sidwell wrote:
> On 10/03/16 08:13, Martin Liška wrote:
>> On 08/18/2016 05:53 PM, Jeff Law wrote:
>>> On 08/18/2016 09:51 AM, Andi Kleen wrote:
>>>>> I'd prefer to make updates atomic in multi-threaded applications.
>>>>> The best proxy we have for that is -pthread.
>>>>>
>>>>> Is it slower, most definitely, but odds are we're giving folks
>>>>> garbage data otherwise, which in many ways is even worse.
>>>>
>>>> It will likely be catastrophically slower in some cases.
>>>>
>>>> Catastrophically as in too slow to be usable.
>>>>
>>>> An atomic instruction is a lot more expensive than a single increment. Also
>>>> they sometimes are really slow depending on the state of the machine.
>>> And for those cases there's a way to override.
>>>
>>> The default should be set for correctness.
>>>
>>> jeff
>>
>> I would to somehow resolve the discussion related to default value selection.
>> Is the prevailing consensus that we should set -fprofile-update=atomic when
>> -pthread is set? If so, I'll prepare a patch. I tend to do it this way.
> 
> This is my preference.
> 
> nathan

Ok, this is final version of patch which implements both the warning and appending -fprofile-update
to a command line options.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

Comments

Jeff Law Oct. 5, 2016, 5:54 p.m. UTC | #1
On 10/04/2016 06:05 AM, Martin Liška wrote:
> On 10/03/2016 02:26 PM, Nathan Sidwell wrote:
>> > On 10/03/16 08:13, Martin Liška wrote:
>>> >> On 08/18/2016 05:53 PM, Jeff Law wrote:
>>>> >>> On 08/18/2016 09:51 AM, Andi Kleen wrote:
>>>>>> >>>>> I'd prefer to make updates atomic in multi-threaded applications.
>>>>>> >>>>> The best proxy we have for that is -pthread.
>>>>>> >>>>>
>>>>>> >>>>> Is it slower, most definitely, but odds are we're giving folks
>>>>>> >>>>> garbage data otherwise, which in many ways is even worse.
>>>>> >>>>
>>>>> >>>> It will likely be catastrophically slower in some cases.
>>>>> >>>>
>>>>> >>>> Catastrophically as in too slow to be usable.
>>>>> >>>>
>>>>> >>>> An atomic instruction is a lot more expensive than a single increment. Also
>>>>> >>>> they sometimes are really slow depending on the state of the machine.
>>>> >>> And for those cases there's a way to override.
>>>> >>>
>>>> >>> The default should be set for correctness.
>>>> >>>
>>>> >>> jeff
>>> >>
>>> >> I would to somehow resolve the discussion related to default value selection.
>>> >> Is the prevailing consensus that we should set -fprofile-update=atomic when
>>> >> -pthread is set? If so, I'll prepare a patch. I tend to do it this way.
>> >
>> > This is my preference.
>> >
>> > nathan
> Ok, this is final version of patch which implements both the warning and appending -fprofile-update
> to a command line options.
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
> Martin
>
>
> 0001-Add-fprofile-update-option-juggling.patch
>
>
> From 343d64a3c6b515053459a8ece6f9b0ad6ce86273 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Tue, 4 Oct 2016 10:46:48 +0200
> Subject: [PATCH] Add -fprofile-update option juggling
>
> gcc/ChangeLog:
>
> 2016-10-04  Martin Liska  <mliska@suse.cz>
>
> 	* gcc.c: Set -fprofile-update=atomic when profiling is
> 	enabled and -pthread is set.  Warn when one combines
> 	-pthread and -fprofile-update=single for an app using
> 	profiling code.
OK.
jeff
diff mbox

Patch

From 343d64a3c6b515053459a8ece6f9b0ad6ce86273 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 4 Oct 2016 10:46:48 +0200
Subject: [PATCH] Add -fprofile-update option juggling

gcc/ChangeLog:

2016-10-04  Martin Liska  <mliska@suse.cz>

	* gcc.c: Set -fprofile-update=atomic when profiling is
	enabled and -pthread is set.  Warn when one combines
	-pthread and -fprofile-update=single for an app using
	profiling code.
---
 gcc/gcc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/gcc/gcc.c b/gcc/gcc.c
index fd2b182..5213cb0 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -1141,7 +1141,14 @@  static const char *cc1_options =
  %{-help=*:--help=%*}\
  %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
  %{fsyntax-only:-o %j} %{-param*}\
- %{coverage:-fprofile-arcs -ftest-coverage}";
+ %{coverage:-fprofile-arcs -ftest-coverage}\
+ %{fprofile-arcs|fprofile-generate*|coverage:\
+   %{!fprofile-update=single:\
+     %{pthread:-fprofile-update=atomic}}}\
+ %{fprofile-update=single:\
+   %{fprofile-arcs|fprofile-generate*|coverage:\
+     %{pthread:%n-fprofile-update=atomic should be used\
+ for a multithreaded application}}}";
 
 static const char *asm_options =
 "%{-target-help:%:print-asm-header()} "
-- 
2.9.2