[PATCH] D119844: [MemoryDependency] Relax the re-ordering of atomic store and unordered load/store

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:565
           return MemDepResult::getClobber(SI);
-        if (SI->getOrdering() != AtomicOrdering::Monotonic)
-          return MemDepResult::getClobber(SI);
+        // Ok, if we are here the guard above guarantee us that
+        // SI is atomic with monotonic ot release semantic and
----------------
reames wrote:
> I don't this comment holds.  We can get here with SI being e.g. a ceq_cst store.  Reordering that with just about anything is illegal.
> 
> The prior change to check is non-unordered instead of non-simple looks reasonable, but the change below doesn't seem to follow.  
Thank you, Philip for starting review.

I tend to disagree with you. This is how I read specs.
Lang Ref:
seq_cst (sequentially consistent)
In addition to the guarantees of acq_rel (acquire for an operation that only reads, release for an operation that only writes), there is a global total order on all sequentially-consistent operations on all addresses, which is consistent with the happens-before partial order and with the modification orders of all the affected addresses. Each sequentially-consistent read sees the last preceding write to the same address in this global order. This corresponds to the C++0x/C1x memory_order_seq_cst and Java volatile.

c++(https://en.cppreference.com/w/c/atomic/memory_order):
memory_order_seq_cst	A load operation with this memory order performs an acquire operation, a store performs a release operation, and read-modify-write performs both an acquire operation and a release operation, plus a single total order exists in which all threads observe all modifications in the same order (see Sequentially-consistent ordering below)

So Store with seq_cst is a Store Release and prohibit re-ordering with any another seq_sct instruction.

Do I miss anything?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119844/new/

https://reviews.llvm.org/D119844