| Message ID | 2419555f-7271-d64d-94ac-ac97ee7cb953@suse.cz |
|---|---|
| State | New |
| Headers | show |
PING^1 On 10/13/2016 05:34 PM, Martin Liška wrote: > Hello. > > As it's very hard to guess from GCC driver whether a target supports atomic updates > for GCOV counter or not, I decided to come up with a new option value (maybe-atomic), > that would be transformed in a corresponding value (single or atomic) in tree-profile.c. > The GCC driver selects the option when -pthread is present in the command line. > > That should fix all tests failures seen on AIX target. > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > > Ready to be installed? > Martin >
PING^2 On 10/31/2016 10:13 AM, Martin Liška wrote: > PING^1 > > On 10/13/2016 05:34 PM, Martin Liška wrote: >> Hello. >> >> As it's very hard to guess from GCC driver whether a target supports atomic updates >> for GCOV counter or not, I decided to come up with a new option value (maybe-atomic), >> that would be transformed in a corresponding value (single or atomic) in tree-profile.c. >> The GCC driver selects the option when -pthread is present in the command line. >> >> That should fix all tests failures seen on AIX target. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Ready to be installed? >> Martin >>
On 11/10/2016 05:19 AM, Martin Liška wrote: >> On 10/13/2016 05:34 PM, Martin Liška wrote: >>> Hello. >>> >>> As it's very hard to guess from GCC driver whether a target supports atomic updates >>> for GCOV counter or not, I decided to come up with a new option value (maybe-atomic), >>> that would be transformed in a corresponding value (single or atomic) in tree-profile.c. >>> The GCC driver selects the option when -pthread is present in the command line. >>> >>> That should fix all tests failures seen on AIX target. >>> >>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>> >>> Ready to be installed? I dislike this. If it's hard for gcc itself to know, how much harder for the user must it be? (does gcc have another instance of an option that behaves 'prefer-A-or-B-if-you-can't'? It's also not clear what problem it's solving for the user? If the user needs atomic update, they should get a hard error if the target doesn't support it. If they don't need atomic, why ask for it? But as ever, I'm not going to veto it. nathan
On Thu, Nov 10, 2016 at 10:43 AM, Nathan Sidwell <nathan@acm.org> wrote: > On 11/10/2016 05:19 AM, Martin Liška wrote: > >>> On 10/13/2016 05:34 PM, Martin Liška wrote: >>>> >>>> Hello. >>>> >>>> As it's very hard to guess from GCC driver whether a target supports >>>> atomic updates >>>> for GCOV counter or not, I decided to come up with a new option value >>>> (maybe-atomic), >>>> that would be transformed in a corresponding value (single or atomic) in >>>> tree-profile.c. >>>> The GCC driver selects the option when -pthread is present in the >>>> command line. >>>> >>>> That should fix all tests failures seen on AIX target. >>>> >>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression >>>> tests. >>>> >>>> Ready to be installed? > > > I dislike this. If it's hard for gcc itself to know, how much harder for > the user must it be? (does gcc have another instance of an option that > behaves 'prefer-A-or-B-if-you-can't'? > > It's also not clear what problem it's solving for the user? If the user > needs atomic update, they should get a hard error if the target doesn't > support it. If they don't need atomic, why ask for it? > > But as ever, I'm not going to veto it. Do you have a better suggestion? gcc.c now imposes profile-update=atomic if -pthread is used, even if the target does not support profile-update=atomic. Either gcc.c must not impose profile-update=atomic or we need some way of differentiating between when the request should fail because the user really expects it and when the request should silently and gently be ignored. The atomic update feature is nice, but currently GCC is trying to be too smart to guess how important the feature is to the user. Thanks, David
On 11/10/2016 04:43 PM, Nathan Sidwell wrote: > On 11/10/2016 05:19 AM, Martin Liška wrote: > >>> On 10/13/2016 05:34 PM, Martin Liška wrote: >>>> Hello. >>>> >>>> As it's very hard to guess from GCC driver whether a target supports atomic updates >>>> for GCOV counter or not, I decided to come up with a new option value (maybe-atomic), >>>> that would be transformed in a corresponding value (single or atomic) in tree-profile.c. >>>> The GCC driver selects the option when -pthread is present in the command line. >>>> >>>> That should fix all tests failures seen on AIX target. >>>> >>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>> >>>> Ready to be installed? > > I dislike this. If it's hard for gcc itself to know, how much harder for the user must it be? (does gcc have another instance of an option that behaves 'prefer-A-or-B-if-you-can't'? > > It's also not clear what problem it's solving for the user? If the user needs atomic update, they should get a hard error if the target doesn't support it. If they don't need atomic, why ask for it? My initial motivation was to automatically selected -fprofile-update=atomic if supported by a target and when '-pthread' is present on command line. As it's very problematic to identify (from GCC driver) whether a target supports or not atomic updates, 'maybe' option is the only possible we can guess. > > But as ever, I'm not going to veto it. Other option is to disable selection of -fprofile-update=atomic automatically. Martin > > nathan >
On 11/10/2016 07:43 AM, Nathan Sidwell wrote: > On 11/10/2016 05:19 AM, Martin Liška wrote: > >>> On 10/13/2016 05:34 PM, Martin Liška wrote: >>>> Hello. >>>> >>>> As it's very hard to guess from GCC driver whether a target supports >>>> atomic updates >>>> for GCOV counter or not, I decided to come up with a new option >>>> value (maybe-atomic), >>>> that would be transformed in a corresponding value (single or >>>> atomic) in tree-profile.c. >>>> The GCC driver selects the option when -pthread is present in the >>>> command line. >>>> >>>> That should fix all tests failures seen on AIX target. >>>> >>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression >>>> tests. >>>> >>>> Ready to be installed? > > I dislike this. If it's hard for gcc itself to know, how much harder > for the user must it be? (does gcc have another instance of an option > that behaves 'prefer-A-or-B-if-you-can't'? Thinking further. why isn't the right solution for -fprofile-update=atomic when faced with a target that cannot support it to: a) issue an error and bail out at the first opportunity b) or issue a warning and fall back to single threaded update? For #b presumably there'll be the capability of suppressing that particular warning? nathan
On Thu, Nov 10, 2016 at 11:14 AM, Nathan Sidwell <nathan@acm.org> wrote: > On 11/10/2016 07:43 AM, Nathan Sidwell wrote: >> >> On 11/10/2016 05:19 AM, Martin Liška wrote: >> >>>> On 10/13/2016 05:34 PM, Martin Liška wrote: >>>>> >>>>> Hello. >>>>> >>>>> As it's very hard to guess from GCC driver whether a target supports >>>>> atomic updates >>>>> for GCOV counter or not, I decided to come up with a new option >>>>> value (maybe-atomic), >>>>> that would be transformed in a corresponding value (single or >>>>> atomic) in tree-profile.c. >>>>> The GCC driver selects the option when -pthread is present in the >>>>> command line. >>>>> >>>>> That should fix all tests failures seen on AIX target. >>>>> >>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression >>>>> tests. >>>>> >>>>> Ready to be installed? >> >> >> I dislike this. If it's hard for gcc itself to know, how much harder >> for the user must it be? (does gcc have another instance of an option >> that behaves 'prefer-A-or-B-if-you-can't'? > > > Thinking further. why isn't the right solution for -fprofile-update=atomic > when faced with a target that cannot support it to: > a) issue an error and bail out at the first opportunity > b) or issue a warning and fall back to single threaded update? > > For #b presumably there'll be the capability of suppressing that particular > warning? Because that incorrectly breaks a huge portion of the testsuite. that's not what the user intended. - David
On Thu, Nov 10, 2016 at 10:58 AM, Martin Liška <mliska@suse.cz> wrote: > On 11/10/2016 04:43 PM, Nathan Sidwell wrote: >> On 11/10/2016 05:19 AM, Martin Liška wrote: >> >>>> On 10/13/2016 05:34 PM, Martin Liška wrote: >>>>> Hello. >>>>> >>>>> As it's very hard to guess from GCC driver whether a target supports atomic updates >>>>> for GCOV counter or not, I decided to come up with a new option value (maybe-atomic), >>>>> that would be transformed in a corresponding value (single or atomic) in tree-profile.c. >>>>> The GCC driver selects the option when -pthread is present in the command line. >>>>> >>>>> That should fix all tests failures seen on AIX target. >>>>> >>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>>> >>>>> Ready to be installed? >> >> I dislike this. If it's hard for gcc itself to know, how much harder for the user must it be? (does gcc have another instance of an option that behaves 'prefer-A-or-B-if-you-can't'? >> >> It's also not clear what problem it's solving for the user? If the user needs atomic update, they should get a hard error if the target doesn't support it. If they don't need atomic, why ask for it? > > My initial motivation was to automatically selected -fprofile-update=atomic if supported by a target and when '-pthread' is present on command line. > As it's very problematic to identify (from GCC driver) whether a target supports or not atomic updates, 'maybe' option is the only possible we can guess. > >> >> But as ever, I'm not going to veto it. > > Other option is to disable selection of -fprofile-update=atomic automatically. Unfortunately, this cannot use a configure test or manually set value based on target because the same gcc.c driver is invoked with different options that may provide atomic update in some variants and no atomic update in other (e.g., -m64) because the profile counter is 64 bits. Maybe instead of adding "maybe", we need to change the severity of the warning so that the warning is not emitted by default. - David
On 11/10/2016 07:55 AM, David Edelsohn wrote: > gcc.c now imposes profile-update=atomic if -pthread is used, even if > the target does not support profile-update=atomic. ah, that's where this is coming from. nathan
On 11/10/2016 05:17 PM, David Edelsohn wrote: > Maybe instead of adding "maybe", we need to change the severity of the > warning so that the warning is not emitted by default. Adding the warning option to -Wextra can be solution. Is it acceptable approach? Martin
On 11/10/2016 08:24 AM, Martin Liška wrote: > On 11/10/2016 05:17 PM, David Edelsohn wrote: >> Maybe instead of adding "maybe", we need to change the severity of the >> warning so that the warning is not emitted by default. > > Adding the warning option to -Wextra can be solution. Is it acceptable > approach? I don't think that's good. Now I understand the -pthreads thing, we have different use cases. 1) user explicitly said -fprofile-update=FOO. They shouldn't have to enable something else to get a diagnostic that FOO doesn't work. 2) driver implicitly said -fprofile-update=FOO, because the user said -pthreads but the driver doesn't know if FOO is acceptable. We want to silently fallback to the old behaviour. The proposed solution addresses #2 by having the driver say -fprofile-update=META-FOO. My dislike is that we're exposing this to the user and they're going to start using it. That strikes me as undesirable. How hard is it to implement the fprofile-update option value as a list. I.e. '-fprofile-update=atomic,single', with semantics of 'pick the first one you can do'? If that's straightforwards, then that seems to me as a better solution for #2. [flyby-thought, have 'atomic,single' as an acceptable single option value?] Failing that, Martin's solution is probably the sanest available solution, but I'd like to rename 'maybe-atomic' to the more meaningful 'prefer-atomic'. With 'maybe-atomic', I'm left wondering if it looks at the phase of the moon. nathan
On 11/10/2016 06:31 PM, Nathan Sidwell wrote: > On 11/10/2016 08:24 AM, Martin Liška wrote: >> On 11/10/2016 05:17 PM, David Edelsohn wrote: >>> Maybe instead of adding "maybe", we need to change the severity of the >>> warning so that the warning is not emitted by default. >> >> Adding the warning option to -Wextra can be solution. Is it acceptable >> approach? > > I don't think that's good. Now I understand the -pthreads thing, we have different use cases. > > 1) user explicitly said -fprofile-update=FOO. They shouldn't have to enable something else to get a diagnostic that FOO doesn't work. > > 2) driver implicitly said -fprofile-update=FOO, because the user said -pthreads but the driver doesn't know if FOO is acceptable. We want to silently fallback to the old behaviour. > > The proposed solution addresses #2 by having the driver say -fprofile-update=META-FOO. My dislike is that we're exposing this to the user and they're going to start using it. That strikes me as undesirable. > > How hard is it to implement the fprofile-update option value as a list. I.e. '-fprofile-update=atomic,single', with semantics of 'pick the first one you can do'? If that's straightforwards, then that seems to me as a better solution for #2. [flyby-thought, have 'atomic,single' as an acceptable single option value?] Hello. We use lists like for -fsanitize=address,undefined, however as -fprofile-update has only 3 (and passing 'single,atomic' does not make sense), I would prefer to s/maybe-atomic/prefer-atomic. I guess handling the option list in gcc.c and doing substitutions would be very inconvenient. Thanks, MArtin > > Failing that, Martin's solution is probably the sanest available solution, but I'd like to rename 'maybe-atomic' to the more meaningful 'prefer-atomic'. With 'maybe-atomic', I'm left wondering if it looks at the phase of the moon. > > nathan >
On 11/11/2016 02:47 AM, Martin Liška wrote: > We use lists like for -fsanitize=address,undefined, however as -fprofile-update has only 3 (and passing 'single,atomic' does not make sense), I would prefer > to s/maybe-atomic/prefer-atomic. I guess handling the option list in gcc.c and doing substitutions would be very inconvenient. ok.
From 1d00b7b4d42d080fe4d6cd51a03829b0fe525c9d Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 12 Oct 2016 15:05:49 +0200 Subject: [PATCH] Introduce -fprofile-update=maybe-atomic gcc/ChangeLog: 2016-10-12 Martin Liska <mliska@suse.cz> * common.opt: Add maybe-atomic as a new enum value for -fprofile-update. * coretypes.h: Likewise. * doc/invoke.texi: Document the new option value. * gcc.c: Replace atomic with maybe-atomic. Remove warning. * tree-profile.c (tree_profiling): Select default value of -fprofile-update when 'maybe-atomic' is selected. gcc/testsuite/ChangeLog: 2016-10-12 Martin Liska <mliska@suse.cz> * gcc.dg/no_profile_instrument_function-attr-1.c: Update test to match scanned pattern. * gcc.dg/tree-ssa/ssa-lim-11.c: Likewise. --- gcc/common.opt | 5 +++- gcc/coretypes.h | 3 +- gcc/doc/invoke.texi | 11 +++++-- gcc/gcc.c | 6 +--- .../gcc.dg/no_profile_instrument_function-attr-1.c | 2 +- gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-11.c | 2 +- gcc/tree-profile.c | 35 +++++++++++----------- 7 files changed, 35 insertions(+), 29 deletions(-) diff --git a/gcc/common.opt b/gcc/common.opt index 15679c5..d6c5acd 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1937,7 +1937,7 @@ Enable correction of flow inconsistent profile data input. fprofile-update= Common Joined RejectNegative Enum(profile_update) Var(flag_profile_update) Init(PROFILE_UPDATE_SINGLE) --fprofile-update=[single|atomic] Set the profile update method. +-fprofile-update=[single|atomic|maybe-atomic] Set the profile update method. Enum Name(profile_update) Type(enum profile_update) UnknownError(unknown profile update method %qs) @@ -1948,6 +1948,9 @@ Enum(profile_update) String(single) Value(PROFILE_UPDATE_SINGLE) EnumValue Enum(profile_update) String(atomic) Value(PROFILE_UPDATE_ATOMIC) +EnumValue +Enum(profile_update) String(maybe-atomic) Value(PROFILE_UPDATE_MAYBE_ATOMIC) + fprofile-generate Common Enable common options for generating profile info for profile feedback directed optimizations. diff --git a/gcc/coretypes.h b/gcc/coretypes.h index fe1e984..aec2a6e 100644 --- a/gcc/coretypes.h +++ b/gcc/coretypes.h @@ -177,7 +177,8 @@ enum offload_abi { /* Types of profile update methods. */ enum profile_update { PROFILE_UPDATE_SINGLE, - PROFILE_UPDATE_ATOMIC + PROFILE_UPDATE_ATOMIC, + PROFILE_UPDATE_MAYBE_ATOMIC }; /* Types of unwind/exception handling info that can be generated. */ diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index c11f1d5..eb6cae3 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -10315,13 +10315,18 @@ To optimize the program based on the collected profile information, use Alter the update method for an application instrumented for profile feedback based optimization. The @var{method} argument should be one of -@samp{single} or @samp{atomic}. The first one is useful for single-threaded -applications, while the second one prevents profile corruption by emitting -thread-safe code. +@samp{single}, @samp{atomic} or @samp{maybe-atomic}. +The first one is useful for single-threaded applications, +while the second one prevents profile corruption by emitting thread-safe code. @strong{Warning:} When an application does not properly join all threads (or creates an detached thread), a profile file can be still corrupted. +Using @samp{maybe-atomic} would be transformed either to @samp{atomic}, +when supported by a target, or to @samp{single} otherwise. The GCC driver +automatically selects @samp{maybe-atomic} when @option{-pthread} +is present in the command line. + @item -fsanitize=address @opindex fsanitize=address Enable AddressSanitizer, a fast memory error detector. diff --git a/gcc/gcc.c b/gcc/gcc.c index 5213cb0..1959fc7 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -1144,11 +1144,7 @@ static const char *cc1_options = %{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}}}"; + %{pthread:-fprofile-update=maybe-atomic}}}"; static const char *asm_options = "%{-target-help:%:print-asm-header()} " diff --git a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c index c93d171..29bffd90 100644 --- a/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c +++ b/gcc/testsuite/gcc.dg/no_profile_instrument_function-attr-1.c @@ -1,4 +1,4 @@ -/* { dg-options "-O2 -fprofile-generate -fdump-tree-optimized" } */ +/* { dg-options "-O2 -fprofile-generate -fprofile-update=single -fdump-tree-optimized" } */ __attribute__ ((no_profile_instrument_function)) int foo() diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-11.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-11.c index e4c11aa..4c38982 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-11.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-lim-11.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O -fprofile-arcs -fdump-tree-lim2-details" } */ +/* { dg-options "-O -fprofile-arcs -fprofile-update=single -fdump-tree-lim2-details" } */ /* { dg-require-profiling "-fprofile-generate" } */ struct thread_param diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c index 1f45b99..fcef2e5 100644 --- a/gcc/tree-profile.c +++ b/gcc/tree-profile.c @@ -534,25 +534,26 @@ tree_profiling (void) struct cgraph_node *node; /* Verify whether we can utilize atomic update operations. */ - if (flag_profile_update == PROFILE_UPDATE_ATOMIC) + bool can_support_atomic = false; + unsigned HOST_WIDE_INT gcov_type_size + = tree_to_uhwi (TYPE_SIZE_UNIT (get_gcov_type ())); + if (gcov_type_size == 4) + can_support_atomic + = HAVE_sync_compare_and_swapsi || HAVE_atomic_compare_and_swapsi; + else if (gcov_type_size == 8) + can_support_atomic + = HAVE_sync_compare_and_swapdi || HAVE_atomic_compare_and_swapdi; + + if (flag_profile_update == PROFILE_UPDATE_ATOMIC + && !can_support_atomic) { - bool can_support = false; - unsigned HOST_WIDE_INT gcov_type_size - = tree_to_uhwi (TYPE_SIZE_UNIT (get_gcov_type ())); - if (gcov_type_size == 4) - can_support - = HAVE_sync_compare_and_swapsi || HAVE_atomic_compare_and_swapsi; - else if (gcov_type_size == 8) - can_support - = HAVE_sync_compare_and_swapdi || HAVE_atomic_compare_and_swapdi; - - if (!can_support) - { - warning (0, "target does not support atomic profile update, " - "single mode is selected"); - flag_profile_update = PROFILE_UPDATE_SINGLE; - } + warning (0, "target does not support atomic profile update, " + "single mode is selected"); + flag_profile_update = PROFILE_UPDATE_SINGLE; } + else if (flag_profile_update == PROFILE_UPDATE_MAYBE_ATOMIC) + flag_profile_update = can_support_atomic + ? PROFILE_UPDATE_ATOMIC : PROFILE_UPDATE_SINGLE; /* This is a small-ipa pass that gets called only once, from cgraphunit.c:ipa_passes(). */ -- 2.9.2