From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com [IPv6:2a00:1450:4864:20::62a]) by sourceware.org (Postfix) with ESMTPS id 3E4BB3858CD1 for ; Thu, 29 Feb 2024 03:07:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3E4BB3858CD1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 3E4BB3858CD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::62a ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709176054; cv=none; b=M20+OoEatcO8vOHMzXFq2HekI1xnnx/YfWxe1NecR/2V+gzvhnIPzzCDyhw14ecT8EG83bdJPdPP/XMWdvzK3/8a1wf0Z8okaJ2MGsXr0tIFgIRH8P7AkoSB+VYJjpp63b/bQBFU+tpmitw7IYEg5jf9xMUnC7fzwBAhYMVYH4c= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709176054; c=relaxed/simple; bh=q7n7bBNIDSoLIAVx2lrnC2rlh9nXqE9hOkXaAAqVug4=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=QFp5msjTg094cv2o6kXDzWOJC4kpgIUGENgaWIDWF6r8Pm7JosmGYPT8NiQ0Uf+kNGueEjxWRL6Ag324uAMuZGtM/jTazpUpZfYVClnRppDA9K5tOJ4vhoMK5oiI6YSD67xTgIFAmIwdeNr2ZssYNBdPM8NRzwr/r28yciutB2c= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-ej1-x62a.google.com with SMTP id a640c23a62f3a-a444205f764so16327366b.2 for ; Wed, 28 Feb 2024 19:07:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1709176051; x=1709780851; darn=gcc.gnu.org; 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=QOgof9DPF/8It1rOBDNKmW1BkRgYXXfwl5dwE2HFRUA=; b=m0RttuyO5JMQSyN9nsdld0pzUE0rXuhAhFWBg0e6vsUwWxuEN3eIeS6rr2iEGTnANf 4FDWtyP8B0hhRqh1cVZ/XY2YTrbfcqYcRWIWvty5k3AdW6VyttDDdleFdZT12AJFTsqc AyNS2HWETZ9Szl+4Rauifoc+qVX8gqrfXW6VANWzepdnd1/DdfqjVER7VDbEq4sei21A ioKM4HeaUvNHlNadWakVp1WWMJgT9/CFO3vYtntR5xmpzygIws818Y1tfdlgcgrOECHN ry4HtdDxIXOrc45PAZmzeJRL+6bmClLvvSwyt+KJCKQyS6CS0jACFKvqixFlj+UcWUyV ZOlg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709176051; x=1709780851; 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=QOgof9DPF/8It1rOBDNKmW1BkRgYXXfwl5dwE2HFRUA=; b=PIdWKY9m8SVAlJpgMSto9TTUx2XNH3ea36dSg9ERAjAx3SYCDaWTC/1/RykVPK91J2 IMOhWLcQbywbOTc/owr6i0yjE2u8S0xwPlCAUS/cDokUJrwn07r/hpXwp/QogUSw1Dam /Fl66UaZjvygNAJnhNt59wiMZWJplQBEFZqXmHomlBXF6gu5bkv/J+7mid4TruueryZ8 Z3tA0XBE/H9C3+zdooq7SSsP0ONcbYww5Ohj2LHbKaYrpZGm9c+S3doyKZr+d1cvKeux lYekhg6V+6PrvFl8x3+f3XT+OWxgXcu0Q+L9FSxskTRbE/J0aOJ2iUzausrBAPyJdDOR AR1A== X-Forwarded-Encrypted: i=1; AJvYcCWbiK5bhcNQZLaCEgyneSyuZ6jle6nQFKsyO1zYC3jeKOkauOp/OX5hkEQ2rbP34BcakM/L1dgnmuoRiFABIAeZkeC2cQAo2A== X-Gm-Message-State: AOJu0YwS7rGAFVAxaIlxo3I53VhCsj21zqmZZccWTTja4BMP6CKopqmZ kcU5uKYdOXaHP/uxyvjZ8CUdrCHCppDn373qbNYxjQbBdEH579rhjsvvCfXovtKvEIa2zzMW4uR YhkWKVwKASbIrARk09FRcx+nVZi4= X-Google-Smtp-Source: AGHT+IHEsU8n7juzVx1BZQKyEuMjOxu0eqQq5D8TRTCUCsL06zKUO9ZtsRwnzdTC/5MoxvKP8cMApC60hCJ1vzhNMCA= X-Received: by 2002:a17:906:5792:b0:a44:64d:93d1 with SMTP id k18-20020a170906579200b00a44064d93d1mr470847ejq.18.1709176050605; Wed, 28 Feb 2024 19:07:30 -0800 (PST) MIME-Version: 1.0 References: <0b01fcf9-8754-46af-b429-cf79442b0cf8@rivosinc.com> In-Reply-To: From: Kito Cheng Date: Thu, 29 Feb 2024 11:07:18 +0800 Message-ID: Subject: Re: [PATCH] RISC-V: Fix __atomic_compare_exchange with 32 bit value on RV64 To: Palmer Dabbelt Cc: "Patrick O'Neill" , kito.cheng@sifive.com, gcc-patches@gcc.gnu.org, pinskia@gmail.com, jeffreyalaw@gmail.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_NUMSUBJECT,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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: Committed with Palmer's suggestions for the commit message, also I plan to back port that to 11, 12 and 13 release branches :) On Thu, Feb 29, 2024 at 4:27=E2=80=AFAM Palmer Dabbelt = wrote: > > On Wed, 28 Feb 2024 09:36:38 PST (-0800), Patrick O'Neill wrote: > > > > On 2/28/24 07:02, Palmer Dabbelt wrote: > >> On Wed, 28 Feb 2024 06:57:53 PST (-0800), jeffreyalaw@gmail.com wrote: > >>> > >>> > >>> On 2/28/24 05:23, Kito Cheng wrote: > >>>> atomic_compare_and_swapsi will use lr.w and sc.w to do the atomic > >>>> operation on > >>>> RV64, however lr.w is doing sign extend to DI and compare > >>>> instruction only have > >>>> DI mode on RV64, so the expected value should be sign extend before > >>>> compare as > >>>> well, so that we can get right compare result. > >>>> > >>>> gcc/ChangeLog: > >>>> > >>>> PR target/114130 > >>>> * config/riscv/sync.md (atomic_compare_and_swap): Sign > >>>> extend the expected value if needed. > >>>> > >>>> gcc/testsuite/ChangeLog: > >>>> > >>>> * gcc.target/riscv/pr114130.c: New. > >>> Nearly rejected this as I think the description was a bit ambiguous a= nd > >>> I thought you were extending the result of the lr.w. But it's actual= ly > >>> the other value you're ensuring gets properly extended. > >> > >> I had the same response, but after reading it I'm not quite sure how > >> to say it better. > > Maybe something like > > atomic_compare_and_swapsi will use lr.w to do obtain the original val= ue, > which sign extends to DI. RV64 only has DI comparisons, so we also n= eed > to sign extend the expected value to DI as otherwise the comparison w= ill > fail when the expected value has the 32nd bit set. > > would do it? Either way > > Reviewed-by: Palmer Dabbelt > > as I've managed to convince myself it's correct. We should probably > backport this one, the bug has likely been around for a while. > > >> > >>> OK. > >> > >> I was looking at the code to try and ask if we have the same bug for > >> the short inline CAS routines, but I've got to run to some meetings... > > > > I don't think subword AMO CAS is impacted. > > > > As part of the CAS we mask both the expected value [2] and the retrieve= d > > value[1] before comparing. > > I'm always a bit lost when it comes to bit arithmetic, but I think it's > OK. It smells like it's being a little loose with the > extensions/comparisons, but just looking at some generated code for this > simple case: > > void foo(uint16_t *p, uint16_t *e, uint16_t *d) { > __atomic_compare_exchange(p, e, d, 0, __ATOMIC_RELAXED, __ATOMIC_= RELAXED); > } > > foo: > lhu a3,0(a2) > lhu a2,0(a1) > andi a4,a0,3 > li a5,65536 > slliw a4,a4,3 > addiw a5,a5,-1 > sllw a5,a5,a4 > sllw a3,a3,a4 > sllw a7,a2,a4 > andi a0,a0,-4 > and a3,a3,a5 > not t1,a5 > and a7,a7,a5 > 1: > lr.w a6, 0(a0) > and t3, a6, a5 // Both a6 (from the lr.w) and a5 > // (from the sllw) are sign extended, > // so the result in t3 is sign extended= . > bne t3, a7, 1f // a7 is also sign extended (before > // and after the masking above), so > // it's safe for comparison > and t3, a6, t1 > or t3, t3, a3 > sc.w t3, t3, 0(a0) // The top bits of t3 end up polluted > // with sign extension, but it doesn't > // matter because of the sc.w. > bnez t3, 1b > 1: > sraw a6,a6,a4 > slliw a2,a2,16 > slliw a5,a6,16 > sraiw a2,a2,16 > sraiw a5,a5,16 > subw a5,a5,a2 > beq a5,zero,.L1 > sh a6,0(a1) > .L1: > ret > > So I think we're OK -- that masking of a7 looks redundant here, but I > don't think we could get away with just > > diff --git a/gcc/config/riscv/sync.md b/gcc/config/riscv/sync.md > index 54bb0a66518..15956940032 100644 > --- a/gcc/config/riscv/sync.md > +++ b/gcc/config/riscv/sync.md > @@ -456,7 +456,6 @@ (define_expand "atomic_cas_value_strong" > riscv_lshift_subword (mode, o, shift, &shifted_o); > riscv_lshift_subword (mode, n, shift, &shifted_n); > > - emit_move_insn (shifted_o, gen_rtx_AND (SImode, shifted_o, mask)); > emit_move_insn (shifted_n, gen_rtx_AND (SImode, shifted_n, mask)); > > enum memmodel model_success =3D (enum memmodel) INTVAL (operands[4= ]); > > because we'd need the masking for when we don't know the high bits are > safe pre-shift. So maybe some sort of simplify call could help out > there, but I bet it's not really worth bothering -- the bookeeping > doesn't generally matter that much around AMOs. > > > - Patrick > > > > [1]: > > https://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob;f=3Dgcc/config/riscv/sync= .md;h=3D54bb0a66518ae353fa4ed640339213bf5da6682c;hb=3Drefs/heads/master#l49= 5 > > [2]: > > https://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob;f=3Dgcc/config/riscv/sync= .md;h=3D54bb0a66518ae353fa4ed640339213bf5da6682c;hb=3Drefs/heads/master#l45= 9 > > > >> > >>> > >>> Jeff