[MemoryLocation] Don't require nocapture in getForDest()

[nikic]

Summary

As @reames mentioned on related reviews, we don't need the nocapture requirement here. First of all, from an API perspective, this is not something that MemoryLocation::getForDest() should be checking in the first place, because it does not affect which memory this particular call can access; it's an orthogonal concern that should be handled by the caller if necessary.

However, for both of the motivating users in DSE and InstCombine, we don't need the nocapture requirement, because the capture can either be purely local to the call (a pointer identity check that is irrelevant to us), be part of the return value (which we check is unused), or be written in the dest location, which we have determined to be dead.

This allows us to remove the special handling for libcalls as well.

[fhahn]

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

231

The capture can only be in the return value

There is no return value in the test :)

Should we add a variant of the test where either the first argument is not local or there's a return value? I guess in both cases we won't remove for non-capture related reasons, but still might be good to have here for completeness

[nikic]

Add tests where return value is used.

[fhahn]

LGTM, thanks!

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

223

nit: %bitcast2 may by captured via the return value.

[reames]

Thanks for following up on this. For the record, LGTM by me as well. :)

llvm/lib/Analysis/MemoryLocation.cpp

default:
       return None;
     case Intrinsic::init_trampoline:
       return MemoryLocation::getForArgument(CB, 0, TLI);
     case Intrinsic::masked_store:
       return MemoryLocation::getForArgument(CB, 1, TLI);
     }
   }
 
-  LibFunc LF;
-  if (TLI.getLibFunc(*CB, LF) && TLI.has(LF)) {
-    switch (LF) {
-    case LibFunc_strncpy:
-    case LibFunc_strcpy:
-    case LibFunc_strcat:
-    case LibFunc_strncat:
-      return getForArgument(CB, 0, &TLI);
-    default:
-      break;
-    }
-  }
-
   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;
     }

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

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -dse -S < %s | FileCheck %s
 
 declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
 
 declare void @unknown()
 declare void @f(i8*)
 declare void @f2(i8*, i8*)
