[PATCH] D54291: [XRay] Add atomic fences around non-atomic reads and writes

[JF Bastien via llvm-commits]

> On Nov 18, 2018, at 10:47 PM, Dean Michael Berris via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
> 
>> On 10 Nov 2018, at 15:41, JF Bastien via Phabricator via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> 
>> jfb added a comment.
>> 
>> In https://reviews.llvm.org/D54291#1293418, @efriedma wrote:
>> 
>>> What is the point of this change?  The atomic_fetch_add operations already use memory_order_acq_rel, so the explicit fences are redundant.
>> 
>> 
>> Agreed, these are all redundant. Ideally the compiler just eliminates the fences, and your change was a no-op :-)
>> 
> 
> Is it guaranteed though that non-atomic operations before the memory_order_acq_rel will have a “happens-before” relationship? It wasn’t clear that the memcpy’s before the atomic operation couldn’t be non-temporal writes (write-coalesced), and that the fences were necessary to ensure that those writes committed before the atomic operation was performed.
> 
> Is there something in the standard that I can look at to convince myself these fences are unnecessary?
> 
> Note also that these are not the same as the std::atomic<…> and family of operations (these are defined in the sanitiser common library).

Buffer.Extents is what you’re using to enforce happens-before. Your adds do the release (they “publish” the updates), and your load performs the acquire (it therefore sees all that was published by the releases that it synchronizes with). Non-temporals have to follow that too, so on some ISAs the implementation has to add other fences such as x86’s lfence / sfence. The same is true of e.g. ARM’s variable-width vector extensions, but that’s all extra ISA details outside of the C++ memory model.