> -----Original Message----- > From: Tamar Christina <tamar.christ...@arm.com> > Sent: Monday, August 8, 2022 10:28 AM > To: Kyrylo Tkachov <kyrylo.tkac...@arm.com>; gcc-patches@gcc.gnu.org > Cc: nd <n...@arm.com>; Richard Earnshaw <richard.earns...@arm.com>; > Marcus Shawcroft <marcus.shawcr...@arm.com>; Richard Sandiford > <richard.sandif...@arm.com> > Subject: RE: [PATCH 1/2]AArch64 Fix 128-bit sequential consistency atomic > operations. > > > > -----Original Message----- > > From: Kyrylo Tkachov <kyrylo.tkac...@arm.com> > > Sent: Tuesday, July 12, 2022 2:46 PM > > To: Tamar Christina <tamar.christ...@arm.com>; gcc- > patc...@gcc.gnu.org > > Cc: nd <n...@arm.com>; Richard Earnshaw <richard.earns...@arm.com>; > > Marcus Shawcroft <marcus.shawcr...@arm.com>; Richard Sandiford > > <richard.sandif...@arm.com> > > Subject: RE: [PATCH 1/2]AArch64 Fix 128-bit sequential consistency atomic > > operations. > > > > Hi Tamar, > > > > Let me be the latest to offer my apologies for the slow review. > > > > > -----Original Message----- > > > From: Tamar Christina <tamar.christ...@arm.com> > > > Sent: Wednesday, June 8, 2022 3:49 PM > > > To: gcc-patches@gcc.gnu.org > > > Cc: nd <n...@arm.com>; Richard Earnshaw > <richard.earns...@arm.com>; > > > Marcus Shawcroft <marcus.shawcr...@arm.com>; Kyrylo Tkachov > > > <kyrylo.tkac...@arm.com>; Richard Sandiford > > > <richard.sandif...@arm.com> > > > Subject: [PATCH 1/2]AArch64 Fix 128-bit sequential consistency atomic > > > operations. > > > > > > Hi All, > > > > > > The AArch64 implementation of 128-bit atomics is broken. > > > > > > For 128-bit atomics we rely on pthread barriers to correct guard the > > > address in the pointer to get correct memory ordering. However for > > > 128-bit atomics the address under the lock is different from the > > > original pointer. > > > > > > This means that one of the values under the atomic operation is not > > > protected properly and so we fail during when the user has requested > > > sequential consistency as there's no barrier to enforce this > > > requirement. > > > > > > As such users have resorted to adding an > > > > > > #ifdef GCC > > > <emit barrier> > > > #endif > > > > > > around the use of these atomics. > > > > > > This corrects the issue by issuing a barrier only when > > > __ATOMIC_SEQ_CST was requested. To remedy this performance hit I > > > think we should revisit using a similar approach to out-line-atomics > > > for the 128-bit atomics. > > > > > > Note that I believe I need the empty file due to the include_next > > > chain but I am not entirely sure. I have hand verified that the > > > barriers are inserted for atomic seq cst. > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > > > Ok for master? and for backporting to GCC 12, 11 and 10? > > > > I'll admit I'm not too familiar with the mechanics of libatomic but... > > > > > > > > Thanks, > > > Tamar > > > > > > libatomic/ChangeLog: > > > > > > PR target/102218 > > > * config/aarch64/aarch64-config.h: New file. > > > * config/aarch64/host-config.h: New file. > > > > > > --- inline copy of patch -- > > > diff --git a/libatomic/config/aarch64/aarch64-config.h > > > b/libatomic/config/aarch64/aarch64-config.h > > > new file mode 100644 > > > index > > > > 0000000000000000000000000000000000000000..d3474fa8ff80cb0c3ddbf8c4 > > > 8acd931d2339d33d > > > --- /dev/null > > > +++ b/libatomic/config/aarch64/aarch64-config.h > > > @@ -0,0 +1,23 @@ > > > +/* Copyright (C) 2022 Free Software Foundation, Inc. > > > + > > > + This file is part of the GNU Atomic Library (libatomic). > > > + > > > + Libatomic is free software; you can redistribute it and/or modify it > > > + under the terms of the GNU General Public License as published by > > > + the Free Software Foundation; either version 3 of the License, or > > > + (at your option) any later version. > > > + > > > + Libatomic is distributed in the hope that it will be useful, but > > > + WITHOUT > > > ANY > > > + WARRANTY; without even the implied warranty of MERCHANTABILITY > or > > > FITNESS > > > + FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > > + more details. > > > + > > > + Under Section 7 of GPL version 3, you are granted additional > > > + permissions described in the GCC Runtime Library Exception, version > > > + 3.1, as published by the Free Software Foundation. > > > + > > > + You should have received a copy of the GNU General Public License > and > > > + a copy of the GCC Runtime Library Exception along with this program; > > > + see the files COPYING3 and COPYING.RUNTIME respectively. If not, > see > > > + <http://www.gnu.org/licenses/>. */ > > > + > > > diff --git a/libatomic/config/aarch64/host-config.h > > > b/libatomic/config/aarch64/host-config.h > > > new file mode 100644 > > > index > > > > 0000000000000000000000000000000000000000..f445a47d25ef5cc51cd21670 > > > 69500245d07bf1bc > > > --- /dev/null > > > +++ b/libatomic/config/aarch64/host-config.h > > > @@ -0,0 +1,46 @@ > > > +/* Copyright (C) 2022 Free Software Foundation, Inc. > > > + > > > + This file is part of the GNU Atomic Library (libatomic). > > > + > > > + Libatomic is free software; you can redistribute it and/or modify it > > > + under the terms of the GNU General Public License as published by > > > + the Free Software Foundation; either version 3 of the License, or > > > + (at your option) any later version. > > > + > > > + Libatomic is distributed in the hope that it will be useful, but > > > + WITHOUT > > > ANY > > > + WARRANTY; without even the implied warranty of MERCHANTABILITY > or > > > FITNESS > > > + FOR A PARTICULAR PURPOSE. See the GNU General Public License for > > > + more details. > > > + > > > + Under Section 7 of GPL version 3, you are granted additional > > > + permissions described in the GCC Runtime Library Exception, version > > > + 3.1, as published by the Free Software Foundation. > > > + > > > + You should have received a copy of the GNU General Public License > and > > > + a copy of the GCC Runtime Library Exception along with this program; > > > + see the files COPYING3 and COPYING.RUNTIME respectively. If not, > see > > > + <http://www.gnu.org/licenses/>. */ > > > + > > > +/* Avoiding the DMB (or kernel helper) can be a good thing. */ > > > +#define WANT_SPECIALCASE_RELAXED > > > + > > > +/* Glibc, at least, uses acq_rel in its pthread mutex > > > + implementation. If the user is asking for seq_cst, > > > + this is insufficient. */ > > > + > > > +static inline void __attribute__((always_inline, artificial)) > > > +pre_seq_barrier(int model) { > > > + if (model == __ATOMIC_SEQ_CST) > > > + __atomic_thread_fence (__ATOMIC_SEQ_CST); } > > > + > > > +static inline void __attribute__((always_inline, artificial)) > > > +post_seq_barrier(int model) { > > > + pre_seq_barrier(model); > > > +} > > > + > > > +#define pre_post_seq_barrier 1 > > > + > > > +#include_next <host-config.h> > > > > ... This does looks sensible and similar to what's done on powerpc, which is > > similar to the aarch64 target in this regard. > > However, there is already a host-config.h in config/linux/aarch64/host- > > config.h . Does this file end up including the one in config/linux? > > If so, does this mean that this works correctly (i.e. was tested) for > > aarch64- > > none-elf as well as Linux? > > > > Hi, > > We don't build libatomic on any elf platforms. It has a default unsupported > flag which we don't override. Indeed we don't produce libatomic.a for elf > and a simple example fails to link as well..
Ok, then I think this patch is a step in the right direction. Ok for trunk. Thanks, Kyrill > > Regards, > Tamar > > > Thanks, > > Kyrill > > > > > > > > > > > > > > > > > --
