diff mbox

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

Message ID 2c750f4c-c96c-2b22-43ae-53bbebf18af8@suse.cz
State New
Headers show

Commit Message

Martin Liška Oct. 3, 2016, 12:13 p.m. UTC 
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.

Moreover, I also have a patch that provides a warning, which can be also useful
even though we would change the default behavior:

$ ./xgcc -B. /tmp/a.c -fprofile-update=single -pthread -fprofile-generate
xgcc: warning: -profile-update=atomic should be used to generate a valid profile for a multithreaded application

Ideas?
Martin

Comments

Nathan Sidwell Oct. 3, 2016, 12:26 p.m. UTC | #1 
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
Jeff Law Oct. 3, 2016, 4:46 p.m. UTC | #2 
On 10/03/2016 06:26 AM, 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.
Likewise.
jeff
Andi Kleen Oct. 3, 2016, 5:52 p.m. UTC | #3 
> >>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.
> Likewise.

I still think it shouldn't be default even with -pthread because it could dramatically
degrade performance in these cases. People likely have -pthread in their Makefiles
without realizing it. Such changes should be explict opt-in.

Often severe performance decreases lead to incorrectness in practice
("is now too slow to finish training workload in rebuild cycle") 

-Andi
diff mbox

Patch

From d5a8097dd07d1a3f4263da7ccad970543d92f3e9 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 3 Oct 2016 14:02:14 +0200
Subject: [PATCH] Warn about -fprofile-update=single and -pthread

gcc/ChangeLog:

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

	* common.opt: Mark couple of flags with 'Driver' keyword.
	* gcc.c (driver_handle_option): Handle these options.
	(process_command): Generate the warning.
---
 gcc/common.opt |  8 ++++----
 gcc/gcc.c      | 31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index 0e01577..3af9c64 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1920,7 +1920,7 @@  Common Report Var(profile_flag)
 Enable basic program profiling code.
 
 fprofile-arcs
-Common Report Var(profile_arc_flag)
+Common Driver Report Var(profile_arc_flag)
 Insert arc-based program profiling code.
 
 fprofile-dir=
@@ -1933,7 +1933,7 @@  Common Report Var(flag_profile_correction)
 Enable correction of flow inconsistent profile data input.
 
 fprofile-update=
-Common Joined RejectNegative Enum(profile_update) Var(flag_profile_update) Init(PROFILE_UPDATE_SINGLE)
+Common Driver Joined RejectNegative Enum(profile_update) Var(flag_profile_update) Init(PROFILE_UPDATE_SINGLE)
 -fprofile-update=[single|atomic]	Set the profile update method.
 
 Enum
@@ -1946,11 +1946,11 @@  EnumValue
 Enum(profile_update) String(atomic) Value(PROFILE_UPDATE_ATOMIC)
 
 fprofile-generate
-Common
+Common Driver
 Enable common options for generating profile info for profile feedback directed optimizations.
 
 fprofile-generate=
-Common Joined RejectNegative
+Common Driver Joined RejectNegative
 Enable common options for generating profile info for profile feedback directed optimizations, and set -fprofile-dir=.
 
 fprofile-use
diff --git a/gcc/gcc.c b/gcc/gcc.c
index d3e8c88..b023013 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -233,6 +233,16 @@  static int print_subprocess_help;
 /* Linker suffix passed to -fuse-ld=... */
 static const char *use_ld;
 
+/* Flag indicating whether pthread is provided as a command line option.  */
+static bool pthread_set = false;
+
+/* Flag indicating whether profiling is enabled by an option  */
+static bool profiling_enabled = false;
+
+/* Flag indicating whether profile-update=atomic is provided as a command
+   line option.  */
+static bool profile_update_atomic = false;
+
 /* Whether we should report subprocess execution times to a file.  */
 
 FILE *report_times_to_file = NULL;
@@ -4112,6 +4122,22 @@  driver_handle_option (struct gcc_options *opts,
       handle_foffload_option (arg);
       break;
 
+    case OPT_fprofile_update_:
+      if ((profile_update)value == PROFILE_UPDATE_ATOMIC)
+	profile_update_atomic = true;
+      break;
+
+    case OPT_pthread:
+      pthread_set = true;
+      break;
+
+    case OPT_fprofile_generate:
+    case OPT_fprofile_generate_:
+    case OPT_fprofile_arcs:
+    case OPT_coverage:
+      profiling_enabled = true;
+      break;
+
     default:
       /* Various driver options need no special processing at this
 	 point, having been handled in a prescan above or being
@@ -4580,6 +4606,11 @@  process_command (unsigned int decoded_options_count,
       add_infile ("help-dummy", "c");
     }
 
+  /* Warn about multi-threaded program that do not use -profile=atomic.  */
+  if (profiling_enabled && pthread_set && !profile_update_atomic)
+    warning (0, "-profile-update=atomic should be used to generate a valid"
+	     " profile for a multithreaded application");
+
   /* Decide if undefined variable references are allowed in specs.  */
 
   /* --version and --help alone or together are safe.  Note that -v would
-- 
2.9.2