[PATCH] D12338: Make `llvm::expandAtomicRMWToCmpXchg`'s initial load atomic.

[Robin Morisset via llvm-commits]

morisset added a comment.

In http://reviews.llvm.org/D12338#232597, @jfb wrote:

> This isn't just for PNaCl, so the description should be updated. You should mention something along the lines of:
>
> > On weak memory systems the CPU can speculate on subsequent loads (e.g. the `cmpxchg`) and observe them without honoring the happens-before ordering of the corresponding stores. This is the "load buffering" problem in literature, and occurs on ARM and POWER.
>


I don't completely understand your argument here. The load buffering pattern (LB) occurs where there is a load to a location, followed by a store to a different location (and I agree that in that case the store can be executed before the load on ARM/Power). Here there is a load followed by a cmpxchg to the same location. No cache-coherent processor I know of can do anything tricky in such a case (accesses to the same location are always globally ordered, and in a way compatible with the program order). The only thing that I could see occurring is for the load to return stale data, which would only lead the cmpxchg to fail, and to an extra iteration of the loop. Am I missing something (likely considering it is 1 am) ?

So I agree that it deserves a comment to explain why it is safe but I still believe the original code safe; and I would rather avoid these extra cmpxchg8b/16b your change introduces unless they are necessary.

Extra remark: if the risk is indeed a LB pattern, then x86 is immune to it anyway, which makes me doubt the utility of these tests. Although in that case I don't have any better ones to suggest, since x86 is the only backend to exercise this function.


http://reviews.llvm.org/D12338