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

[Jose Fonseca]

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:

http://cgit.freedesktop.org/~jrfonseca/mesa/commit/?h=u_atomic

> 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>
#else
# define __ASSERT_VOID_CAST (void)
#endif
...
# define assert(expr)		(__ASSERT_VOID_CAST (0))


Jose