[InstCombine] Don't DSE across readnone functions that may throw

[sanjoy]

Summary

Depends on D28740

[majnemer]

Instead of using I->mayReadFromMemoryOrThrow() || I->mayWriteToMemory(), I'd recommend using I->mayReadFromMemory() || I->mayHaveSideEffects()

mayReadFromMemoryOrThrow is a rather strange predicate...

[sanjoy]

In D28742#646705, @majnemer wrote:

Instead of using I->mayReadFromMemoryOrThrow() || I->mayWriteToMemory(), I'd recommend using I->mayReadFromMemory() || I->mayHaveSideEffects()

What do you think about I->mayReadFromMemory() || I->mayWriteToMemory() || I->mayThrow()? I don't particularly like that we club may-throw with has-side-effects.

[sanjoy]

• Address review: remove usage of mayReadMemoryOrThrow()

[majnemer]

LGTM

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

if (LoadInst *LI = dyn_cast<LoadInst>(BBI)) {
         return eraseInstFromFunction(SI);
       }
 
       // Otherwise, this is a load from some other location.  Stores before it
       // may not be dead.
       break;
     }
 
-    // Don't skip over loads or things that can modify memory.
-    if (BBI->mayWriteToMemory() || BBI->mayReadFromMemory())
+    // Don't skip over loads, throws or things that can modify memory.
+    if (BBI->mayWriteToMemory() || BBI->mayReadFromMemory() || BBI->mayThrow())
       break;
   }
 
   // store X, null    -> turns into 'unreachable' in SimplifyCFG
   if (isa<ConstantPointerNull>(Ptr) && SI.getPointerAddressSpace() == 0) {
     if (!isa<UndefValue>(Val)) {
       SI.setOperand(0, UndefValue::get(Val->getType()));
       if (Instruction *U = dyn_cast<Instruction>(Val))
for (;; --BBI) {
       if ((OtherStore = dyn_cast<StoreInst>(BBI))) {
         if (OtherStore->getOperand(1) != SI.getOperand(1) ||
             !SI.isSameOperationAs(OtherStore))
           return false;
         break;
       }
       // If we find something that may be using or overwriting the stored
       // value, or if we run out of instructions, we can't do the xform.
-      if (BBI->mayReadFromMemory() || BBI->mayWriteToMemory() ||
+      if (BBI->mayReadFromMemory() || BBI->mayThrow() || BBI->mayWriteToMemory() ||
           BBI == OtherBB->begin())
         return false;
     }
 
     // In order to eliminate the store in OtherBr, we have to
     // make sure nothing reads or overwrites the stored value in
     // StoreBB.
     for (BasicBlock::iterator I = StoreBB->begin(); &*I != &SI; ++I) {
       // FIXME: This should really be AA driven.
-      if (I->mayReadFromMemory() || I->mayWriteToMemory())
+      if (I->mayReadFromMemory() || I->mayThrow() || I->mayWriteToMemory())
         return false;
     }
   }
 
   // Insert a PHI node now if we need it.
   Value *MergedVal = OtherStore->getOperand(0);
   if (MergedVal != SI.getOperand(0)) {
     PHINode *PN = PHINode::Create(MergedVal->getType(), 2, "storemerge");

test/Transforms/InstCombine/readnone-maythrow.ll

+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+declare void @readnone_but_may_throw() readnone
+
+define void @f_0(i32* %ptr) {
+; CHECK-LABEL: @f_0(
+entry:
+; CHECK:  store i32 10, i32* %ptr
+; CHECK-NEXT:  call void @readnone_but_may_throw()
+; CHECK-NEXT:  store i32 20, i32* %ptr, align 4
+; CHECK:  ret void
+
+  store i32 10, i32* %ptr
+  call void @readnone_but_may_throw()
+  store i32 20, i32* %ptr
+  ret void
+}
+
+define void @f_1(i1 %cond, i32* %ptr) {
+; CHECK-LABEL: @f_1(
+; CHECK:  store i32 10, i32* %ptr
+; CHECK-NEXT:  call void @readnone_but_may_throw()
+
+  store i32 10, i32* %ptr
+  call void @readnone_but_may_throw()
+  br i1 %cond, label %left, label %merge
+
+left:
+  store i32 20, i32* %ptr
+  br label %merge
+
+merge:
+  ret void
+}