This is the mail archive of the libc-ports@sources.redhat.com mailing list for the libc-ports project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [ARM] bits/atomic.h and GCC atomic builtins


On Tuesday, November 16, 2010 4:55:36 pm Joseph S. Myers wrote:
> On Fri, 12 Nov 2010, Ken Werner wrote:
> > Hi,
> > 
> > This patch enhances the ARM version of bits/atomic.h to make use of the
> > atomic builtins provided by GCC in case the backend provides a pattern
> > to do this efficiently. Ok to apply?
> 
> How exactly did you test this patch?
> 
> > +#ifdef GNUC_PREREQ (4, 4)
> 
> This is not valid syntax.  The compiler should have warned.  It looks like
> you mean #if not #ifdef and __GNUC_PREREQ not GNUC_PREREQ.

Yes, this is a typo. I only verified that the GCC emits the correct 
instructions for the 4 byte version of the compare-and-swap macro. Later on I 
thought it won't hurt to also have the 1,2 and 8 byte versions defined even if 
the GCC only emits __sync_val_compare_and_swap_* calls. Therefore I've added 
the GNUC_PREREQ (4, 4) macro and made the typo.

> > +/* Use the atomic builtins provided by GCC version 4.4 and later.  */
> > +#define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
> > +  __sync_val_compare_and_swap ((mem), (oldval), (newval))
> > +#define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
> > +  __sync_val_compare_and_swap ((mem), (oldval), (newval))
> > +#define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
> > +  __sync_val_compare_and_swap ((mem), (oldval), (newval))
> > +#else
> > 
> >  #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
> >  
> >    ({ __arm_link_error (); oldval; })
> > 
> > -
> > 
> >  #define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
> >  
> >    ({ __arm_link_error (); oldval; })
> > 
> > +#define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
> > +  ({ __arm_link_error (); oldval; })
> > +#endif
> 
> In any case, the fact that the code worked before shows that these macros
> are not used and so this part of the change serves no purpose.  If it were
> useful, conditioning on __GCC_HAVE_SYNC_COMPARE_AND_SWAP_* would probably
> be better than a version conditional, although that would limit it to the
> cases where these operations are inlined.

Ok, sounds reasonable - thanks for your review. Attached is a revised patch 
that only touches the __arch_compare_and_exchange_val_32_acq and the 
atomic_full_barrier macros. Tested on arm-linux-gnueabi with no regressions.

Regards
Ken
diff --git a/ChangeLog b/ChangeLog
index e42182f..a9bc8fc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2010-11-18  Ken Werner  <ken.werner@de.ibm.com>
+
+	* sysdeps/unix/sysv/linux/arm/nptl/bits/atomic.h (atomic_full_barrier,
+	__arch_compare_and_exchange_val_32_acq): Use the atomic builtins
+	provided by GCC if __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is defined.
+
 2010-04-14  Joseph Myers  <joseph@codesourcery.com>
 
 	* libc-abis: Remove.
diff --git a/sysdeps/unix/sysv/linux/arm/nptl/bits/atomic.h b/sysdeps/unix/sysv/linux/arm/nptl/bits/atomic.h
index b0586ea..979db9f 100644
--- a/sysdeps/unix/sysv/linux/arm/nptl/bits/atomic.h
+++ b/sysdeps/unix/sysv/linux/arm/nptl/bits/atomic.h
@@ -37,7 +37,12 @@ typedef uintmax_t uatomic_max_t;
 
 void __arm_link_error (void);
 
-#ifdef __thumb2__
+/* Use the atomic builtins provided by GCC in case the backend provides
+   a pattern to do this efficiently.  */
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+#define atomic_full_barrier() __sync_synchronize ()
+#elif defined __thumb2__
 #define atomic_full_barrier() \
      __asm__ __volatile__						      \
 	     ("movw\tip, #0x0fa0\n\t"					      \
@@ -64,11 +69,15 @@ void __arm_link_error (void);
 #define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
   ({ __arm_link_error (); oldval; })
 
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+#define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
+  __sync_val_compare_and_swap ((mem), (oldval), (newval))
+
 /* It doesn't matter what register is used for a_oldval2, but we must
    specify one to work around GCC PR rtl-optimization/21223.  Otherwise
    it may cause a_oldval or a_tmp to be moved to a different register.  */
 
-#ifdef __thumb2__
+#elif defined __thumb2__
 /* Thumb-2 has ldrex/strex.  However it does not have barrier instructions,
    so we still need to use the kernel helper.  */
 #define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]