[PATCH v5 2/3] server: Add USD support with timestamp updates.
Rémi Bernon
rbernon at codeweavers.com
Sat May 9 07:08:07 CDT 2020
On 5/9/20 1:11 PM, Jacek Caban wrote:
> On 09.05.2020 11:22, Rémi Bernon wrote:
>> On 5/9/20 1:18 AM, Jacek Caban wrote:
>>> On 08.05.2020 22:12, Alexandre Julliard wrote:
>>>> Rémi Bernon <rbernon at codeweavers.com> writes:
>>>>
>>>>> On 5/7/20 10:45 PM, Alexandre Julliard wrote:
>>>>>> Rémi Bernon <rbernon at codeweavers.com> writes:
>>>>>>
>>>>>>> +#if defined(__GNUC__) && ((__GNUC__ > 4) || ((__GNUC__ == 4) &&
>>>>>>> (__GNUC_MINOR__ >= 7)))
>>>>>>> + __atomic_store_n(&ptr->SystemTime.High2Time,
>>>>>>> system_time_high, __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->SystemTime.LowPart, system_time_low,
>>>>>>> __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->SystemTime.High1Time,
>>>>>>> system_time_high, __ATOMIC_SEQ_CST);
>>>>>>> +
>>>>>>> + __atomic_store_n(&ptr->InterruptTime.High2Time,
>>>>>>> interrupt_time_high, __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->InterruptTime.LowPart,
>>>>>>> interrupt_time_low, __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->InterruptTime.High1Time,
>>>>>>> interrupt_time_high, __ATOMIC_SEQ_CST);
>>>>>>> +
>>>>>>> + __atomic_store_n(&ptr->TickCount.High2Time, tick_count_high,
>>>>>>> __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->TickCount.LowPart, tick_count_low,
>>>>>>> __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->TickCount.High1Time, tick_count_high,
>>>>>>> __ATOMIC_SEQ_CST);
>>>>>>> + __atomic_store_n(&ptr->TickCountLowDeprecated, tick_count_low,
>>>>>>> __ATOMIC_SEQ_CST);
>>>>>> Is this gcc-specific code really necessary?
>>>>>>
>>>>> I'm not sure it is strictly necessary but then it depends what
>>>>> guarantees we want to have. The volatile qualifier only acts as a
>>>>> compiler barrier and will make sure that the stores aren't optimized
>>>>> or reordered by the compiler. However it doesn't enforce anything on
>>>>> the CPU. I believe that X86 has global store ordering guarantees, but
>>>>> a quick search tells me that ARM or POWER don't, so the stores to the
>>>>> various members may be seen out of order from another CPU depending on
>>>>> the architecture.
>>>> My concern is that if the code is necessary, it means that the fallback
>>>> path for older gccs would yield a broken implementation. But if the
>>>> fallback works correctly on x86 that's probably good enough.
>>>
>>>
>>> The check should probably be adjusted for clang through. It pretends
>>> to be GCC 4.2 and supports __atomic_store_n (and is a required
>>> compiler on aarch64).
>>>
>>>
>>> Jacek
>>>
>>
>> So would it be acceptable to extend the check for clang and simply
>> error in the case where __atomic builtins aren't supported? Or should
>> we keep the fallback but check that it's on X86 only?
>>
>> There's also a possible intermediate but heavier fallback, usable on
>> clang 3.0 or gcc >= 4.1, with __sync_synchronize, but still GCC-like
>> specific again.
>>
>> Or, do the complete portable implementation with the asm variants for
>> all the architectures we need.
>
>
> The implementation and use of __atomic_store_n is fine, it's just values
> of __GNUC__ and __GNUC_MINOR__ that we can't trust on clang, so it's
> only #if that could be improved. In practice, you could probably just
> add "|| defined(__clang__)" there. There is
> also__has_builtin(__atomic_store_n) that could be used, but I'm not sure
> it's worth it. We could also avoid dealing with hardcoded compiler
> version and use a configure check.
>
Yes, I'm planning on adding a __clang__ version check, as some quick
test tells me that __atomic builtins are only supported for clang >= 3.1.
But then, as the fallback isn't very reliable and depends on the
guarantees that the target architecture provides, it can either simply
be completely unsupported and report a compilation error, or we can try
to be a bit clever and have it only for X86 (but that may not be a good
idea).
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list