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

[Matt Turner]

On Tue, Nov 25, 2014 at 1:42 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
> 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

Awesome, thanks!

>> 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?

Oh, I'm sorry -- I totally misread. Yeah, that seems completely fine.