[Mesa-dev] [PATCH 0/7] util: Move u_atomic.h to src/util and modify API

[Matt Turner]

On Tue, Nov 25, 2014 at 6:48 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
> On 25/11/14 00:39, Matt Turner wrote:
>>
>> I've got some thread-safety fixes queued up after this and thought I'd
>> be a good Mesa citizen and pull some code into src/util.
>
>
> Thanks for going the extra mile!
>
>> I did some clean ups like replacing INLINE (MSVC knows about "inline"
>> these days, right?) and used stdbool.h instead of the "boolean" type.
>
>
> No, at least MSVC 2012 doesn't have `inline` keyword when compiling C files,
> and requires a
>
>   #if !defined(__cplusplus) && !defined(inline)
>   #define inline __inline
>   #endif
>
> somewhere.  Anyway, there are no `INLINES` nor `inlines` left after your
> series, so we're good.
>
> stdbool.h is fine -- we include our own when MSVC doesn't
>
>> I also removed the inline assembly implementations because they were
>> either dead code, or only allowed *ancient* gcc to build Mesa and
>> because I didn't want to update them for the next patch, which makes
>> the API consist of some macros that internally do the right operation
>> based on the type.
>>
>> The last patch looks funky, but I think it's actually a reasonable
>> solution. I don't have MSVC or Sun Studio, so please give this a
>> test.
>
>
> I had to do a few tweaks to get things building on MSVC properly.
>
> I pushed my changes to
>
>   http://cgit.freedesktop.org/~jrfonseca/mesa/log/?h=u_atomic
>
> I need to do a few more tests, but all looks feasible so far -- I don't get
> any warnings with MSVC and I believe that the generated code quality should
> be exactly the same.
>
> And it is indeed a nice cleanup.

Excellent, thanks a lot José!

I'll merge your patches into my series and wire up the test into
automake. I'll also put parentheses around the nested ternary in the
Sun Studio case, like you noticed was necessary for MSVC.

My thread-safety fixes actually do a compare-and-swap on a bool, so I
do need an 8-bit CAS. I found this [0] page lists
_InterlockedCompareExchange8. I don't see a non-intrinsic version
though. Can we use this? GCC will generate 8-bit atomic ops on 32- and
64-bit x86, so I don't know of a technical reason it can't work.

[0] http://msdn.microsoft.com/en-us/library/ttk2z1ws.aspx

Also, I'm a little surprised that

+#define p_atomic_dec(_v) \
+ ((void) p_atomic_dec_return(_v))

is sufficient. Are you sure?

> BTW, we could rename the macros to something not allusive to gallium (ie,
> remove "pipe").  (We could even match the C11 stdatomic.h until the C
> runtime provide them, like we're doing with "thread.h".)  Anyway, this is a
> just cosmetic, so it can wait.

That's probably a good idea.