[DSE] fix - missing store to runtime stack in thunk with tail call bvval arg

[Gerolf]

Summary

In a thunk DSE deletes a store to the stack. This causes an incorrect
parameter value (for a byval argument) to be passed on to a tail call.

This exposes a corner case that requires clarification. I believe for a byval in
a tail call, the pass by value semantics of byval holds but not the ‘owned by
caller’ aspect of it. That part does not make sense for tail call. And this
justifies the code in getModRefInfo. DSE looks at the parameter list of the
call, but asks whether or not the call could mod ref a stack location. Since
this is a tail call I think mod ref analysis correctly asserts “ no mod ref “.
Here is the relevant code in BasicAliasAnalysis.cpp/getModRefInfo():

If this is a tail call and Loc.Ptr points to a stack location, we know that
the tail call cannot access or modify the local stack.
We cannot exclude byval arguments here; these belong to the caller of
the current function not to the current function, and a tail callee
// may reference them.
if (isa<AllocaInst>(Object))

if (const CallInst *CI = dyn_cast<CallInst>(CS.getInstruction()))
  if (CI->isTailCall())
    return MRI_NoModRef;

The information DSE misses out on is whether the call arguments need values from
the runtime stack. The patch is adding that check. Does anyone have different
opinion about the fix?

Also, FWIW, I think a tail call with a byval argument can happen only in a
thunk. That assertion is missing yet.

[eli.friedman]

Special-casing this in DSE makes no sense... alias analysis should always return the correct result.

In this case, alias analysis is correct; the definition of "tail" doesn't allow marking this call. If clang is generating code like this, it's a bug in clang.

[Gerolf]

Ahmed pointed out the clang commit causing the issue is r244207 - Mark calls in thunk functions as tail-call optimization candidates

The purpose of the fix was to prevent thunks from showing up on the call stack.

It seems that this patch overlooked the implications of tail calls with byval parameters and should be backed out. Any objections?

Thanks
Gerolf

[reames]

Appears to be a stale revision. Feel free to readd as reviewer if further review needed.

lib/Transforms/Scalar/DeadStoreElimination.cpp

if (auto CS = CallSite(&*BBI)) {
         return A == MRI_ModRef || A == MRI_Ref;
       });
 
       // If all of the allocas were clobbered by the call then we're not going
       // to find anything else to process.
       if (DeadStackObjects.empty())
         break;
 
+      // Check a corner case: when a tail call has a byval parameter,
+      // getModRefInfo report no mod ref for the call site. The reason
+      // is that a tail call cannot reference callers local memory nor
+      // can the caller own the byval of a tail call (which the caller
+      // by definiton of byval is supposed to do. But since the tail does not
+      // return getModRefInfo does not check that byval condition for tail
+      // calls).
+      // But DSE cannot remove a store to local memory when a byval argument of
+      // a tail call needs it.
+      if (const CallInst *CI = dyn_cast<CallInst>(CS.getInstruction()))
+        if (CI->isTailCall()) {
+          bool PassedAsArg = false;
+          unsigned OperandNo = 0;
+          for (auto Arg = CS.data_operands_begin(), CE = CS.data_operands_end();
+               Arg != CE; ++Arg, ++OperandNo) {
+            if ((*Arg)->getType()->isPointerTy() &&
+                CS.isByValArgument(OperandNo))
+              if (std::find_if(DeadStackObjects.begin(), DeadStackObjects.end(),
+                               [&](Value *I) {
+                    // Does the call site touch the value?
+                    AliasResult AR = AA->alias(
+                        MemoryLocation(*Arg),
+                        MemoryLocation(I, getPointerSize(I, DL, *TLI)));
+                    return AR != NoAlias;
+                    ;
+                  }))
+                PassedAsArg = true;
+          }
+          if (PassedAsArg)
+            break;
+        }
+
       continue;
     }
 
     MemoryLocation LoadedLoc;
 
     // If we encounter a use of the pointer, it is no longer considered dead
     if (LoadInst *L = dyn_cast<LoadInst>(BBI)) {
       if (!L->isUnordered()) // Be conservative with atomic/volatile load

test/Transforms/DeadStoreElimination/tail-call-with-byval-param.ll

+; RUN: opt < %s -basicaa -dse -S | FileCheck %s
+target datalayout = "e-m:o-p:32:32-f64:32:64-f80:128-n8:16:32-S128"
+target triple = "i386-apple-macosx10.11.0"
+
+%class.IReproHeapDumb = type { i8 }
+%class.CReproHeapDumbObject = type { i32 (...)**, %class.IReproHeapDumb* }
+%class.IStreamDumb = type { i32 (...)** }
+%class.CByteStreamWrapperBase = type { %class.CReproHeapDumbObject, i64, i32, [4 x i8] }
+%class.CByteStreamToIStream = type { %class.CByteStreamWrapperBase.base, %class.IStreamDumb }
+%class.CByteStreamWrapperBase.base = type { %class.CReproHeapDumbObject, i64, i32 }
+%union._LARGE_INTEGER2 = type { %struct.anon }
+%struct.anon = type { i32, i32 }
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+declare void @llvm.dbg.value(metadata, i64)
+declare i32 @_ZN20CByteStreamToIStream4SeekE15_LARGE_INTEGER2jPS0_(%class.CByteStreamToIStream* %this, %union._LARGE_INTEGER2* align 4, i32 %dwOrigin, %union._LARGE_INTEGER2* %plibNewPosition)
+
+define i32
+@_ZThn20_N20CByteStreamToIStream4SeekE15_LARGE_INTEGER2jPS0_(%class.CByteStreamToIStream* %this, %union._LARGE_INTEGER2* byval align 4, i32 %dwOrigin, %union._LARGE_INTEGER2* %plibNewPosition)  {
+entry:
+  %dlibMove = alloca %union._LARGE_INTEGER2, align 8
+  %v1 = bitcast %union._LARGE_INTEGER2* %0 to i64*
+  %v2 = bitcast %union._LARGE_INTEGER2* %dlibMove to i64*
+  %v3 = load i64, i64* %v1, align 4
+; CHECK: store i64 %v3, i64* %v2
+  store i64 %v3, i64* %v2, align 8
+  call void @llvm.dbg.value(metadata %class.CByteStreamToIStream* %this, i64 0)
+  call void @llvm.dbg.value(metadata i32 %dwOrigin, i64 0)
+  call void @llvm.dbg.value(metadata %union._LARGE_INTEGER2* %plibNewPosition, i64 0)
+  %v4 = getelementptr inbounds %class.CByteStreamToIStream, %class.CByteStreamToIStream* %this, i32 -1, i32 0, i32 0, i32 1
+  %v5 = bitcast %class.IReproHeapDumb** %v4 to %class.CByteStreamToIStream*
+  call void @llvm.dbg.value(metadata %union._LARGE_INTEGER2* %dlibMove, i64 0)
+  %call = tail call i32 @_ZN20CByteStreamToIStream4SeekE15_LARGE_INTEGER2jPS0_(%class.CByteStreamToIStream* %v5, %union._LARGE_INTEGER2* nonnull byval  align 4 %dlibMove, i32 %dwOrigin, %union._LARGE_INTEGER2* %plibNewPosition) 
+  ret i32 %call
+}