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

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1089
@@ -1088,4 +1088,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:
> An ordered load has ordering semantics that you'd be losing. The *value* stored can be eliminated, but you still need to keep atomic *ordering*.
> 
> And you just can't remove `volatile` :-)
> 
> Though it looks like the write-back tests are already correct? I'm confused now.
@JF - I'm 99% certain you're misreading the code and my comments.  We're *only* removing the *store* here.  The *load* is not being changed.  Whether the load is ordered/volatile does not effect the legality of removing the store.  In fact, this exact case is covered by the tests @write_back6 and @write_back7.

Or, am I missing something?

================
Comment at: test/Transforms/InstCombine/store.ll:127
@@ +126,3 @@
+; same location, then the non-atomic one is undefined if we're actual
+; racing.  As such, we're free to pick either store under the assumption
+; that we're not racing with any other thread.
----------------
jfb wrote:
> We should at least check that the latest value gets stored (here and below).
Will do.  I also need to update the comment slightly.  The store is well defined; it's the *contents* of the memory location which isn't.  The rest is still correct, but the wording was misleading.  


http://reviews.llvm.org/D15354