Implement sk_atomic_conditional_inc with sk_atomic_cas.

include/core/SkThread.h

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkThread_DEFINED
 #define SkThread_DEFINED
 
 #include "SkTypes.h"
 
 // SK_ATOMICS_PLATFORM_H must provide inline implementations for the following declarations.
 
 /** Atomically adds one to the int referenced by addr and returns the previous value.
  *  No additional memory barrier is required; this must act as a compiler barrier.
  */
 static int32_t sk_atomic_inc(int32_t* addr);
 
 /** Atomically adds inc to the int referenced by addr and returns the previous value.
  *  No additional memory barrier is required; this must act as a compiler barrier.
  */
 static int32_t sk_atomic_add(int32_t* addr, int32_t inc);
 
 /** Atomically subtracts one from the int referenced by addr and returns the previous value.
  *  This must act as a release (SL/S) memory barrier and as a compiler barrier.
  */
 static int32_t sk_atomic_dec(int32_t* addr);
 
-/** Atomically adds one to the int referenced by addr iff the referenced int was not 0
- *  and returns the previous value.
- *  No additional memory barrier is required; this must act as a compiler barrier.
- */
-static int32_t sk_atomic_conditional_inc(int32_t* addr);
-
 /** Atomic compare and set.
  *  If *addr == before, set *addr to after and return true, otherwise return false.
  *  This must act as a release (SL/S) memory barrier and as a compiler barrier.
  */
 static bool sk_atomic_cas(int32_t* addr, int32_t before, int32_t after);
 
 /** If sk_atomic_dec does not act as an acquire (L/SL) barrier,
  *  this must act as an acquire (L/SL) memory barrier and as a compiler barrier.
  */
 static void sk_membar_acquire__after_atomic_dec();
 
 /** If sk_atomic_conditional_inc does not act as an acquire (L/SL) barrier,
  *  this must act as an acquire (L/SL) memory barrier and as a compiler barrier.
  */
 static void sk_membar_acquire__after_atomic_conditional_inc();
 
 #include SK_ATOMICS_PLATFORM_H
 
+/** Atomically adds one to the int referenced by addr iff the referenced int was not 0
+ *  and returns the previous value.
+ *  No additional memory barrier is required; this must act as a compiler barrier.
+ */
+static int32_t sk_atomic_conditional_inc(int32_t* addr) {
+    int32_t prev;
+    do {
+        prev = *addr;

[bungeman-skia, 2014/05/27 14:20:20]

Eck, it irks me that we have to do this load on all platforms, when on x86 the
cas tells us the previous value. Linked Load / Store Conditional platforms like
ARM actually don't know, which is why android only reports bool. I suppose on
all platforms the addr has to be loaded as part of the cas, so at least the
value at that point should be close at hand.

[mtklein, 2014/05/27 14:27:27]

Agreed.  sk_atomic_cas is a bit least-common-denominator, but it seems good
enough.

+        if (0 == prev) {

[reed1, 2014/05/27 14:36:18]

Why the test/break? Does this warrent a comment?

[mtklein, 2014/05/27 14:38:30]

Would return 0 be clearer?  That's the conditional part of the conditional_inc:
don't increment 0.

[reed1, 2014/05/27 14:44:19]

Ah, no, my bad. I didn't read the bloody name of the (new) function.

Returning 0 explicitly might be clearer, but I'm not asking for that change
per-se.

+            break;
+        }
+    } while (!sk_atomic_cas(addr, prev, prev+1));
+    return prev;
+}
+
 /** SK_MUTEX_PLATFORM_H must provide the following (or equivalent) declarations.
 
 class SkBaseMutex {
 public:
     void acquire();
     void release();
 };
 
 class SkMutex : SkBaseMutex {
 public:
@@ skipping 35 matching lines @@
             fMutex = NULL;
         }
     }
 
 private:
     SkBaseMutex* fMutex;
 };
 #define SkAutoMutexAcquire(...) SK_REQUIRE_LOCAL_VAR(SkAutoMutexAcquire)
 
 #endif