[MemCpyOpt] Use dereferenceable pointer helper

[nikic]

Summary

The call slot optimization has some home-grown code for checking whether the destination is dereferenceable. Replace this with the generic isDereferenceableAndAlignedPointer() helper.

I'm not checking alignment here, because that is currently handled separately and may be an enforced alignment for allocas. The clean way of integrating that part would probably be to accept a callback in isDereferenceableAndAlignedPointer() for the actual isAligned check, which would then have a chance to use an enforced alignment instead.

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

867

By the way, is there any existing API for this? That is, to move an instruction and its dependencies up, for trivially speculatable cases. I've seen CodeMoverUtils, but that's a very big hammer (even requiring PDT).

[efriedma]

LGTM

We could probably clean up the alignment handling in this pass now that the src and dest alignment of memcpy are separate.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

817

Not really related to this patch, but is mayThrow actually the right check? Do we need willreturn instead?

[efriedma]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

867

I can't think of anything off the top of my head. You wouldn't usually want to speculate an arbitrary number of instructions without some cost model, anyway, and then it gets really complicated.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/GlobalsModRef.h"
+#include "llvm/Analysis/Loads.h"
 #include "llvm/Analysis/MemoryDependenceAnalysis.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/MemorySSA.h"
 #include "llvm/Analysis/MemorySSAUpdater.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/BasicBlock.h"
uint64_t srcSize = DL.getTypeAllocSize(srcAlloca->getAllocatedType()) *
                      srcArraySize->getZExtValue();
 
   if (cpyLen < srcSize)
     return false;
 
   // Check that accessing the first srcSize bytes of dest will not cause a
   // trap.  Otherwise the transform is invalid since it might cause a trap
   // to occur earlier than it otherwise would.
-  // TODO: Use isDereferenceablePointer() API instead.
-  if (AllocaInst *A = dyn_cast<AllocaInst>(cpyDest)) {
-    // The destination is an alloca.  Check it is larger than srcSize.
-    ConstantInt *destArraySize = dyn_cast<ConstantInt>(A->getArraySize());
-    if (!destArraySize)
-      return false;
+  if (!isDereferenceableAndAlignedPointer(cpyDest, Align(1), APInt(64, cpyLen),
+                                          DL, C, DT))
 
-    uint64_t destSize = DL.getTypeAllocSize(A->getAllocatedType()) *
-                        destArraySize->getZExtValue();
-
-    if (destSize < srcSize)
-      return false;
-  } else if (Argument *A = dyn_cast<Argument>(cpyDest)) {
-    if (A->getDereferenceableBytes() < srcSize) {
-      // If the destination is an sret parameter then only accesses that are
-      // outside of the returned struct type can trap.
-      if (!A->hasStructRetAttr())
-        return false;
-
-      Type *StructTy = A->getParamStructRetType();
-      if (!StructTy->isSized()) {
-        // The call may never return and hence the copy-instruction may never
-        // be executed, and therefore it's not safe to say "the destination
-        // has at least <cpyLen> bytes, as implied by the copy-instruction",
-        return false;
-      }
-
-      uint64_t destSize = DL.getTypeAllocSize(StructTy);
-      if (destSize < srcSize)
-        return false;
-    }
-  } else {
-    return false;
-  }
-
   // Make sure that nothing can observe cpyDest being written early. There are
   // a number of cases to consider:
   //  1. cpyDest cannot be accessed between C and cpyStore as a precondition of
   //     the transform.
   //  2. C itself may not access cpyDest (prior to the transform). This is
   //     checked further below.
   //  3. If cpyDest is accessible to the caller of this function (potentially
   //     captured and not based on an alloca), we need to ensure that we cannot
   //     unwind between C and cpyStore. This is checked here.
   //  4. If cpyDest is potentially captured, there may be accesses to it from
   //     another thread. In this case, we need to check that cpyStore is
   //     guaranteed to be executed if C is. As it is a non-atomic access, it
   //     renders accesses from other threads undefined.
   //     TODO: This is currently not checked.
+  // TODO: Check underlying object, so we can look through GEPs.
   if (!isa<AllocaInst>(cpyDest)) {
     assert(C->getParent() == cpyStore->getParent() &&
            "call and copy must be in the same block");
     for (const Instruction &I : make_range(C->getIterator(),
                                            cpyStore->getIterator())) {
       if (I.mayThrow())
         return false;
     }
   }
 
   // Check that dest points to memory that is at least as aligned as src.
   Align srcAlign = srcAlloca->getAlign();
   bool isDestSufficientlyAligned = srcAlign <= cpyAlign;
   // If dest is not aligned enough and we can't increase its alignment then
   // bail out.
bool MemCpyOptPass::performCallSlotOptzn(Instruction *cpyLoad,
   // Check that src isn't captured by the called function since the
   // transformation can cause aliasing issues in that case.
   for (unsigned ArgI = 0, E = C->arg_size(); ArgI != E; ++ArgI)
     if (C->getArgOperand(ArgI) == cpySrc && !C->doesNotCapture(ArgI))
       return false;
 
   // Since we're changing the parameter to the callsite, we need to make sure
   // that what would be the new parameter dominates the callsite.
+  // TODO: Support moving instructions like GEPs upwards.
   if (Instruction *cpyDestInst = dyn_cast<Instruction>(cpyDest))
     if (!DT->dominates(cpyDestInst, C))
       return false;
 
   // In addition to knowing that the call does not access src in some
   // unexpected manner, for example via a global, which we deduce from
   // the use analysis, we also need to know that it does not sneakily
   // access dest.  We rely on AA to figure this out for us.

llvm/test/Transforms/MemCpyOpt/callslot.ll

 }
 
 define void @dest_is_gep_nounwind_call() {
 ; CHECK-LABEL: @dest_is_gep_nounwind_call(
 ; CHECK-NEXT:    [[DEST:%.*]] = alloca [16 x i8], align 1
 ; CHECK-NEXT:    [[SRC:%.*]] = alloca [8 x i8], align 1
 ; CHECK-NEXT:    [[SRC_I8:%.*]] = bitcast [8 x i8]* [[SRC]] to i8*
 ; CHECK-NEXT:    [[DEST_I8:%.*]] = getelementptr [16 x i8], [16 x i8]* [[DEST]], i64 0, i64 8
-; CHECK-NEXT:    call void @accept_ptr(i8* [[SRC_I8]]) [[ATTR3:#.*]]
-; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[DEST_I8]], i8* [[SRC_I8]], i64 8, i1 false)
+; CHECK-NEXT:    [[DEST_I81:%.*]] = bitcast i8* [[DEST_I8]] to [8 x i8]*
+; CHECK-NEXT:    [[DEST_I812:%.*]] = bitcast [8 x i8]* [[DEST_I81]] to i8*
+; CHECK-NEXT:    call void @accept_ptr(i8* [[DEST_I812]]) [[ATTR3:#.*]]
 ; CHECK-NEXT:    ret void
 ;
   %dest = alloca [16 x i8]
   %src = alloca [8 x i8]
   %src.i8 = bitcast [8 x i8]* %src to i8*
   %dest.i8 = getelementptr [16 x i8], [16 x i8]* %dest, i64 0, i64 8
   call void @accept_ptr(i8* %src.i8) nounwind
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dest.i8, i8* %src.i8, i64 8, i1 false)