+declare i8* @f3(i8*, i8*)
 
 ; Basic case for DSEing a trivially dead writing call
 define void @test_dead() {
 ; CHECK-LABEL: @test_dead(
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
;
   %a = alloca i32, align 4
   %a2 = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
   %bitcast2 = bitcast i32* %a2 to i8*
   call void @f2(i8* nocapture writeonly %bitcast, i8* nocapture readonly %bitcast2) argmemonly nounwind willreturn
   ret void
 }
 
-; But that removing a capture of an unrelated pointer isn't okay.
-define void @test_neg_unreleated_capture() {
-; CHECK-LABEL: @test_neg_unreleated_capture(
+; Removing a capture is also okay. The capture can only be in the return value
+; (which is unused) or written into the dead out parameter.
+define void @test_unrelated_capture() {
+; CHECK-LABEL: @test_unrelated_capture(
+; CHECK-NEXT:    ret void
+;
+  %a = alloca i32, align 4
+  %a2 = alloca i32, align 4
+  %bitcast = bitcast i32* %a to i8*
+  %bitcast2 = bitcast i32* %a2 to i8*
+  call i8* @f3(i8* nocapture writeonly %bitcast, i8* readonly %bitcast2) argmemonly nounwind willreturn
+  ret void
+}
+
+; Cannot remove call, as %bitcast2 is captured via the return value.
+define i8 @test_neg_unrelated_capture_used_via_return() {
+; CHECK-LABEL: @test_neg_unrelated_capture_used_via_return(
 ; 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* readonly [[BITCAST2]]) #[[ATTR1]]
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    [[CAPTURE:%.*]] = call i8* @f3(i8* nocapture writeonly [[BITCAST]], i8* readonly [[BITCAST2]]) #[[ATTR1]]
+; CHECK-NEXT:    [[V:%.*]] = load i8, i8* [[CAPTURE]], align 1
+; CHECK-NEXT:    ret i8 [[V]]
 ;
   %a = alloca i32, align 4
   %a2 = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
   %bitcast2 = bitcast i32* %a2 to i8*
-  call void @f2(i8* nocapture writeonly %bitcast, i8* readonly %bitcast2) argmemonly nounwind willreturn
-  ret void
+  %capture = call i8* @f3(i8* nocapture writeonly %bitcast, i8* readonly %bitcast2) argmemonly nounwind willreturn
+  %v = load i8, i8* %capture
+  ret i8 %v
 }
 
 ; As long as the result is unused, we can even remove reads of the alloca
 ; itself since the write will be dropped.
 define void @test_self_read() {
 ; CHECK-LABEL: @test_self_read(
 ; CHECK-NEXT:    ret void
 ;

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

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -instcombine -S < %s | FileCheck %s
 
 declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
 
 declare void @unknown()
 declare void @f(i8*)
 declare void @f2(i8*, i8*)
+declare i8* @f3(i8*, i8*)
 
 ; Basic case for DSEing a trivially dead writing call
 define void @test_dead() {
 ; CHECK-LABEL: @test_dead(
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
;
   %a = alloca i32, align 4
   %a2 = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
   %bitcast2 = bitcast i32* %a2 to i8*
   call void @f2(i8* nocapture writeonly %bitcast, i8* nocapture readonly %bitcast2) argmemonly nounwind willreturn
   ret void
 }
 
-; But that removing a capture of an unrelated pointer isn't okay.
-define void @test_neg_unreleated_capture() {
-; CHECK-LABEL: @test_neg_unreleated_capture(
+; Removing a capture is also okay. The capture can only be in the return value
+; (which is unused) or written into the dead out parameter.
+define void @test_unrelated_capture() {
+; CHECK-LABEL: @test_unrelated_capture(
+; CHECK-NEXT:    ret void
+;
+  %a = alloca i32, align 4
+  %a2 = alloca i32, align 4
+  %bitcast = bitcast i32* %a to i8*
+  %bitcast2 = bitcast i32* %a2 to i8*
+  call i8* @f3(i8* nocapture writeonly %bitcast, i8* readonly %bitcast2) argmemonly nounwind willreturn
+  ret void
+}
+
+; Cannot remove call, as %bitcast2 is captured via the return value.
+define i8 @test_neg_unrelated_capture_used_via_return() {
+; CHECK-LABEL: @test_neg_unrelated_capture_used_via_return(
 ; 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 nonnull writeonly [[BITCAST]], i8* nonnull readonly [[BITCAST2]]) #[[ATTR1]]
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    [[CAPTURE:%.*]] = call i8* @f3(i8* nocapture nonnull writeonly [[BITCAST]], i8* nonnull readonly [[BITCAST2]]) #[[ATTR1]]
+; CHECK-NEXT:    [[V:%.*]] = load i8, i8* [[CAPTURE]], align 1
+; CHECK-NEXT:    ret i8 [[V]]
 ;
   %a = alloca i32, align 4
   %a2 = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
   %bitcast2 = bitcast i32* %a2 to i8*
-  call void @f2(i8* nocapture writeonly %bitcast, i8* readonly %bitcast2) argmemonly nounwind willreturn
-  ret void
+  %capture = call i8* @f3(i8* nocapture writeonly %bitcast, i8* readonly %bitcast2) argmemonly nounwind willreturn
+  %v = load i8, i8* %capture
+  ret i8 %v
 }
 
 ; As long as the result is unused, we can even remove reads of the alloca
 ; itself since the write will be dropped.
 define void @test_self_read() {
 ; CHECK-LABEL: @test_self_read(
 ; CHECK-NEXT:    ret void
 ;
   %a = alloca i32, align 4
   %bitcast = bitcast i32* %a to i8*
   call void @f2(i8* nocapture writeonly %bitcast, i8* nocapture readonly %bitcast) argmemonly nounwind willreturn
   ret void
 }