Resolve a long-standing FIXME in memcpyopt.

[resistor]

Summary

Inspecting the downstream use of the cpyAlign, it is clear that
performCallSlotOptzn is expecting it to represent the alignment
of the copy destination, not the minimum of the src and dest
alignments. This patch renames the parameter to make this more
obvious.

I believe this change is NFC, because the downstream code has
alignment checks such that it all works out in the end. I have not
been able to construct a test case that actually triggers a change
in output.

[resistor]

Remove accidentally added cruft.

[resistor]

Actually remove the FIXME

[nikic]

You should be able to test this as follows: https://llvm.godbolt.org/z/ascrvMxPd Both should be equivalent after this change.

[resistor]

Thank you for the test case.

Something strange is occurring here. When I run the test you provided through opt manually, I see the expected outcome. When I add that test to test/Transforms/MemCpyOpt/callslot.ll I don't see the transformation happening.

I need to dig into what's going wrong...

[resistor]

Add testcase

[nikic]

LGTM, thanks!

[resistor]

Rebase and update commit message.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

 }
 
 /// Takes a memcpy and a call that it depends on,
 /// and checks for the possibility of a call slot optimization by having
 /// the call write its result directly into the destination of the memcpy.
 bool MemCpyOptPass::performCallSlotOptzn(Instruction *cpyLoad,
                                          Instruction *cpyStore, Value *cpyDest,
                                          Value *cpySrc, TypeSize cpySize,
-                                         Align cpyAlign, BatchAAResults &BAA,
+                                         Align cpyDestAlign, BatchAAResults &BAA,
                                          std::function<CallInst *()> GetC) {
   // The general transformation to keep in mind is
   //
   //   call @func(..., src, ...)
   //   memcpy(dest, src, ...)
   //
   // ->
   //
bool MemCpyOptPass::performCallSlotOptzn(Instruction *cpyLoad,
   //     TODO: This is currently not checked.
   if (mayBeVisibleThroughUnwinding(cpyDest, C, cpyStore)) {
     LLVM_DEBUG(dbgs() << "Call Slot: Dest may be visible through unwinding\n");
     return false;
   }
 
   // Check that dest points to memory that is at least as aligned as src.
   Align srcAlign = srcAlloca->getAlign();
-  bool isDestSufficientlyAligned = srcAlign <= cpyAlign;
+  bool isDestSufficientlyAligned = srcAlign <= cpyDestAlign;
   // If dest is not aligned enough and we can't increase its alignment then
   // bail out.
   if (!isDestSufficientlyAligned && !isa<AllocaInst>(cpyDest)) {
     LLVM_DEBUG(dbgs() << "Call Slot: Dest not sufficiently aligned\n");
     return false;
   }
 
   // Check that src is not accessed except via the call and the memcpy.  This
bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI) {
   //   c) memcpy from freshly alloca'd space or space that has just started
   //      its lifetime copies undefined data, and we can therefore eliminate
   //      the memcpy in favor of the data that was already at the destination.
   //   d) memcpy from a just-memset'd source can be turned into memset.
   if (auto *MD = dyn_cast<MemoryDef>(SrcClobber)) {
     if (Instruction *MI = MD->getMemoryInst()) {
       if (auto *CopySize = dyn_cast<ConstantInt>(M->getLength())) {
         if (auto *C = dyn_cast<CallInst>(MI)) {
-          // FIXME: Can we pass in either of dest/src alignment here instead
-          // of conservatively taking the minimum?
-          Align Alignment = std::min(M->getDestAlign().valueOrOne(),
-                                     M->getSourceAlign().valueOrOne());
           if (performCallSlotOptzn(M, M, M->getDest(), M->getSource(),
                                    TypeSize::getFixed(CopySize->getZExtValue()),
-                                   Alignment, BAA,
+                                   M->getDestAlign().valueOrOne(), BAA,
                                    [C]() -> CallInst * { return C; })) {
             LLVM_DEBUG(dbgs() << "Performed call slot optimization:\n"
                               << "    call: " << *C << "\n"
                               << "    memcpy: " << *M << "\n");
             eraseInstruction(M);
             ++NumMemCpyInstr;
             return true;
           }

llvm/test/Transforms/MemCpyOpt/callslot.ll

captures:
   ret void
 
 nocaptures:
   call void @accept_ptr(ptr %src) nounwind
   call void @llvm.memcpy.p0.p0.i64(ptr %dest, ptr %src, i64 16, i1 false)
   ret void
 }
 
+define void @source_alignment(ptr noalias dereferenceable(128) %dst) {
+; CHECK-LABEL: @source_alignment(
+; CHECK-NEXT:    [[SRC:%.*]] = alloca [128 x i8], align 4
+; CHECK-NEXT:    call void @accept_ptr(ptr nocapture [[DST:%.*]]) #[[ATTR3]]
+; CHECK-NEXT:    ret void
+;
+  %src = alloca [128 x i8], align 4
+  call void @accept_ptr(ptr nocapture %src) nounwind
+  call void @llvm.memcpy.p0.p0.i64(ptr align 4 %dst, ptr %src, i64 128, i1 false)
+  ret void
+}
+
 declare void @may_throw()
 declare void @accept_ptr(ptr)
 declare void @llvm.memcpy.p0.p0.i64(ptr, ptr, i64, i1)
 declare void @llvm.memset.p0.i64(ptr, i8, i64, i1)