On 09/06/2016 02:38 PM, David Edelsohn wrote:
> On Tue, Sep 6, 2016 at 8:14 AM, Nathan Sidwell <nat...@acm.org> wrote:
>> On 09/06/16 06:57, David Edelsohn wrote:
>>
>>> What about Jakub's comment in the PR?
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77378#c6
>>
>>
>> This needs addressing. Can you clarify PPC behaviour, because I may have
>> misunderstood:
>>
>> 1) PPC currently has 64 bit counters -- but cannot support 64bit atomic ops
>> in 32 bit mode.
>>
>> 2) PPC currently has 32 bit counters anyway.
>
> The rs6000 port ABIs implement 64 bit "long long" type. The current
> code uses LONG_LONG_TYPE_SIZE for the counters. I assume that most
> ports don't support native 64 bit atomic operations in 32 bit ABI --
> PowerPC does not.
>
> The previous code allowed gcov type to be overridden, but I don't
> think that it was 32 bit on most targets.
>
>>
>> I had interpreted the comment to be implying #2, but now I'm not so sure.
>>
>>> The proposed patch seems wrong or at least incomplete. The recent
>>> change is imposing a 64 bit DImode counter when producing 32 bit code.
>>> PowerPC does support 64 bit atomic operations in 32 bit mode.
>>
>>
>> I'm presuming you've missed a 'NOT' in that sentence.
>
> Yes, I omitted a "NOT".
>
> PowerPC64 has 64 bit atomics, but PowerPC32 subset only provides 32
> bit atomics in the ISA.
>
> If the counters always should be 64 bit, then a poor-man's 64 bit
> atomic operation proposed by Jakub seems like a better solution.
>
> Thanks, David
Hi David.
I sent the previous email before I read the Jakub's comment.
I'm attaching simplified patch (based of the comment), which works for i386
target. I'm testing that on on m68k target.
I prefer to have a single type for one config, same what Nathan suggested.
I like the idea of poor-man's atomics, I can make an incremental patch.
Martin
>
>>
>>> Was there a design decision that profile counters always should be 64
>>> bits? Either 32 bit targets won't support multi-threaded profiling or
>>> 32 bit targets can overflow the counter sooner.
>>
>>
>>
>>> Which is worse?
>>> Which is more likely?
>>
>>
>> My initial thought is that it is probably awkward to support 2 different
>> sized counter types in the 'same' config. I.e. 64-bit single-threaded
>> counters and 32-bit threaded counters.
>>
>> nathan
>From 44289abf2e3ecfb7e17c6f204b280af06bf20b0e Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Tue, 6 Sep 2016 14:35:52 +0200
Subject: [PATCH] [PATCH] Detect whether target can use -fprofile-update=atomic
libgcc/ChangeLog:
2016-09-06 Martin Liska <mli...@suse.cz>
* libgcov-profiler.c: Use __GCC_HAVE_SYNC_COMPARE_AND_SWAP_{4,8} to
conditionaly enable/disable *_atomic functions.
gcc/ChangeLog:
2016-09-06 Martin Liska <mli...@suse.cz>
* tree-profile.c (tree_profiling): Detect whether target can use
-fprofile-update=atomic.
gcc/testsuite/ChangeLog:
2016-09-06 Martin Liska <mli...@suse.cz>
* gcc.dg/profile-update-warning.c: New test.
---
gcc/testsuite/gcc.dg/profile-update-warning.c | 7 +++++++
gcc/tree-profile.c | 17 +++++++++++++++++
libgcc/libgcov-profiler.c | 24 +++++++++++++++++++-----
3 files changed, 43 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/profile-update-warning.c
diff --git a/gcc/testsuite/gcc.dg/profile-update-warning.c b/gcc/testsuite/gcc.dg/profile-update-warning.c
new file mode 100644
index 0000000..0614fad
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/profile-update-warning.c
@@ -0,0 +1,7 @@
+/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-options "-fprofile-update=atomic -fprofile-generate -march=i386 -m32" } */
+
+int main(int argc, char *argv[])
+{
+ return 0;
+} /* { dg-warning "target does not support atomic profile update, single mode is selected" } */
diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c
index 622869e..8ce35be 100644
--- a/gcc/tree-profile.c
+++ b/gcc/tree-profile.c
@@ -528,6 +528,13 @@ gimple_gen_ior_profiler (histogram_value value, unsigned tag, unsigned base)
gsi_insert_before (&gsi, call, GSI_NEW_STMT);
}
+#ifndef HAVE_sync_compare_and_swapsi
+#define HAVE_sync_compare_and_swapsi 0
+#endif
+#ifndef HAVE_atomic_compare_and_swapsi
+#define HAVE_atomic_compare_and_swapsi 0
+#endif
+
/* Profile all functions in the callgraph. */
static unsigned int
@@ -535,6 +542,16 @@ tree_profiling (void)
{
struct cgraph_node *node;
+ /* Verify whether we can utilize atomic update operations. */
+ if (flag_profile_update == PROFILE_UPDATE_ATOMIC
+ && !HAVE_sync_compare_and_swapsi
+ && !HAVE_atomic_compare_and_swapsi)
+ {
+ warning (0, "target does not support atomic profile update, "
+ "single mode is selected");
+ flag_profile_update = PROFILE_UPDATE_SINGLE;
+ }
+
/* This is a small-ipa pass that gets called only once, from
cgraphunit.c:ipa_passes(). */
gcc_assert (symtab->state == IPA_SSA);
diff --git a/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c
index 70a821d..887041f 100644
--- a/libgcc/libgcov-profiler.c
+++ b/libgcc/libgcov-profiler.c
@@ -24,8 +24,20 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
<http://www.gnu.org/licenses/>. */
#include "libgcov.h"
+#include "auto-target.h"
#if !defined(inhibit_libc)
+/* Detect whether target can support atomic update of profilers. */
+#if LONG_LONG_TYPE_SIZE <= 32 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+#define GCOV_SUPPORTS_ATOMIC 1
+#else
+#if LONG_LONG_TYPE_SIZE > 32 && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
+#define GCOV_SUPPORTS_ATOMIC 1
+#else
+#define GCOV_SUPPORTS_ATOMIC 0
+#endif
+#endif
+
#ifdef L_gcov_interval_profiler
/* If VALUE is in interval <START, START + STEPS - 1>, then increases the
corresponding counter in COUNTERS. If the VALUE is above or below
@@ -46,7 +58,7 @@ __gcov_interval_profiler (gcov_type *counters, gcov_type value,
}
#endif
-#ifdef L_gcov_interval_profiler_atomic
+#if defined(L_gcov_interval_profiler_atomic) && GCOV_SUPPORTS_ATOMIC
/* If VALUE is in interval <START, START + STEPS - 1>, then increases the
corresponding counter in COUNTERS. If the VALUE is above or below
the interval, COUNTERS[STEPS] or COUNTERS[STEPS + 1] is increased
@@ -80,7 +92,7 @@ __gcov_pow2_profiler (gcov_type *counters, gcov_type value)
}
#endif
-#ifdef L_gcov_pow2_profiler_atomic
+#if defined(L_gcov_pow2_profiler_atomic) && GCOV_SUPPORTS_ATOMIC
/* If VALUE is a power of two, COUNTERS[1] is incremented. Otherwise
COUNTERS[0] is incremented. Function is thread-safe. */
@@ -134,7 +146,7 @@ __gcov_one_value_profiler (gcov_type *counters, gcov_type value)
}
#endif
-#ifdef L_gcov_one_value_profiler_atomic
+#if defined(L_gcov_one_value_profiler_atomic) && GCOV_SUPPORTS_ATOMIC
/* Update one value profilers (COUNTERS) for a given VALUE.
@@ -342,6 +354,7 @@ __gcov_time_profiler (gcov_type* counters)
counters[0] = ++function_counter;
}
+#if GCOV_SUPPORTS_ATOMIC
/* Sets corresponding COUNTERS if there is no value.
Function is thread-safe. */
@@ -352,6 +365,7 @@ __gcov_time_profiler_atomic (gcov_type* counters)
counters[0] = __atomic_add_fetch (&function_counter, 1, MEMMODEL_RELAXED);
}
#endif
+#endif
#ifdef L_gcov_average_profiler
@@ -366,7 +380,7 @@ __gcov_average_profiler (gcov_type *counters, gcov_type value)
}
#endif
-#ifdef L_gcov_average_profiler_atomic
+#if defined(L_gcov_average_profiler_atomic) && GCOV_SUPPORTS_ATOMIC
/* Increase corresponding COUNTER by VALUE. FIXME: Perhaps we want
to saturate up. Function is thread-safe. */
@@ -388,7 +402,7 @@ __gcov_ior_profiler (gcov_type *counters, gcov_type value)
}
#endif
-#ifdef L_gcov_ior_profiler_atomic
+#if defined(L_gcov_ior_profiler_atomic) && GCOV_SUPPORTS_ATOMIC
/* Bitwise-OR VALUE into COUNTER. Function is thread-safe. */
void
--
2.9.2
