[PATCH] D15354: [InstCombine] Extend peephole DSE to handle unordered atomics

[Philip Reames via llvm-commits]

reames added a comment.

replying to JF's comments, updated diff to follow


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1031
@@ -1030,4 +1030,3 @@
     if (LoadInst *LI = dyn_cast<LoadInst>(BBI)) {
-      if (LI == Val && equivalentAddressValues(LI->getOperand(0), Ptr) &&
-          LI->isSimple())
+      if (LI == Val && equivalentAddressValues(LI->getOperand(0), Ptr))
         return EraseInstFromFunction(SI);
----------------
jfb wrote:
> Doesn't this need to test for `&& LI->isUnordered()`? Otherwise it can remove a `volatile` or ordered load.
Nope.  We can remove the store even if the load is ordered.  Removing the load wouldn't be valid, but removing the store is.  

================
Comment at: test/Transforms/InstCombine/store.ll:127
@@ +126,3 @@
+; CHECK-LABEL: dse2
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
----------------
jfb wrote:
> Same as in D15352: I think the remaining store should still be atomic, and store the latest value.
Same as in that review, the second store is undefined behaviour if access is racy.  The location is no longer atomic and we can pick either store.

================
Comment at: test/Transforms/InstCombine/store.ll:136
@@ +135,3 @@
+; CHECK-LABEL: dse3
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
----------------
jfb wrote:
> Same.
same answer

================
Comment at: test/Transforms/InstCombine/store.ll:196
@@ +195,3 @@
+
+; Can't remove store due to ordering side effect
+define void @write_back5(i32* %p) {
----------------
jfb wrote:
> We could just have a fence, or make it a no-op RMW to the stack top? Maybe worth documenting, not implementing yet.
This is documented in EarlyCSE already.  I don't see the point in duplicating this in a bunch of places.


http://reviews.llvm.org/D15354