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

[JF Bastien via llvm-commits]

jfb added a comment.

Looks good with the 2 suggested updates (assert, and extra check in test).


================
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);
----------------
reames wrote:
> 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?
Ah yes, you're totally right! Could you `assert(SI->isUnordered() && "can't eliminate volatile or ordered store");` here? I know it's checked above, but the intent seems to be to relax this later so the assert will prevent sadness.


http://reviews.llvm.org/D15354