From: Richard Biener <richard.guenther@gmail.com>
To: Sebastian Huber <sebastian.huber@embedded-brains.de>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] gcov: Fix -fprofile-update=atomic
Date: Tue, 13 Dec 2022 15:30:55 +0100 [thread overview]
Message-ID: <CAFiYyc0V=5RDhF-ofOgaMcbC6UeaxC8E5j-uhrduGQpzzqUqyw@mail.gmail.com> (raw)
In-Reply-To: <20221209135609.55159-1-sebastian.huber@embedded-brains.de>
On Fri, Dec 9, 2022 at 2:56 PM Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> The code coverage support uses counters to determine which edges in the control
> flow graph were executed. If a counter overflows, then the code coverage
> information is invalid. Therefore the counter type should be a 64-bit integer.
> In multithreaded applications, it is important that the counter increments are
> atomic. This is not the case by default. The user can enable atomic counter
> increments through the -fprofile-update=atomic and
> -fprofile-update=prefer-atomic options.
>
> If the hardware supports 64-bit atomic operations, then everything is fine. If
> not and -fprofile-update=prefer-atomic was chosen by the user, then non-atomic
> counter increments will be used. However, if the hardware does not support the
> required atomic operations and -fprofile-atomic=update was chosen by the user,
> then a warning was issued and as a forced fall-back to non-atomic operations
> was done. This is probably not what a user wants. There is still hardware on
> the market which does not have atomic operations and is used for multithreaded
> applications. A user which selects -fprofile-update=atomic wants consistent
> code coverage data and not random data.
>
> This patch removes the fall-back to non-atomic operations for
> -fprofile-update=atomic. If atomic operations in hardware are not available,
> then a library call to libatomic is emitted. To mitigate potential performance
> issues an optimization for systems which only support 32-bit atomic operations
> is provided. Here, the edge counter increments are done like this:
>
> low = __atomic_add_fetch_4 (&counter.low, 1, MEMMODEL_RELAXED);
> high_inc = low == 0 ? 1 : 0;
> __atomic_add_fetch_4 (&counter.high, high_inc, MEMMODEL_RELAXED);
You check for compare_and_swapsi and the old code checks
TYPE_PRECISION (gcov_type_node) > 32 to determine whether 8 byte or 4 byte
gcov_type is used. But you do not seem to handle the case where
neither SImode nor DImode atomic operations are available? Can we instead
do
if (gcov_type_size == 4)
can_support_atomic4
= HAVE_sync_compare_and_swapsi || HAVE_atomic_compare_and_swapsi;
else if (gcov_type_size == 8)
can_support_atomic8
= HAVE_sync_compare_and_swapdi || HAVE_atomic_compare_and_swapdi;
if (flag_profile_update == PROFILE_UPDATE_ATOMIC
&& !can_support_atomic4 && !can_support_atomic8)
{
warning (0, "target does not support atomic profile update, "
"single mode is selected");
flag_profile_update = PROFILE_UPDATE_SINGLE;
}
thus retain the diagnostic & fallback for this case? The existing
code also suggests
there might be targets with HImode or TImode counters?
In another mail you mentioned that for gen_time_profiler this doesn't
work but its
code relies on flag_profile_update as well. So do we need to split the flag
somehow, or continue using the PROFILE_UPDATE_SINGLE fallback when
we are doing more than just edge profiling?
Thanks,
Richard.
> gcc/ChangeLog:
>
> * tree-profile.cc (split_atomic_increment): New.
> (gimple_gen_edge_profiler): Split the atomic edge counter increment in
> two 32-bit atomic operations if necessary.
> (tree_profiling): Remove profile update warning and fall-back. Set
> split_atomic_increment if necessary.
> ---
> gcc/tree-profile.cc | 81 +++++++++++++++++++++++++++++++++------------
> 1 file changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index 2beb49241f2..1d326dde59a 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -73,6 +73,17 @@ static GTY(()) tree ic_tuple_var;
> static GTY(()) tree ic_tuple_counters_field;
> static GTY(()) tree ic_tuple_callee_field;
>
> +/* If the user selected atomic profile counter updates
> + (-fprofile-update=atomic), then the counter updates will be done atomically.
> + Ideally, this is done through atomic operations in hardware. If the
> + hardware supports only 32-bit atomic increments and gcov_type_node is a
> + 64-bit integer type, then for the profile edge counters the increment is
> + performed through two separate 32-bit atomic increments. This case is
> + indicated by the split_atomic_increment variable begin true. If the
> + hardware does not support atomic operations at all, then a library call to
> + libatomic is emitted. */
> +static bool split_atomic_increment;
> +
> /* Do initialization work for the edge profiler. */
>
> /* Add code:
> @@ -242,30 +253,59 @@ gimple_init_gcov_profiler (void)
> void
> gimple_gen_edge_profiler (int edgeno, edge e)
> {
> - tree one;
> -
> - one = build_int_cst (gcov_type_node, 1);
> + const char *name = "PROF_edge_counter";
> + tree ref = tree_coverage_counter_ref (GCOV_COUNTER_ARCS, edgeno);
> + tree one = build_int_cst (gcov_type_node, 1);
>
> if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
> {
> - /* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */
> - tree addr = tree_coverage_counter_addr (GCOV_COUNTER_ARCS, edgeno);
> - tree f = builtin_decl_explicit (TYPE_PRECISION (gcov_type_node) > 32
> - ? BUILT_IN_ATOMIC_FETCH_ADD_8:
> - BUILT_IN_ATOMIC_FETCH_ADD_4);
> - gcall *stmt = gimple_build_call (f, 3, addr, one,
> - build_int_cst (integer_type_node,
> - MEMMODEL_RELAXED));
> - gsi_insert_on_edge (e, stmt);
> + tree addr = build_fold_addr_expr (ref);
> + tree relaxed = build_int_cst (integer_type_node, MEMMODEL_RELAXED);
> + if (!split_atomic_increment)
> + {
> + /* __atomic_fetch_add (&counter, 1, MEMMODEL_RELAXED); */
> + tree f = builtin_decl_explicit (TYPE_PRECISION (gcov_type_node) > 32
> + ? BUILT_IN_ATOMIC_FETCH_ADD_8:
> + BUILT_IN_ATOMIC_FETCH_ADD_4);
> + gcall *stmt = gimple_build_call (f, 3, addr, one, relaxed);
> + gsi_insert_on_edge (e, stmt);
> + }
> + else
> + {
> + /* low = __atomic_add_fetch_4 (addr, 1, MEMMODEL_RELAXED);
> + high_inc = low == 0 ? 1 : 0;
> + __atomic_add_fetch_4 (addr_high, high_inc, MEMMODEL_RELAXED); */
> + tree zero32 = build_zero_cst (uint32_type_node);
> + tree one32 = build_one_cst (uint32_type_node);
> + tree addr_high = make_temp_ssa_name (TREE_TYPE (addr), NULL, name);
> + gimple *stmt = gimple_build_assign (addr_high, POINTER_PLUS_EXPR,
> + addr,
> + build_int_cst (size_type_node,
> + 4));
> + gsi_insert_on_edge (e, stmt);
> + if (WORDS_BIG_ENDIAN)
> + std::swap (addr, addr_high);
> + tree f = builtin_decl_explicit (BUILT_IN_ATOMIC_ADD_FETCH_4);
> + stmt = gimple_build_call (f, 3, addr, one, relaxed);
> + tree low = make_temp_ssa_name (uint32_type_node, NULL, name);
> + gimple_call_set_lhs (stmt, low);
> + gsi_insert_on_edge (e, stmt);
> + tree is_zero = make_temp_ssa_name (boolean_type_node, NULL, name);
> + stmt = gimple_build_assign (is_zero, EQ_EXPR, low, zero32);
> + gsi_insert_on_edge (e, stmt);
> + tree high_inc = make_temp_ssa_name (uint32_type_node, NULL, name);
> + stmt = gimple_build_assign (high_inc, COND_EXPR, is_zero, one32,
> + zero32);
> + gsi_insert_on_edge (e, stmt);
> + stmt = gimple_build_call (f, 3, addr_high, high_inc, relaxed);
> + gsi_insert_on_edge (e, stmt);
> + }
> }
> else
> {
> - tree ref = tree_coverage_counter_ref (GCOV_COUNTER_ARCS, edgeno);
> - tree gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
> - NULL, "PROF_edge_counter");
> + tree gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node, NULL, name);
> gassign *stmt1 = gimple_build_assign (gcov_type_tmp_var, ref);
> - gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node,
> - NULL, "PROF_edge_counter");
> + gcov_type_tmp_var = make_temp_ssa_name (gcov_type_node, NULL, name);
> gassign *stmt2 = gimple_build_assign (gcov_type_tmp_var, PLUS_EXPR,
> gimple_assign_lhs (stmt1), one);
> gassign *stmt3 = gimple_build_assign (unshare_expr (ref),
> @@ -710,11 +750,8 @@ tree_profiling (void)
>
> if (flag_profile_update == PROFILE_UPDATE_ATOMIC
> && !can_support_atomic)
> - {
> - warning (0, "target does not support atomic profile update, "
> - "single mode is selected");
> - flag_profile_update = PROFILE_UPDATE_SINGLE;
> - }
> + split_atomic_increment = HAVE_sync_compare_and_swapsi
> + || HAVE_atomic_compare_and_swapsi;
> else if (flag_profile_update == PROFILE_UPDATE_PREFER_ATOMIC)
> flag_profile_update = can_support_atomic
> ? PROFILE_UPDATE_ATOMIC : PROFILE_UPDATE_SINGLE;
> --
> 2.35.3
>
next prev parent reply other threads:[~2022-12-13 14:31 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-09 13:56 Sebastian Huber
2022-12-13 14:30 ` Richard Biener [this message]
2022-12-15 8:34 ` Sebastian Huber
2022-12-16 9:47 ` Richard Biener
2022-12-16 10:39 ` Sebastian Huber
2022-12-16 12:09 ` Richard Biener
2022-12-16 13:01 ` Sebastian Huber
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc0V=5RDhF-ofOgaMcbC6UeaxC8E5j-uhrduGQpzzqUqyw@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=sebastian.huber@embedded-brains.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).