On 13/12/2022 15:30, Richard Biener wrote:
On Fri, Dec 9, 2022 at 2:56 PM Sebastian Huber
<sebastian.hu...@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?
No, if you select -fprofile-update=atomic, then the updates shall be
atomic from my point of view. If a fallback is acceptable, then you can
use -fprofile-update=prefer-atomic. Using the fallback in
-fprofile-update=atomic is a bug which prevents the use of gcov for
multi-threaded applications on the lower end targets which do not have
atomic operations in hardware.
The existing
code also suggests
there might be targets with HImode or TImode counters?
Sorry, I don't know.
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?
If atomic operations are not available in hardware, then with this patch
calls to libatomic are emitted. In gen_time_profiler() this is not an
issue at all, since the atomic increment is only done if counters[0] ==
0 (if I read the code correctly).
For example for
void f(void) {}
we get on riscv:
gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic
lui a4,%hi(__gcov0.f)
li a3,1
addi a4,a4,%lo(__gcov0.f)
amoadd.w a5,a3,0(a4)
lui a4,%hi(__gcov0.f+4)
addi a5,a5,1
seqz a5,a5
addi a4,a4,%lo(__gcov0.f+4)
amoadd.w zero,a5,0(a4)
ret
gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic -mbig-endian
lui a4,%hi(__gcov0.f+4)
li a3,1
addi a4,a4,%lo(__gcov0.f+4)
amoadd.w a5,a3,0(a4)
lui a4,%hi(__gcov0.f)
addi a5,a5,1
seqz a5,a5
addi a4,a4,%lo(__gcov0.f)
amoadd.w zero,a5,0(a4)
ret
gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic -march=rv64g
-mabi=lp64
lui a5,%hi(__gcov0.f)
li a4,1
addi a5,a5,%lo(__gcov0.f)
amoadd.d zero,a4,0(a5)
ret
gcc --coverage -O2 -S -o - test.c -fprofile-update=atomic -march=rv32id
lui a0,%hi(__gcov0.f)
li a3,0
li a1,1
li a2,0
addi a0,a0,%lo(__gcov0.f)
tail __atomic_fetch_add_8
gcc --coverage -O2 -S -o - test.c -fprofile-update=prefer-atomic
-march=rv32id
lui a4,%hi(__gcov0.f)
lw a5,%lo(__gcov0.f)(a4)
lw a2,%lo(__gcov0.f+4)(a4)
addi a3,a5,1
sltu a5,a3,a5
add a5,a5,a2
sw a3,%lo(__gcov0.f)(a4)
sw a5,%lo(__gcov0.f+4)(a4)
ret
--
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.hu...@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
