[Mesa-dev] util/u_atomic: Add new macro p_atomic_add

Submitted by Carl Worth on Feb. 6, 2015, 7:43 p.m.

Details

Message ID 1423251804-15279-1-git-send-email-cworth@cworth.org
State New
Headers show

Not browsing as part of any series.

Commit Message

Carl Worth Feb. 6, 2015, 7:43 p.m.
This provides for atomic addition, which will be used by an upcoming
shader-cache patch. A simple test is added to "make check" as well.

Note: The various O/S functions differ on whether they return the
original value or the value after the addition, so I did not provide
an add_return() macro which would be sensitive to that difference.

---

Note: I referenced the MSDN web pages and some online Solaris man
pages to update the relevant sections, (but I have neither compiled
nor tested this new macro on those operating systems). I would
appreciate any relvant review or reports from testing.

 src/util/u_atomic.h      | 16 ++++++++++++++++
 src/util/u_atomic_test.c |  5 +++++
 2 files changed, 21 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/util/u_atomic.h b/src/util/u_atomic.h
index cf7fff3..4eb0ec6 100644
--- a/src/util/u_atomic.h
+++ b/src/util/u_atomic.h
@@ -39,6 +39,7 @@ 
 #define p_atomic_dec_zero(v) (__sync_sub_and_fetch((v), 1) == 0)
 #define p_atomic_inc(v) (void) __sync_add_and_fetch((v), 1)
 #define p_atomic_dec(v) (void) __sync_sub_and_fetch((v), 1)
+#define p_atomic_add(v, i) (void) __sync_add_and_fetch((v), (i))
 #define p_atomic_inc_return(v) __sync_add_and_fetch((v), 1)
 #define p_atomic_dec_return(v) __sync_sub_and_fetch((v), 1)
 #define p_atomic_cmpxchg(v, old, _new) \
@@ -60,6 +61,7 @@ 
 #define p_atomic_dec_zero(_v) (p_atomic_dec_return(_v) == 0)
 #define p_atomic_inc(_v) ((void) p_atomic_inc_return(_v))
 #define p_atomic_dec(_v) ((void) p_atomic_dec_return(_v))
+#define p_atomic_add(_v, _i) (*(_v) = *(_v) + (_i))
 #define p_atomic_inc_return(_v) (++(*(_v)))
 #define p_atomic_dec_return(_v) (--(*(_v)))
 #define p_atomic_cmpxchg(_v, _old, _new) (*(_v) == (_old) ? (*(_v) = (_new), (_old)) : *(_v))
@@ -144,6 +146,13 @@  char _InterlockedCompareExchange8(char volatile *Destination8, char Exchange8, c
    sizeof *(_v) == sizeof(__int64) ? InterlockedDecrement64 ((__int64 *)(_v)) : \
                                      (assert(!"should not get here"), 0))
 
+#define p_atomic_add(_v, _i) (\
+   sizeof *(_v) == sizeof(short)   ? _InterlockedExchangeAdd8 ((char *)   (_v), (_i)) : \
+   sizeof *(_v) == sizeof(short)   ? _InterlockedExchangeAdd16((short *)  (_v), (_i)) : \
+   sizeof *(_v) == sizeof(long)    ? _InterlockedExchangeAdd  ((long *)   (_v), (_i)) : \
+   sizeof *(_v) == sizeof(__int64) ? _InterlockedExchangeAdd64((__int64 *)(_v), (_i)) : \
+                                     (assert(!"should not get here"), 0))
+
 #define p_atomic_cmpxchg(_v, _old, _new) (\
    sizeof *(_v) == sizeof(char)    ? _InterlockedCompareExchange8 ((char *)   (_v), (char)   (_new), (char)   (_old)) : \
    sizeof *(_v) == sizeof(short)   ? _InterlockedCompareExchange16((short *)  (_v), (short)  (_new), (short)  (_old)) : \
@@ -198,6 +207,13 @@  char _InterlockedCompareExchange8(char volatile *Destination8, char Exchange8, c
    sizeof(*v) == sizeof(uint64_t) ? atomic_dec_64_nv((uint64_t *)(v)) : \
                                     (assert(!"should not get here"), 0))
 
