Fix and improve arm64 atomic operations in Base.

base/atomicops_internals_arm64_gcc.h

Index: base/atomicops_internals_arm64_gcc.h
diff --git a/base/atomicops_internals_arm64_gcc.h b/base/atomicops_internals_arm64_gcc.h
index a2b0abc1a470508f862a3d2ca00bd0d730bdcd92..03f1aec552d4454418eebefad0b0dc6d3fc644a5 100644
--- a/base/atomicops_internals_arm64_gcc.h
+++ b/base/atomicops_internals_arm64_gcc.h
@@ -21,12 +21,16 @@ namespace base {
 namespace subtle {
 
 inline void MemoryBarrier() {
-  __asm__ __volatile__ (  // NOLINT
-    "dmb ish                                  \n\t"  // Data memory barrier.
-    ::: "memory"
-  );  // NOLINT
+  __asm__ __volatile__ ("dmb ish" ::: "memory");  // NOLINT
 }
 
+// NoBarrier versions of the operation include "memory" in the clobber list.
+// This is not required for direct usage of the NoBarrier versions of the
+// operations. However this is required for correctness when they are used as
+// part of the Acquire or Release versions, to ensure that nothing from outside
+// the call is reordered between the operation and the memory barrier. This does
+// not change the code generated, so has no or minimal impact on the
+// NoBarrier operations.
 
 inline Atomic32 NoBarrier_CompareAndSwap(volatile Atomic32* ptr,
                                          Atomic32 old_value,
@@ -42,13 +46,12 @@ inline Atomic32 NoBarrier_CompareAndSwap(volatile Atomic32* ptr,
     "stxr %w[temp], %w[new_value], %[ptr]  \n\t"  // Try to store the new value.
     "cbnz %w[temp], 0b                     \n\t"  // Retry if it did not work.
     "1:                                    \n\t"
-    "clrex                                 \n\t"  // In case we didn't swap.
     : [prev]"=&r" (prev),
       [temp]"=&r" (temp),
       [ptr]"+Q" (*ptr)
-    : [old_value]"r" (old_value),
+    : [old_value]"IJr" (old_value),
       [new_value]"r" (new_value)
-    : "memory", "cc"
+    : "cc", "memory"
   );  // NOLINT
 
   return prev;
@@ -88,7 +91,7 @@ inline Atomic32 NoBarrier_AtomicIncrement(volatile Atomic32* ptr,
     : [result]"=&r" (result),
       [temp]"=&r" (temp),
       [ptr]"+Q" (*ptr)
-    : [increment]"r" (increment)
+    : [increment]"IJr" (increment)
     : "memory"
   );  // NOLINT
 
@@ -97,8 +100,10 @@ inline Atomic32 NoBarrier_AtomicIncrement(volatile Atomic32* ptr,
 
 inline Atomic32 Barrier_AtomicIncrement(volatile Atomic32* ptr,
                                         Atomic32 increment) {
+  Atomic32 result;

[Mark Mentovai, 2014/04/09 16:50:41]

This change doesn’t seem to help anything. We usually just declare things at the
point of first use (line 106). Atomic32 is just a standard integral type, so
it’s not like putting this here before the barrier runs any constructors.

[rmcilroy, 2014/04/10 12:30:46]

I think this change was to be consistent with the following functions, but I
agree - it's more usual to declare on first use.  Done.

+
   MemoryBarrier();
-  Atomic32 result = NoBarrier_AtomicIncrement(ptr, increment);
+  result = NoBarrier_AtomicIncrement(ptr, increment);
   MemoryBarrier();
 
   return result;
@@ -108,27 +113,9 @@ inline Atomic32 Acquire_CompareAndSwap(volatile Atomic32* ptr,
                                        Atomic32 old_value,
                                        Atomic32 new_value) {
   Atomic32 prev;
-  int32_t temp;
 
-  __asm__ __volatile__ (  // NOLINT
-    "0:                                    \n\t"
-    "ldxr %w[prev], %[ptr]                 \n\t"  // Load the previous value.
-    "cmp %w[prev], %w[old_value]           \n\t"
-    "bne 1f                                \n\t"
-    "stxr %w[temp], %w[new_value], %[ptr]  \n\t"  // Try to store the new value.
-    "cbnz %w[temp], 0b                     \n\t"  // Retry if it did not work.
-    "dmb ish                               \n\t"  // Data memory barrier.
-    "1:                                    \n\t"
-    // If the compare failed the 'dmb' is unnecessary, but we still need a
-    // 'clrex'.
-    "clrex                                 \n\t"
-    : [prev]"=&r" (prev),
-      [temp]"=&r" (temp),
-      [ptr]"+Q" (*ptr)
-    : [old_value]"r" (old_value),
-      [new_value]"r" (new_value)
-    : "memory", "cc"
-  );  // NOLINT
+  prev = NoBarrier_CompareAndSwap(ptr, old_value, new_value);

[Mark Mentovai, 2014/04/09 16:50:41]

Likewise, why not declare it on this line?

Also in Barrier_AtomicIncrement, Acquire_CompareAndSwap, and
Release_CompareAndSwap.

+  MemoryBarrier();
 
   return prev;
 }
@@ -137,27 +124,9 @@ inline Atomic32 Release_CompareAndSwap(volatile Atomic32* ptr,
                                        Atomic32 old_value,
                                        Atomic32 new_value) {
   Atomic32 prev;
-  int32_t temp;
 
   MemoryBarrier();
-
-  __asm__ __volatile__ (  // NOLINT
-    "0:                                    \n\t"
-    "ldxr %w[prev], %[ptr]                 \n\t"  // Load the previous value.
-    "cmp %w[prev], %w[old_value]           \n\t"
-    "bne 1f                                \n\t"
-    "stxr %w[temp], %w[new_value], %[ptr]  \n\t"  // Try to store the new value.
-    "cbnz %w[temp], 0b                     \n\t"  // Retry if it did not work.
-    "1:                                    \n\t"
-    // If the compare failed the we still need a 'clrex'.
-    "clrex                                 \n\t"
-    : [prev]"=&r" (prev),
-      [temp]"=&r" (temp),
-      [ptr]"+Q" (*ptr)
-    : [old_value]"r" (old_value),
-      [new_value]"r" (new_value)
-    : "memory", "cc"
-  );  // NOLINT
+  prev = NoBarrier_CompareAndSwap(ptr, old_value, new_value);

[Mark Mentovai, 2014/04/09 16:50:41]

And again here. In fact, can’t this function just be MemoryBarrier(); return
NoBarrier_CompareAndSwap(…);?

 
   return prev;
 }
@@ -172,8 +141,12 @@ inline void Acquire_Store(volatile Atomic32* ptr, Atomic32 value) {
 }
 
 inline void Release_Store(volatile Atomic32* ptr, Atomic32 value) {
-  MemoryBarrier();
-  *ptr = value;
+  __asm__ __volatile__ (  // NOLINT
+    "stlr %w[value], %[ptr]  \n\t"
+    : [ptr]"=Q" (*ptr)
+    : [value]"r" (value)
+    : "memory"
+  );  // NOLINT
 }
 
 inline Atomic32 NoBarrier_Load(volatile const Atomic32* ptr) {
@@ -181,8 +154,15 @@ inline Atomic32 NoBarrier_Load(volatile const Atomic32* ptr) {
 }
 
 inline Atomic32 Acquire_Load(volatile const Atomic32* ptr) {
-  Atomic32 value = *ptr;
-  MemoryBarrier();
+  Atomic32 value;
+
+  __asm__ __volatile__ (  // NOLINT
+    "ldar %w[value], %[ptr]  \n\t"
+    : [value]"=r" (value)
+    : [ptr]"Q" (*ptr)
+    : "memory"
+  );  // NOLINT
+
   return value;
 }
 
@@ -208,13 +188,12 @@ inline Atomic64 NoBarrier_CompareAndSwap(volatile Atomic64* ptr,
     "stxr %w[temp], %[new_value], %[ptr]   \n\t"
     "cbnz %w[temp], 0b                     \n\t"
     "1:                                    \n\t"
-    "clrex                                 \n\t"
     : [prev]"=&r" (prev),
       [temp]"=&r" (temp),
       [ptr]"+Q" (*ptr)
-    : [old_value]"r" (old_value),
+    : [old_value]"IJr" (old_value),
       [new_value]"r" (new_value)
-    : "memory", "cc"
+    : "cc", "memory"
   );  // NOLINT
 
   return prev;
@@ -254,7 +233,7 @@ inline Atomic64 NoBarrier_AtomicIncrement(volatile Atomic64* ptr,
     : [result]"=&r" (result),
       [temp]"=&r" (temp),
       [ptr]"+Q" (*ptr)
-    : [increment]"r" (increment)
+    : [increment]"IJr" (increment)
     : "memory"
   );  // NOLINT
 
@@ -263,8 +242,10 @@ inline Atomic64 NoBarrier_AtomicIncrement(volatile Atomic64* ptr,
 
 inline Atomic64 Barrier_AtomicIncrement(volatile Atomic64* ptr,
                                         Atomic64 increment) {
+  Atomic64 result;
+
   MemoryBarrier();
-  Atomic64 result = NoBarrier_AtomicIncrement(ptr, increment);
+  result = NoBarrier_AtomicIncrement(ptr, increment);
   MemoryBarrier();
 
   return result;
@@ -274,25 +255,9 @@ inline Atomic64 Acquire_CompareAndSwap(volatile Atomic64* ptr,
                                        Atomic64 old_value,
                                        Atomic64 new_value) {
   Atomic64 prev;
-  int32_t temp;
 
-  __asm__ __volatile__ (  // NOLINT
-    "0:                                    \n\t"
-    "ldxr %[prev], %[ptr]                  \n\t"
-    "cmp %[prev], %[old_value]             \n\t"
-    "bne 1f                                \n\t"
-    "stxr %w[temp], %[new_value], %[ptr]   \n\t"
-    "cbnz %w[temp], 0b                     \n\t"
-    "dmb ish                               \n\t"
-    "1:                                    \n\t"
-    "clrex                                 \n\t"
-    : [prev]"=&r" (prev),
-      [temp]"=&r" (temp),
-      [ptr]"+Q" (*ptr)
-    : [old_value]"r" (old_value),
-      [new_value]"r" (new_value)
-    : "memory", "cc"
-  );  // NOLINT
+  prev = NoBarrier_CompareAndSwap(ptr, old_value, new_value);
+  MemoryBarrier();
 
   return prev;
 }
@@ -301,26 +266,9 @@ inline Atomic64 Release_CompareAndSwap(volatile Atomic64* ptr,
                                        Atomic64 old_value,
                                        Atomic64 new_value) {
   Atomic64 prev;
-  int32_t temp;
 
   MemoryBarrier();
-
-  __asm__ __volatile__ (  // NOLINT
-    "0:                                    \n\t"
-    "ldxr %[prev], %[ptr]                  \n\t"
-    "cmp %[prev], %[old_value]             \n\t"
-    "bne 1f                                \n\t"
-    "stxr %w[temp], %[new_value], %[ptr]   \n\t"
-    "cbnz %w[temp], 0b                     \n\t"
-    "1:                                    \n\t"
-    "clrex                                 \n\t"
-    : [prev]"=&r" (prev),
-      [temp]"=&r" (temp),
-      [ptr]"+Q" (*ptr)
-    : [old_value]"r" (old_value),
-      [new_value]"r" (new_value)
-    : "memory", "cc"
-  );  // NOLINT
+  prev = NoBarrier_CompareAndSwap(ptr, old_value, new_value);
 
   return prev;
 }
@@ -335,8 +283,12 @@ inline void Acquire_Store(volatile Atomic64* ptr, Atomic64 value) {
 }
 
 inline void Release_Store(volatile Atomic64* ptr, Atomic64 value) {
-  MemoryBarrier();
-  *ptr = value;
+  __asm__ __volatile__ (  // NOLINT
+    "stlr %x[value], %[ptr]  \n\t"
+    : [ptr]"=Q" (*ptr)
+    : [value]"r" (value)
+    : "memory"
+  );  // NOLINT
 }
 
 inline Atomic64 NoBarrier_Load(volatile const Atomic64* ptr) {
@@ -344,8 +296,15 @@ inline Atomic64 NoBarrier_Load(volatile const Atomic64* ptr) {
 }
 
 inline Atomic64 Acquire_Load(volatile const Atomic64* ptr) {
-  Atomic64 value = *ptr;
-  MemoryBarrier();
+  Atomic32 value;
+
+  __asm__ __volatile__ (  // NOLINT
+    "ldar %x[value], %[ptr]  \n\t"
+    : [value]"=r" (value)
+    : [ptr]"Q" (*ptr)
+    : "memory"
+  );  // NOLINT
+
   return value;
 }