From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-f42.google.com (mail-qv1-f42.google.com [209.85.219.42]) by sourceware.org (Postfix) with ESMTPS id 0409B3858D28 for ; Tue, 11 Oct 2022 20:46:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0409B3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gcc.gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-qv1-f42.google.com with SMTP id l19so9728135qvu.4 for ; Tue, 11 Oct 2022 13:46:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=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=6g353RwAPk0+e2wGl5uVXDF5MIU1Y2OMNL0kWCVTxk0=; b=FqhAzD3i61hY+bsPQvpAkrbG6p+cJk1x1GPlbKGnt7s765aHFi2+sEnY0U0NO5PIcw LR/UyQJ5K9YzaDR/SOnujI4qdgEonmZkIo9h1BLEXZ2eQlsrow2OvLKTnVSrmcLg/IJ/ 6eQAFazzoiImpYER4HJqv+RdgBtYGRi6kPzpkOsg0Z4t1y9oi6RoJ6Hk1quEQVmsl0zc 0htFDJbTqmy9MkZo/DNO3YAyEiz8VKIM6yFghOmlD4lg1V2nhclcJiTtmQtC/dVcTpFm cT6EPOP8Hr9wFHoktH0zcfWT7F1Du97LLBX0DksoXMogpUGOif84zPEn+mvuQPZWW7wY nYMQ== X-Gm-Message-State: ACrzQf2tSYARPPa/zu3/suENZaHgLFUvpeFMZ7dUt80pZq6TzEZ7V8Hc 1bkPuiWi/Rg8XJiVkFeWp8+PvFqJakwuxA== X-Google-Smtp-Source: AMsMyM6Rn1Oy8JQZINiIUFwKaiuMn/jeu0JT8Bq530lahSoiMCLDR08+Y3bfbKzT//8X87vADDcwtw== X-Received: by 2002:a05:6214:c2a:b0:474:2411:b482 with SMTP id a10-20020a0562140c2a00b004742411b482mr20670609qvd.128.1665521172296; Tue, 11 Oct 2022 13:46:12 -0700 (PDT) Received: from mail-yb1-f176.google.com (mail-yb1-f176.google.com. [209.85.219.176]) by smtp.gmail.com with ESMTPSA id x18-20020a05620a259200b006ced196a73fsm13768774qko.135.2022.10.11.13.46.11 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 11 Oct 2022 13:46:11 -0700 (PDT) Received: by mail-yb1-f176.google.com with SMTP id j7so17869571ybb.8 for ; Tue, 11 Oct 2022 13:46:11 -0700 (PDT) X-Received: by 2002:a25:e6d4:0:b0:6c0:5b31:d865 with SMTP id d203-20020a25e6d4000000b006c05b31d865mr16555673ybh.85.1665521171507; Tue, 11 Oct 2022 13:46:11 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: =?UTF-8?Q?Christoph_M=C3=BCllner?= Date: Tue, 11 Oct 2022 22:46:00 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 00/10] [RISC-V] Atomics improvements [PR100265/PR100266] To: Palmer Dabbelt Cc: Vineet Gupta , andrea@rivosinc.com, gcc-patches@gcc.gnu.org, kito.cheng@sifive.com, gnu-toolchain@rivosinc.com Content-Type: multipart/alternative; boundary="000000000000cb349105eac85dec" X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,KAM_DMARC_STATUS,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --000000000000cb349105eac85dec Content-Type: text/plain; charset="UTF-8" On Tue, Oct 11, 2022 at 9:31 PM Palmer Dabbelt wrote: > On Tue, 11 Oct 2022 12:06:27 PDT (-0700), Vineet Gupta wrote: > > Hi Christoph, Kito, > > > > On 5/5/21 12:36, Christoph Muellner via Gcc-patches wrote: > >> This series provides a cleanup of the current atomics implementation > >> of RISC-V: > >> > >> * PR100265: Use proper fences for atomic load/store > >> * PR100266: Provide programmatic implementation of CAS > >> > >> As both are very related, I merged the patches into one series. > >> > >> The first patch could be squashed into the following patches, > >> but I found it easier to understand the chances with it in place. > >> > >> The series has been tested as follows: > >> * Building and testing a multilib RV32/64 toolchain > >> (bootstrapped with riscv-gnu-toolchain repo) > >> * Manual review of generated sequences for GCC's atomic builtins API > >> > >> The programmatic re-implementation of CAS benefits from a REE > improvement > >> (see PR100264): > >> https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568680.html > >> If this patch is not in place, then an additional extension instruction > >> is emitted after the SC.W (in case of RV64 and CAS for uint32_t). > >> > >> Further, the new CAS code requires cbranch INSN helpers to be present: > >> https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569689.html > > > > I was wondering is this patchset is blocked on some technical grounds. > > There's a v3 (though I can't find all of it, so not quite sure what > happened), but IIUC that still has the same fundamental problems that > all these have had: changing over to the new fence model may by an ABI > break and the split CAS implementation doesn't ensure eventual success > (see Jim's comments). Not sure if there's other comments floating > around, though, that's just what I remember. > v3 was sent on May 27, 2022, when I rebased this on an internal tree: https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595712.html I dropped the CAS patch in v3 (issue: stack spilling under extreme register pressure instead of erroring out) as I thought that this was the blocker for the series. I just learned a few weeks ago, when I asked Palmer at the GNU Cauldron about this series, that the ABI break is the blocker. My initial understanding was that fixing something broken cannot be an ABI break. And that the mismatch of the implementation in 2021 and the recommended mappings in the ratified specification from 2019 is something that is broken. I still don't know the background here, but I guess this assumption is incorrect from a historical point of view. However, I'm sure that I am not the only one that assumes the mappings in the specification to be implemented in compilers and tools. Therefore I still consider the implementation of the RISC-V atomics in GCC as broken (at least w.r.t. user expectation from people that lack the historical background and just read the RISC-V specification). > > +Andrea, in case he has time to look at the memory model / ABI issues. > > We'd still need to sort out the CAS issues, though, and it's not > abundantly clear it's worth the work: we're essentailly constrained to > just emitting those fixed CAS sequences due to the eventual success > rules, so it's not clear what the benefit of splitting those up is. > With WRS there are some routines we might want to generate code for > (cond_read_acquire() in Linux, for example) but we'd really need to dig > into those to see if it's even sane/fast. > > There's another patch set to fix the lack of inline atomic routines > without breaking stuff, there were some minor comments from Kito and > IIRC I had some test failures that I needed to chase down as well. > That's a much safer fix in the short term, we'll need to deal with this > eventually but at least we can stop the libatomic issues for the distro > folks. > I expect that the pressure for a proper fix upstream (instead of a backward compatible compromise) will increase over time (once people start building big iron based on RISC-V and start hunting performance bottlenecks in multithreaded workloads to be competitive). What could be done to get some relief is to enable the new atomics ABI by a command line switch and promote its use. And at one point in the future (if there are enough fixes to justify a break) the new ABI can be enabled by default with a new flag to enable the old ABI. > > > > > Thx, > > -Vineet > > > >> Changes for v2: > >> * Guard LL/SC sequence by compiler barriers ("blockage") > >> (suggested by Andrew Waterman) > >> * Changed commit message for AMOSWAP->STORE change > >> (suggested by Andrew Waterman) > >> * Extracted cbranch4 patch from patchset (suggested by Kito Cheng) > >> * Introduce predicate riscv_sync_memory_operand (suggested by Jim > Wilson) > >> * Fix small code style issue > >> > >> Christoph Muellner (10): > >> RISC-V: Simplify memory model code [PR 100265] > >> RISC-V: Emit proper memory ordering suffixes for AMOs [PR 100265] > >> RISC-V: Eliminate %F specifier from riscv_print_operand() [PR 100265] > >> RISC-V: Use STORE instead of AMOSWAP for atomic stores [PR 100265] > >> RISC-V: Emit fences according to chosen memory model [PR 100265] > >> RISC-V: Implement atomic_{load,store} [PR 100265] > >> RISC-V: Model INSNs for LR and SC [PR 100266] > >> RISC-V: Add s.ext-consuming INSNs for LR and SC [PR 100266] > >> RISC-V: Provide programmatic implementation of CAS [PR 100266] > >> RISC-V: Introduce predicate "riscv_sync_memory_operand" [PR 100266] > >> > >> gcc/config/riscv/riscv-protos.h | 1 + > >> gcc/config/riscv/riscv.c | 136 +++++++++++++------- > >> gcc/config/riscv/sync.md | 216 +++++++++++++++++++++----------- > >> 3 files changed, 235 insertions(+), 118 deletions(-) > >> > --000000000000cb349105eac85dec--