From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by sourceware.org (Postfix) with ESMTPS id D42BA3854162 for ; Tue, 11 Oct 2022 23:31:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D42BA3854162 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-pf1-x42f.google.com with SMTP id i3so14902607pfk.9 for ; Tue, 11 Oct 2022 16:31:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=mOtErK1fvYOWLBcEOMHjP4JSZnsLbTeSLgh7xo95WKs=; b=02BbdGUABL1kwT5MWtvCjRPGEfsfdkrtSnfLhYstosn0ag4ZUJRq2KqiqUbAPfrAvm WwTaX1LGpSfUgXpG/wny7nkotOla5ghQnwnUXgk42F93xzDYyeft367mjHTsQkHKr6+X 7iWBaKX9pQqDzabuNk1CNLho/iPyrBd52aj+tJiB31kIzR9l9XUddqbgqdUBlH9p0fG+ dTe4kTf8V9JqxlOWI4SFEqHwiEXlKCw8LBHIV30dlPxdGzrdlq0hn8O1FIUMLoIgcrYT Wxkc8Wzb3FgXNEoYmK4r3BHRTvrB6D70R54s4qTRWi6wGJaWXiiDaaSsDEZYbw5jKzFd MKKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=mOtErK1fvYOWLBcEOMHjP4JSZnsLbTeSLgh7xo95WKs=; b=mglr7y9AMQBlQmMJ6/pOHXQGbPG2+c7Nqtmzn09GFR81fJU4Q06RTghZDloAI3W1iz 50azmyFbhRKCCstCBYwXAfK/J+T10LQZYbcH+CTNGIh2fuUKk6TVPGji0ydEOjWk5fdX PKhn6FH/deIP/SzTgL1geaAS4DW5PrS5H/KXxr2cDOzDhnV2zEjFZebzLzs8u4RcEHsV cW95o2r9m54sngCQ3cOpJyRX6TY9x0OTZlRIwnYbtPClZ3UXpfkCnD3Qcqx7NLXWuElG xgu3p94dEKxoZxNfZT0iSJ4Zt/4Bz0C3WkmYe3DXwMAwHWa/NYvGa+HljckgZvrvmakT 1ypA== X-Gm-Message-State: ACrzQf28Nz9Uw4K8uxJ+dEcRTsifIBIGw7VKNsy94DcRqV1tK8C7wYL1 eLtnl9kKRFmel6t2qfV0XL1d0A== X-Google-Smtp-Source: AMsMyM4t8n0u0LdMZIXHdl5WzPAcFWTrjRDU5bjFShJ5/oFbC2Lu1gVs/z+se4PeZCxgK8tOzv9mtA== X-Received: by 2002:a05:6a00:2392:b0:549:be0:cd3c with SMTP id f18-20020a056a00239200b005490be0cd3cmr27616441pfc.8.1665531087677; Tue, 11 Oct 2022 16:31:27 -0700 (PDT) Received: from [192.168.50.116] (c-24-4-73-83.hsd1.ca.comcast.net. [24.4.73.83]) by smtp.gmail.com with ESMTPSA id f11-20020a170902ab8b00b0017a09ebd1e2sm9102183plr.237.2022.10.11.16.31.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 11 Oct 2022 16:31:27 -0700 (PDT) Message-ID: <3862a109-8083-7a36-3d85-8f9e5e10627c@rivosinc.com> Date: Tue, 11 Oct 2022 16:31:25 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v2 00/10] [RISC-V] Atomics improvements [PR100265/PR100266] Content-Language: en-US To: =?UTF-8?Q?Christoph_M=c3=bcllner?= , Palmer Dabbelt Cc: andrea@rivosinc.com, gcc-patches@gcc.gnu.org, kito.cheng@sifive.com, gnu-toolchain@rivosinc.com References: From: Vineet Gupta In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,KAM_SHORT,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,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 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. > 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. Indeed we are stuck with inefficiencies with status quo. The new abi option sounds like a reasonable plan going fwd. 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. -Vineet