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
>From d5a8097dd07d1a3f4263da7ccad970543d92f3e9 Mon Sep 17 00:00:00 2001
From: marxin <mli...@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 <mli...@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
