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

Jose Fonseca jfonseca at vmware.com
Tue Nov 25 13:42:13 PST 2014

On 25/11/14 21:04, Matt Turner wrote:
> 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
>>    https://urldefense.proofpoint.com/v2/url?u=http-3A__cgit.freedesktop.org_-7Ejrfonseca_mesa_log_-3Fh-3Du-5Fatomic&d=AAIFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=8QJFF29dW3a3z8eyBjuTu3qPR2fyjPrA74i0KYdqcfY&s=VuQNqF0RPRZ7KG-MqTPY5S8_b_yShiJ6ddvx_9Y9LT0&e=
>> 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] https://urldefense.proofpoint.com/v2/url?u=http-3A__msdn.microsoft.com_en-2Dus_library_ttk2z1ws.aspx&d=AAIFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=8QJFF29dW3a3z8eyBjuTu3qPR2fyjPrA74i0KYdqcfY&s=Qkw_NXkDNtKz2k3ULg5JWRJNL0PhrQFGAAbtcNrjz-0&e=

You're right.  I was confused because I didn't find a corresponding 
InterlockedCompareExchange8() in the Win32 API, but the 
underscore-prefixed intrinsic itself exists and works fine.  I've 
brought it back with:


> Also, I'm a little surprised that
> +#define p_atomic_dec(_v) \
> + ((void) p_atomic_dec_return(_v))
> is sufficient. Are you sure?

Could you elaborate your concern?

The `void` return type is the only difference between 
p_atomic_{inc,dec}_return and p_atomic_{inc,dec}, even for 
PIPE_ATOMIC_GCC_INTRINSIC case.   In fact we should probably drop the 
the void case completely, since it seems a pointless special case.

If it is the parenthesis around the (void) cast you're worried about, it 
is fine.  See e.g., the assert macro on Linux:

$ cat /usr/include/assert.h
#if defined __cplusplus && __GNUC_PREREQ (2,95)
# define __ASSERT_VOID_CAST static_cast<void>
# define __ASSERT_VOID_CAST (void)
# define assert(expr)		(__ASSERT_VOID_CAST (0))


More information about the mesa-dev mailing list