> -----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-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. > > 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.. Regards, Tamar > Thanks, > Kyrill > > > > > > > > > > > --