+#define p_atomic_add(v, i) ((void)				     \
+   sizeof(*v) == sizeof(uint8_t)  ? atomic_add_8 ((uint8_t  *)(v), (i)) : \
+   sizeof(*v) == sizeof(uint16_t) ? atomic_add_16((uint16_t *)(v), (i)) : \
+   sizeof(*v) == sizeof(uint32_t) ? atomic_add_32((uint32_t *)(v), (i)) : \
+   sizeof(*v) == sizeof(uint64_t) ? atomic_add_64((uint64_t *)(v), (i)) : \
+                                    (assert(!"should not get here"), 0))
+
 #define p_atomic_cmpxchg(v, old, _new) ((typeof(*v)) \
    sizeof(*v) == sizeof(uint8_t)  ? atomic_cas_8 ((uint8_t  *)(v), (uint8_t )(old), (uint8_t )(_new)) : \
    sizeof(*v) == sizeof(uint16_t) ? atomic_cas_16((uint16_t *)(v), (uint16_t)(old), (uint16_t)(_new)) : \
diff --git a/src/util/u_atomic_test.c b/src/util/u_atomic_test.c
index 4845e75..c506275 100644
--- a/src/util/u_atomic_test.c
+++ b/src/util/u_atomic_test.c
@@ -97,6 +97,11 @@ 
       assert(v == ones && "p_atomic_dec_return"); \
       assert(r == v && "p_atomic_dec_return"); \
       \
+      v = 23; \
+      p_atomic_add(&v, 42); \
+      r = p_atomic_read(&v); \
+      assert(r == 65 && "p_atomic_add"); \
+      \
       (void) r; \
       (void) b; \
    }

Comments

On Fri, Feb 6, 2015 at 11:43 AM, Carl Worth <cworth@cworth.org> wrote:
> This provides for atomic addition, which will be used by an upcoming
> shader-cache patch. A simple test is added to "make check" as well.
>
> Note: The various O/S functions differ on whether they return the
> original value or the value after the addition, so I did not provide
> an add_return() macro which would be sensitive to that difference.
>
> ---
>
> Note: I referenced the MSDN web pages and some online Solaris man
> pages to update the relevant sections, (but I have neither compiled
> nor tested this new macro on those operating systems). I would
> appreciate any relvant review or reports from testing.

The gcc bits are

Reviewed-by: Matt Turner <mattst88@gmail.com>

and I don't see anything wrong with the MSVC/Solaris bits, FWIW.

Thanks for extending the test case as well.
On Fri, Feb 6, 2015 at 1:43 PM, Carl Worth <cworth@cworth.org> wrote:

> This provides for atomic addition, which will be used by an upcoming
> shader-cache patch. A simple test is added to "make check" as well.
>
> Note: The various O/S functions differ on whether they return the
> original value or the value after the addition, so I did not provide
> an add_return() macro which would be sensitive to that difference.
>
> ---
>
> Note: I referenced the MSDN web pages and some online Solaris man
> pages to update the relevant sections, (but I have neither compiled
> nor tested this new macro on those operating systems). I would
> appreciate any relvant review or reports from testing.
>
>  src/util/u_atomic.h      | 16 ++++++++++++++++
>  src/util/u_atomic_test.c |  5 +++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/src/util/u_atomic.h b/src/util/u_atomic.h
> index cf7fff3..4eb0ec6 100644
> --- a/src/util/u_atomic.h
> +++ b/src/util/u_atomic.h
> @@ -39,6 +39,7 @@
>  #define p_atomic_dec_zero(v) (__sync_sub_and_fetch((v), 1) == 0)
>  #define p_atomic_inc(v) (void) __sync_add_and_fetch((v), 1)
>  #define p_atomic_dec(v) (void) __sync_sub_and_fetch((v), 1)
> +#define p_atomic_add(v, i) (void) __sync_add_and_fetch((v), (i))
>  #define p_atomic_inc_return(v) __sync_add_and_fetch((v), 1)
>  #define p_atomic_dec_return(v) __sync_sub_and_fetch((v), 1)
>  #define p_atomic_cmpxchg(v, old, _new) \
> @@ -60,6 +61,7 @@
>  #define p_atomic_dec_zero(_v) (p_atomic_dec_return(_v) == 0)
>  #define p_atomic_inc(_v) ((void) p_atomic_inc_return(_v))
>  #define p_atomic_dec(_v) ((void) p_atomic_dec_return(_v))
> +#define p_atomic_add(_v, _i) (*(_v) = *(_v) + (_i))
>  #define p_atomic_inc_return(_v) (++(*(_v)))
>  #define p_atomic_dec_return(_v) (--(*(_v)))
>  #define p_atomic_cmpxchg(_v, _old, _new) (*(_v) == (_old) ? (*(_v) =
> (_new), (_old)) : *(_v))
> @@ -144,6 +146,13 @@ char _InterlockedCompareExchange8(char volatile
> *Destination8, char Exchange8, c
>     sizeof *(_v) == sizeof(__int64) ? InterlockedDecrement64 ((__int64
> *)(_v)) : \
>                                       (assert(!"should not get here"), 0))
>
> +#define p_atomic_add(_v, _i) (\
> +   sizeof *(_v) == sizeof(short)   ? _InterlockedExchangeAdd8 ((char *)
>  (_v), (_i)) : \
>
+   sizeof *(_v) == sizeof(short)   ? _InterlockedExchangeAdd16((short *)
> (_v), (_i)) : \
>

