[PATCH] D15352: [EarlyCSE] DSE of atomic unordered stores

[JF Bastien via llvm-commits]

jfb added a comment.

Two comment edits, then lgtm.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:718
@@ +717,3 @@
+        // At the moment, we don't remove ordered stores, but do remove
+        // unordered atomic stores.  There's no special requirement about
+        // removing atomic stores only in favor of other atomic stores since we
----------------
I'd clarify: "there's no special requirement **for unordered stores** about removing atomic stores only".

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:753
@@ +752,3 @@
+        // since fences are slightly stronger than stores in their ordering,
+        // it's not clear this is a profitable transform.
+        if (MemInst.isUnordered() && !MemInst.isVolatile())
----------------
Could you also mention that we could "sink" the atomicity to all post-dominating stores? This is just documentation, I don't advocate that we do so at the moment since it's similar to @morisset's [fence elimination proposal](http://lists.llvm.org/pipermail/llvm-dev/2014-September/076701.html).


http://reviews.llvm.org/D15352