From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by sourceware.org (Postfix) with ESMTPS id 348CA3858D28 for ; Tue, 11 Oct 2022 19:31:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 348CA3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dabbelt.com Received: by mail-pj1-x102d.google.com with SMTP id q10-20020a17090a304a00b0020b1d5f6975so13095040pjl.0 for ; Tue, 11 Oct 2022 12:31:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=l0rMQCAoBzgc+n0ZVExcy94PXfeBEECeXJtbDwd6inc=; b=4HQ56UHpm33w8EO105yGzNpqtA4hjTAaE3BSSoeBemBvV2WDhoHbhOFzTxDRqgn2k7 teYH8Yi350Cj0VQHojvp4KcLAWhbRRsDvYyaDV4vd6IHVTbL4P+tndTOX8DMSK2i5PEr G63J2fGhD3u8NPgMaulW2GGeEdeKbQefy/nMgFMBO61Qax3OhwhyGuh7Th8/xrcoO9jJ bBM8nJa6FIL5vz13q0wkrNPTyZtgPiiUL2JdokE94dXhjiN6PrQ9RRNluwxTmNopgjPp JWOdSG15ggI7VPDCt9Grq/6INw17X0yjEt01ArUFsGRfr+feoT+gHfSiaDfCDXrLHB0m eLag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=l0rMQCAoBzgc+n0ZVExcy94PXfeBEECeXJtbDwd6inc=; b=TLT+41//Vm4/5He4BTixzAOlIMHGrmlcV34wWaqnmJviEvA0khcGzoU+nunrtzzP7P sipf6gJGHRERMc34hQsctri6M3KEjM8Esnf+sAc/QiaTCvfCm6kcfeVn3GpyENHZjsUp pbG1Ucgfos0kdSc5Fohonfv2Z1CvesRnSnSeP+j1JJ89vrvL75rzkkhRPjw6soba1WPh t3bLYjqyy7rw0ZT4Bm074Y+//Ee3x0A7NHsMtdMy2pjtFw1G5j+G+MJccnVIIZbgExrf FmklcMxlY3XPjIUznk1xWzwyGlQm1Xi5QnN0OqYEq8xiB0YTH8fA4t56OL3aao/PTEvO KBFQ== X-Gm-Message-State: ACrzQf2sKiIrHTAwKepk9w9C8qNoViW5Ey3KrA76TYXvIsqHrYC9g4oB 6shyh6bRxvyUbpsEZPz2v7DTZQ== X-Google-Smtp-Source: AMsMyM6Lm8PSCvf/OYE16q+7f+NXbOswowPIDB5IfIvOY+R/wUFx52DC8/Hy7xxKaWiD/rVeMtyvsw== X-Received: by 2002:a17:902:f791:b0:17c:c1dd:a3b5 with SMTP id q17-20020a170902f79100b0017cc1dda3b5mr25529227pln.141.1665516674991; Tue, 11 Oct 2022 12:31:14 -0700 (PDT) Received: from localhost ([50.221.140.188]) by smtp.gmail.com with ESMTPSA id x9-20020a170902a38900b0017f9db0236asm9005035pla.82.2022.10.11.12.31.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Oct 2022 12:31:14 -0700 (PDT) Date: Tue, 11 Oct 2022 12:31:14 -0700 (PDT) X-Google-Original-Date: Tue, 11 Oct 2022 12:30:46 PDT (-0700) Subject: Re: [PATCH v2 00/10] [RISC-V] Atomics improvements [PR100265/PR100266] In-Reply-To: CC: cmuellner@gcc.gnu.org, gcc-patches@gcc.gnu.org, kito.cheng@sifive.com, gnu-toolchain@rivosinc.com From: Palmer Dabbelt To: Vineet Gupta , andrea@rivosinc.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,KAM_SHORT,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 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. +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. > > 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(-) >>