Ignore me if this is a stupid question, but should those both be
sizeof(short)?  I'd expect the first to be sizeof(char).

--Aaron


> +   sizeof *(_v) == sizeof(long)    ? _InterlockedExchangeAdd  ((long *)
>  (_v), (_i)) : \
> +   sizeof *(_v) == sizeof(__int64) ? _InterlockedExchangeAdd64((__int64
> *)(_v), (_i)) : \
> +                                     (assert(!"should not get here"), 0))
> +
>  #define p_atomic_cmpxchg(_v, _old, _new) (\
>     sizeof *(_v) == sizeof(char)    ? _InterlockedCompareExchange8 ((char
> *)   (_v), (char)   (_new), (char)   (_old)) : \
>     sizeof *(_v) == sizeof(short)   ? _InterlockedCompareExchange16((short
> *)  (_v), (short)  (_new), (short)  (_old)) : \
> @@ -198,6 +207,13 @@ char _InterlockedCompareExchange8(char volatile
> *Destination8, char Exchange8, c
>     sizeof(*v) == sizeof(uint64_t) ? atomic_dec_64_nv((uint64_t *)(v)) : \
>                                      (assert(!"should not get here"), 0))
>
> +#define p_atomic_add(v, i) ((void)                                  \
> +   sizeof(*v) == sizeof(uint8_t)  ? atomic_add_8 ((uint8_t  *)(v), (i)) :
> \
> +   sizeof(*v) == sizeof(uint16_t) ? atomic_add_16((uint16_t *)(v), (i)) :
> \
> +   sizeof(*v) == sizeof(uint32_t) ? atomic_add_32((uint32_t *)(v), (i)) :
> \
> +   sizeof(*v) == sizeof(uint64_t) ? atomic_add_64((uint64_t *)(v), (i)) :
> \
> +                                    (assert(!"should not get here"), 0))
> +
>  #define p_atomic_cmpxchg(v, old, _new) ((typeof(*v)) \
>     sizeof(*v) == sizeof(uint8_t)  ? atomic_cas_8 ((uint8_t  *)(v),
> (uint8_t )(old), (uint8_t )(_new)) : \
>     sizeof(*v) == sizeof(uint16_t) ? atomic_cas_16((uint16_t *)(v),
> (uint16_t)(old), (uint16_t)(_new)) : \
> diff --git a/src/util/u_atomic_test.c b/src/util/u_atomic_test.c
> index 4845e75..c506275 100644
> --- a/src/util/u_atomic_test.c
> +++ b/src/util/u_atomic_test.c
> @@ -97,6 +97,11 @@
>        assert(v == ones && "p_atomic_dec_return"); \
>        assert(r == v && "p_atomic_dec_return"); \
>        \
> +      v = 23; \
> +      p_atomic_add(&v, 42); \
> +      r = p_atomic_read(&v); \
> +      assert(r == 65 && "p_atomic_add"); \
> +      \
>        (void) r; \
>        (void) b; \
>     }
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Fri, Feb 06 2015, Aaron Watry wrote:
> Ignore me if this is a stupid question, but should those both be
> sizeof(short)?  I'd expect the first to be sizeof(char).

Not a stupid question. That was a copy-and-paste (kill-and-yank ?) bug
of mine.

Thanks for your attention to detail. I've fixed this in my tree.

-Carl