From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com [IPv6:2a00:1450:4864:20::12d]) by sourceware.org (Postfix) with ESMTPS id 6592938308EF for ; Fri, 16 Dec 2022 09:47:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6592938308EF Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x12d.google.com with SMTP id bf43so2656125lfb.6 for ; Fri, 16 Dec 2022 01:47:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=pOixscq+Ntl1ov6s5P496cj4TxWAFcbgSEHrIqo0vgU=; b=LlcKzQqSIQ8X3t2JHv+Zcp6ArYAtKMf2X3W4y+EQNoQe+DunXPJB4EjmOVtGTCnZOO Pi/5Jndf+S2i/EwGjY0OlVgS8n12Bxj5XTUZtRTjfg3j77Fqv8iRNVxR2J0POC23J0Nd ilu5m492PKF8P9HQ/0LNz7KVytmQU/UtX4Gwrv/YshLukT/bJBI4o27rBKAmddNxTnPD bUWvGPy6TM57gRGAhk9NfqvVgw9CYJHEiy8XdSdcZ0rlJ6ek0we9jb6jVcK5iZLN8OE2 Q0k43Kk+etzxW5Y5WH58nniaKM3PXGGZov/VzgxoRZnxU8sodpiA7RU1+fP3dLIMUlAo 325A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=pOixscq+Ntl1ov6s5P496cj4TxWAFcbgSEHrIqo0vgU=; b=VY64L2TwuGcomKH9NRkquXD/HPqP4CMmz85fSBgGmVfuG00xm6x/Q/t7AJg26hvybz ilokMt8Y+MThX/Te/6BEL6KdDdg8jv+JUvlo5aZT0FqDSn2n8AcUxu0eLEKUE4Mre3xQ hM2JYlxKQ0mVC5Vx4oDvFZ/W6dkCJ5eaMzRkOjXYXPHEMc1ivUzNfQ4m2ACESpMgOSwy Ce5jQB16DNmyxWVMqKU+V6t9lCSUvoNtuD/ZTHD1X/efXxmacvQv87s92EOOg6UUeTGu 46cfqBboAgCO/6p85ipebXgu9pbTM2coTBaMkIQ/2IyYbyy4g9M7GbkG6d8vealejH/n /W+w== X-Gm-Message-State: ANoB5pkTBEDs3+bgP/pCBUardnKuQgjfTAkCXoML7RJ/eeNiEcELGzRr M1n7di8ot85HcsSdl0o0wDyPlXqkL3SGOWNpOkHP6DUp X-Google-Smtp-Source: AA0mqf6ZwFSdRP/OGsIG1KnGmaUB/N2vL2WO0oBSkbGWKmmeaYIwb4qWhn2ZhhHOg8ldrlrw64d/+46Y8YaMkXWd+os= X-Received: by 2002:ac2:4c42:0:b0:4a5:bf09:a700 with SMTP id o2-20020ac24c42000000b004a5bf09a700mr26241570lfk.656.1671184034384; Fri, 16 Dec 2022 01:47:14 -0800 (PST) MIME-Version: 1.0 References: <20221209135609.55159-1-sebastian.huber@embedded-brains.de> <2d6f0cf1-f979-b3a6-6daa-81fc0882a1d7@embedded-brains.de> In-Reply-To: <2d6f0cf1-f979-b3a6-6daa-81fc0882a1d7@embedded-brains.de> From: Richard Biener Date: Fri, 16 Dec 2022 10:47:01 +0100 Message-ID: Subject: Re: [PATCH] gcov: Fix -fprofile-update=atomic To: Sebastian Huber Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Dec 15, 2022 at 9:34 AM Sebastian Huber wrote: > > On 13/12/2022 15:30, Richard Biener wrote: > > On Fri, Dec 9, 2022 at 2:56 PM Sebastian Huber > > wrote: > >> The code coverage support uses counters to determine which edges in th= e control > >> flow graph were executed. If a counter overflows, then the code cover= age > >> information is invalid. Therefore the counter type should be a 64-bit= integer. > >> In multithreaded applications, it is important that the counter increm= ents are > >> atomic. This is not the case by default. The user can enable atomic = counter > >> increments through the -fprofile-update=3Datomic and > >> -fprofile-update=3Dprefer-atomic options. > >> > >> If the hardware supports 64-bit atomic operations, then everything is = fine. If > >> not and -fprofile-update=3Dprefer-atomic was chosen by the user, then = non-atomic > >> counter increments will be used. However, if the hardware does not su= pport the > >> required atomic operations and -fprofile-atomic=3Dupdate was chosen by= the user, > >> then a warning was issued and as a forced fall-back to non-atomic oper= ations > >> was done. This is probably not what a user wants. There is still har= dware on > >> the market which does not have atomic operations and is used for multi= threaded > >> applications. A user which selects -fprofile-update=3Datomic wants co= nsistent > >> code coverage data and not random data. > >> > >> This patch removes the fall-back to non-atomic operations for > >> -fprofile-update=3Datomic. If atomic operations in hardware are not a= vailable, > >> then a library call to libatomic is emitted. To mitigate potential pe= rformance > >> issues an optimization for systems which only support 32-bit atomic op= erations > >> is provided. Here, the edge counter increments are done like this: > >> > >> low =3D __atomic_add_fetch_4 (&counter.low, 1, MEMMODEL_RELAXED); > >> high_inc =3D low =3D=3D 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 b= yte > > gcov_type is used. But you do not seem to handle the case where > > neither SImode nor DImode atomic operations are available? Can we inst= ead > > do > > > > if (gcov_type_size =3D=3D 4) > > can_support_atomic4 > > =3D HAVE_sync_compare_and_swapsi || HAVE_atomic_compare_and_swap= si; > > else if (gcov_type_size =3D=3D 8) > > can_support_atomic8 > > =3D HAVE_sync_compare_and_swapdi || HAVE_atomic_compare_and_swap= di; > > > > if (flag_profile_update =3D=3D 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 =3D PROFILE_UPDATE_SINGLE; > > } > > > > thus retain the diagnostic & fallback for this case? > > No, if you select -fprofile-update=3Datomic, then the updates shall be > atomic from my point of view. If a fallback is acceptable, then you can > use -fprofile-update=3Dprefer-atomic. Using the fallback in > -fprofile-update=3Datomic 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. Ah, OK. So the fallback there is libatomic calls as well then? Note not all targets support libatomic, for those the failure mode is likely a link error (which might be fine, but we eventually might want to amend documentation to indicate this failure mode). > > The existing > > code also suggests > > there might be targets with HImode or TImode counters? > > Sorry, I don't know. can you conditionalize the split_atomic_increment init on gcov_type_size =3D=3D 8 please? The patch is missing adjusments to the -fprofile-update documentation. I think the prefer-atomic vs. atomic needs more explanation with this change. otherwise the patch looks OK to me. Thanks, Richard. > > > > 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] =3D= =3D > 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=3Datomic > > 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=3Datomic -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=3Datomic -march=3Drv64= g > -mabi=3Dlp64 > > 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=3Datomic -march=3Drv32= id > > 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=3Dprefer-atomic > -march=3Drv32id > > 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.huber@embedded-brains.de > phone: +49-89-18 94 741 - 16 > fax: +49-89-18 94 741 - 08 > > Registergericht: Amtsgericht M=C3=BCnchen > Registernummer: HRB 157899 > Vertretungsberechtigte Gesch=C3=A4ftsf=C3=BChrer: Peter Rasmussen, Thomas= D=C3=B6rfler > Unsere Datenschutzerkl=C3=A4rung finden Sie hier: > https://embedded-brains.de/datenschutzerklaerung/