Revert "[DSE] Remove calls with known writes to dead memory"

This reverts commit a8a51fe.

This breaks the strncpy-overflow.cpp test case.

llvm/lib/Analysis/MemoryLocation.cpp

@@ -147,44 +147,7 @@ MemoryLocation::getForDest(const CallBase *CB, const TargetLibraryInfo &TLI) {
     }
   }
 
-  if (!CB->onlyAccessesArgMemory())
-    return None;
-
-  if (CB->hasOperandBundles())
-    // TODO: remove implementation restriction
-    return None;
-
-  Value *UsedV = nullptr;
-  Optional<unsigned> UsedIdx;
-  for (unsigned i = 0; i < CB->arg_size(); i++) {
-    if (!CB->getArgOperand(i)->getType()->isPointerTy())
-      continue;
-    if (!CB->doesNotCapture(i))
-      // capture would allow the address to be read back in an untracked manner
-      return None;
-     if (CB->onlyReadsMemory(i))
-       continue;
-    if (!UsedV) {
-      // First potentially writing parameter
-      UsedV = CB->getArgOperand(i);
-      UsedIdx = i;
-      continue;
-    }
-    UsedIdx = None;
-    if (UsedV != CB->getArgOperand(i))
-      // Can't describe writing to two distinct locations.
-      // TODO: This results in an inprecision when two values derived from the
-      // same object are passed as arguments to the same function.
-      return None;
-  }
-  if (!UsedV)
-    // We don't currently have a way to represent a "does not write" result
-    // and thus have to be conservative and return unknown.
-    return None;
-
-  if (UsedIdx)
-    return getForArgument(CB, *UsedIdx, &TLI);
-  return MemoryLocation::getBeforeOrAfter(UsedV, CB->getAAMetadata());
+  return None;
 }
 
 MemoryLocation MemoryLocation::getForArgument(const CallBase *Call,

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

@@ -2562,24 +2562,35 @@ static bool isNeverEqualToUnescapedAlloc(Value *V, const TargetLibraryInfo &TLI,
 
 /// Given a call CB which uses an address UsedV, return true if we can prove the
 /// call's only possible effect is storing to V.
-static bool isRemovableWrite(CallBase &CB, Value *UsedV,
-                             const TargetLibraryInfo &TLI) {
+static bool isRemovableWrite(CallBase &CB, Value *UsedV) {
   if (!CB.use_empty())
     // TODO: add recursion if returned attribute is present
     return false;
 
-  if (CB.isTerminator())
-    // TODO: remove implementation restriction
+  if (!CB.willReturn() || !CB.doesNotThrow() || !CB.onlyAccessesArgMemory() ||
+      CB.isTerminator())
     return false;
 
-  if (!CB.willReturn() || !CB.doesNotThrow())
+  if (CB.hasOperandBundles())
     return false;
 
-  // If the only possible side effect of the call is writing to the alloca,
-  // and the result isn't used, we can safely remove any reads implied by the
-  // call including those which might read the alloca itself.
-  Optional<MemoryLocation> Dest = MemoryLocation::getForDest(&CB, TLI);
-  return Dest && Dest->Ptr == UsedV;
+  for (unsigned i = 0; i < CB.arg_size(); i++) {
+    if (!CB.getArgOperand(i)->getType()->isPointerTy())
+      continue;
+    if (!CB.doesNotCapture(i))
+      // capture would allow the address to be read back in an untracked manner
+      return false;
+    if (UsedV != CB.getArgOperand(i) && !CB.onlyReadsMemory(i))
+      // A write to another memory location keeps the call live, and thus we
+      // must keep the alloca so that the call has somewhere to write to.
+      // TODO: This results in an inprecision when two values derived from the
+      // same alloca are passed as arguments to the same function.
+      return false;
+    // Note: Both reads from and writes to the alloca are fine.  Since the
+    // result is unused nothing can observe the values read from the alloca
+    // without writing it to some other observable location (checked above).
+  }
+  return true;
 }
 
 static bool isAllocSiteRemovable(Instruction *AI,
@@ -2649,7 +2660,7 @@ static bool isAllocSiteRemovable(Instruction *AI,
           }
         }
 
-        if (isRemovableWrite(*cast<CallBase>(I), PI, TLI)) {
+        if (isRemovableWrite(*cast<CallBase>(I), PI)) {
           Users.emplace_back(I);
           continue;
         }

llvm/test/Transforms/DeadStoreElimination/trivial-dse-calls.ll

@@ -11,6 +11,9 @@ declare void @f2(i8*, i8*)
 ; Basic case for DSEing a trivially dead writing call
 define void @test_dead() {
 ; CHECK-LABEL: @test_dead(
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
+; CHECK-NEXT:    call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1:[0-9]+]]
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
@@ -25,6 +28,7 @@ define void @test_lifetime() {
 ; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
 ; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
 ; CHECK-NEXT:    call void @llvm.lifetime.start.p0i8(i64 4, i8* [[BITCAST]])
+; CHECK-NEXT:    call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1]]
 ; CHECK-NEXT:    call void @llvm.lifetime.end.p0i8(i64 4, i8* [[BITCAST]])
 ; CHECK-NEXT:    ret void
 ;
@@ -44,6 +48,7 @@ define void @test_lifetime2() {
 ; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
 ; CHECK-NEXT:    call void @llvm.lifetime.start.p0i8(i64 4, i8* [[BITCAST]])
 ; CHECK-NEXT:    call void @unknown()
+; CHECK-NEXT:    call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1]]
 ; CHECK-NEXT:    call void @unknown()
 ; CHECK-NEXT:    call void @llvm.lifetime.end.p0i8(i64 4, i8* [[BITCAST]])
 ; CHECK-NEXT:    ret void
@@ -62,6 +67,9 @@ define void @test_lifetime2() {
 ; itself since the write will be dropped.
 define void @test_dead_readwrite() {
 ; CHECK-LABEL: @test_dead_readwrite(
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
+; CHECK-NEXT:    call void @f(i8* nocapture [[BITCAST]]) #[[ATTR1]]
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
@@ -74,7 +82,7 @@ define i32 @test_neg_read_after() {
 ; CHECK-LABEL: @test_neg_read_after(
 ; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
 ; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
-; CHECK-NEXT:    call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1:[0-9]+]]
+; CHECK-NEXT:    call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1]]
 ; CHECK-NEXT:    [[RES:%.*]] = load i32, i32* [[A]], align 4
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
@@ -195,6 +203,11 @@ define i32 @test_neg_captured_before() {
 ; Show that reading from unrelated memory is okay
 define void @test_unreleated_read() {
 ; CHECK-LABEL: @test_unreleated_read(
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[A2:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
+; CHECK-NEXT:    [[BITCAST2:%.*]] = bitcast i32* [[A2]] to i8*
+; CHECK-NEXT:    call void @f2(i8* nocapture writeonly [[BITCAST]], i8* nocapture readonly [[BITCAST2]]) #[[ATTR1]]
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
@@ -227,6 +240,9 @@ define void @test_neg_unreleated_capture() {
 ; itself since the write will be dropped.
 define void @test_self_read() {
 ; CHECK-LABEL: @test_self_read(
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast i32* [[A]] to i8*
+; CHECK-NEXT:    call void @f2(i8* nocapture writeonly [[BITCAST]], i8* nocapture readonly [[BITCAST]]) #[[ATTR1]]
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4