From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1035.google.com (mail-pj1-x1035.google.com [IPv6:2607:f8b0:4864:20::1035]) by sourceware.org (Postfix) with ESMTPS id 3878D3858404 for ; Wed, 12 Oct 2022 00:15:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3878D3858404 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-x1035.google.com with SMTP id h12so8134847pjk.0 for ; Tue, 11 Oct 2022 17:15:45 -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=Vn1fBbjNy/GKt60ysk2vJ3iHl0vESHUd8H334a/RyLI=; b=Bth2nROhiFWrObpbz86dflX7VlIMZXeS4GbB7Ixsb0RfcqSFxyyPsYbd2veBCx0kc3 cyiC1BssVgtkgXwkqz3zofllYMM9csled8JVjpkXgwE628oZU0adj5jaBUtyxK1g/KDh pPbNuvE2uZaHynN8OxueU/5M4qx/o0QAs9GdY3Pj6LDVYqucEhZAqQoG9rn3a8GlVJbn esJVXXEB4SWQpiXSIidMJg+OV4BrhDs2SPBvSx5OG7Teljxc6KpDFiLx7Ov47Fg7QJ1G WQ26NPCREmhQLHq9taxtAlRfOPSWYlEcFaai3Vz/K1UQtvxfm6tWaKseLs9xmKJ0k3RJ rsUg== 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=Vn1fBbjNy/GKt60ysk2vJ3iHl0vESHUd8H334a/RyLI=; b=6FUtAy9x0t08xkfXVAHWq1vJEeohO2XXPs51NWjTalceg7+RNEluLad9qwc1pvjvOz TlyCKXfQq0qiXi/VYrEdi8biiguTE7OXD+DLry3HqiflI5fGmZnOdLeB9GABtowRjtVb knJUvJZKzoYoMtRTjyzSuviJzoBdqgiqMYze/N32Vea4nc+Pp4+lKlh2GqFCbaRXuA2i SRa/kFCJqxiNON5fqBUSa0X9Xl8IfVaYssAwt/O/FUTAOCB0A5PTQmpjO4vqSkE+C6yB 1r8943R2TD2yhZzy55x0Nzm1jT89hT2UsQOUckRnxmDFxlgqBXFjLvYcWnmqEgT69aon vdpQ== X-Gm-Message-State: ACrzQf2QWgcOwKY2TX23IUDMoi9EPaq1bPDUf2JJ6ju9bA3lEbi98vmJ X/QKv2ETYGj5t6bR9vYoGrc3UA== X-Google-Smtp-Source: AMsMyM411J1krdGGmC3yaGUj2vTicmZi9hQ+HJzKb6JqVJFqDqZ9lWhp/Nqz1wBIPNssgTXX48Dh2A== X-Received: by 2002:a17:902:e747:b0:17f:86f9:3712 with SMTP id p7-20020a170902e74700b0017f86f93712mr27520138plf.128.1665533743930; Tue, 11 Oct 2022 17:15:43 -0700 (PDT) Received: from localhost ([50.221.140.188]) by smtp.gmail.com with ESMTPSA id o1-20020a170902bcc100b00177f32b1a32sm9136083pls.271.2022.10.11.17.15.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Oct 2022 17:15:43 -0700 (PDT) Date: Tue, 11 Oct 2022 17:15:43 -0700 (PDT) X-Google-Original-Date: Tue, 11 Oct 2022 17:15:41 PDT (-0700) Subject: Re: [PATCH v2 00/10] [RISC-V] Atomics improvements [PR100265/PR100266] In-Reply-To: <3862a109-8083-7a36-3d85-8f9e5e10627c@rivosinc.com> CC: cmuellner@gcc.gnu.org, Andrea Parri , gcc-patches@gcc.gnu.org, kito.cheng@sifive.com, gnu-toolchain@rivosinc.com From: Palmer Dabbelt To: Vineet Gupta , jeffreyalaw@gmail.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=-5.0 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 16:31:25 PDT (-0700), Vineet Gupta wrote: > > > On 10/11/22 13:46, Christoph Müllner wrote: >> 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. > > Yeah I was confused about the ABI aspect as I didn't see any mention of > that in the public reviews of v1 and v2. Sorry, I thought we'd talked about it somewhere but it must have just been in meetings and such. Patrick was writing a similar patch set around the same time so it probably just got tied up in that, we ended up reducing it to just the strong CAS inline stuff because we couldn't sort out the correctness of the rest of it. >> 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. We agreed that we wouldn't break binaries back when we submitted the port. The ISA has changed many times since then, including adding the recommended mappings, but those binaries exist and we can't just silently break things for users. >> 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). You can't just read one of those RISC-V PDFs and assume that implementations that match those words will function correctly. Those words regularly change in ways where reasonable readers would end up with incompatible implementations due to those differences. That's why we're so explicit about versions and such these days, we're just getting burned by these old mappings because they're from back when we though the RISC-V definition of compatibility was going to match the more common one and we didn't build in fallbacks. >> +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. > > Indeed we are stuck with inefficiencies with status quo. The new abi > option sounds like a reasonable plan going fwd. I don't think we're just stuck with the status quo, we really just need to go through the mappings and figure out which can be made both fast and ABI-compatible. Then we can fix those and see where we stand, maybe it's good enough or maybe we need to introduce some sort of compatibility break to make things faster (and/or compatible with LLVM, where I suspect we're broken right now). If we do need a break then I think it's probably possible to do it in phases, where we have a middle-ground compatibility mode that works for both the old and new mappings so distros can gradually move over as they rebuild packages. Issues like the libstdc++ shared_ptr/mutex fallback don't map well to that, though. There's also some stuff like the IO fence bits that we can probably just add an argument for now, those were likely just a bad idea at the time and should be safe to turn off for the vast majority of users (though those are more of an API break). > Also my understand is that while the considerations are ABI centric, the > option to faciliate this need not be tied to canonical -mabi=lp32, lp64d > etc. It might just be a toggle as -matomic=legacy,2019 etc (this is not > suggestive just indicative). Otherwise there's another level of blowup > in multilib testing etc. The psABI doesn't mention memory ordering at all. IIUC that's a pretty standard hole in psABI documents, but it means we're in a grey area here. +Jeff, who was offering to help when the threads got crossed. I'd punted on a lot of this in the hope Andrea could help out, as I'm not really a memory model guy and this is pretty far down the rabbit hole. Happy to have the help if you're offering, though, as what's there is likely a pretty big performance issue for anyone with a reasonable memory system. > -Vineet