[MemCpyOpt] Fix up debug loc for simplified memset in processMemSetMemCpyDependence

[bjope]

Summary

Make sure the code comments in processMemSetMemCpyDependence match
with the actual transform. They indicated that the memset being
rewritten was sunk to after a memcpy, while it actually is inserted
just before the memcpy.

Also make sure we use the debug location of the original memset
when creating the new simplified memset. In the past we've been
using the debug location for the memcpy which could be a bit
confusing.

[bjope]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1281–1284

Side note: This can be replaced by an usub_sat. I guess it would be safe to do that directly here instead of letting InstCombine detect the pattern later?

1295–1296

@fhahn : I think you added the MSSA updating here. But the code comments did not match up with the actual movement of the memset. But I think the actual MSSA update seem to match the implementation, so it was only code comment that was wrong. Right?

And as I hinted elsewhere, maybe we want this transform to move the memset below the memcpy. But that seem to make the MSSA update a bit trickier. I kind of assumed that one could use MSSAU->moveAfter/moveBefore in a situation like this? But maybe that require that the memset instruction only is moved and not replaced (as we need to adjust the arguments here) in some way?

[bjope]

Alternative patch that aligns the code with the comments instead of updating the comments: D135653

Not sure which one is best or how to motivate the reordering (for out OOT target it might be better to do the memcpy first sometimes, but that is probably a more general scheduling thing rather than something this pass should need to think about).

[bjope]

Change the focus of the patch to be around correcting the debug loc of the new
memset. And then cleaning up incorrect code comments is an extra bonus.

[bjope]

Adding some debug info people as reviewers.

I originally added @fhahn as reviewer, to consider the alternative solution D135653 involving changing the implementation to match with the old code comments instead. But since this has been stalled since October I thought that I could shape up this patch and promote this solution in favor of D135653 (as it is a smaller change to only impact debug info and not codegen).

[jmorse]

Makes sense to me, the new comments appear to reflect what the code is doing better (insert the memset before the memcpy). The source locations look good too (added one nit comment). Added Orlando as a reviewer as peeling apart memcpys is his favourite thing.

I've a small concern over whether the new memset is going to re-order source locations -- as defined in https://llvm.org/docs/HowToUpdateDebugInfo.html we try to avoid source locations appearing outside of their original block or out of the original program order. It looks like the only caller to processMemSetMemCpyDependence enforces that the memset/memcpy are in the same block which means this patch is fine, but this is liable to become untrue in a future refactor. Would you be able to put a MemSet->getParent() == MemCpy->getParent() assertion next to the call to SetCurrentDebugLocation plus a comment about avoiding the source location appearing out of order? It feels a bit pointless, but IMO it'll guard against future refactors making a mistake.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1296

llvm/test/Transforms/MemCpyOpt/memset-memcpy-dbgloc.ll

26–29

I'd suggest dropping the scope metadata node as it's liable to be re-numbered by output changes in the future, and in this context I don't think it's testing anything meaningful.

[bjope]

Updated according to review feedback.

[jmorse]

LGTM, cheers

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

 
 /// We've found that the (upward scanning) memory dependence of \p MemCpy is
 /// \p MemSet.  Try to simplify \p MemSet to only set the trailing bytes that
 /// weren't copied over by \p MemCpy.
 ///
 /// In other words, transform:
 /// \code
 ///   memset(dst, c, dst_size);
+///   ...
 ///   memcpy(dst, src, src_size);
 /// \endcode
 /// into:
 /// \code
-///   memcpy(dst, src, src_size);
+///   ...
 ///   memset(dst + src_size, c, dst_size <= src_size ? 0 : dst_size - src_size);
+///   memcpy(dst, src, src_size);
 /// \endcode
+///
+/// The memset is sunk to just before the memcpy to ensure that src_size is
+/// present when emitting the simplified memset.
 bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
                                                   MemSetInst *MemSet,
                                                   BatchAAResults &BAA) {
   // We can only transform memset/memcpy with the same destination.
   if (!BAA.isMustAlias(MemSet->getDest(), MemCpy->getDest()))
     return false;
 
   // Check that src and dst of the memcpy aren't the same. While memcpy
bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
   const Align DestAlign = std::max(MemSet->getDestAlign().valueOrOne(),
                                    MemCpy->getDestAlign().valueOrOne());
   if (DestAlign > 1)
     if (auto *SrcSizeC = dyn_cast<ConstantInt>(SrcSize))
       Alignment = commonAlignment(DestAlign, SrcSizeC->getZExtValue());
 
   IRBuilder<> Builder(MemCpy);
 
+  // Preserve the debug location of the old memset for the code emitted here
+  // related to the new memset. This is correct according to the rules in
+  // https://llvm.org/docs/HowToUpdateDebugInfo.html about "when to preserve an
+  // instruction location", given that we move the memset within the basic
+  // block.
+  assert(MemSet->getParent() == MemCpy->getParent() &&
+         "Preserving debug location based on moving memset within BB.");
+  Builder.SetCurrentDebugLocation(MemSet->getDebugLoc());
+
   // If the sizes have different types, zext the smaller one.
   if (DestSize->getType() != SrcSize->getType()) {
     if (DestSize->getType()->getIntegerBitWidth() >
         SrcSize->getType()->getIntegerBitWidth())
       SrcSize = Builder.CreateZExt(SrcSize, DestSize->getType());
     else
       DestSize = Builder.CreateZExt(DestSize, SrcSize->getType());
   }
 
   Value *Ule = Builder.CreateICmpULE(DestSize, SrcSize);
   Value *SizeDiff = Builder.CreateSub(DestSize, SrcSize);
   Value *MemsetLen = Builder.CreateSelect(
       Ule, ConstantInt::getNullValue(DestSize->getType()), SizeDiff);
   unsigned DestAS = Dest->getType()->getPointerAddressSpace();
   Instruction *NewMemSet = Builder.CreateMemSet(
       Builder.CreateGEP(
           Builder.getInt8Ty(),
           Builder.CreatePointerCast(Dest, Builder.getInt8PtrTy(DestAS)),
           SrcSize),
       MemSet->getOperand(1), MemsetLen, Alignment);
 
   assert(isa<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy)) &&
          "MemCpy must be a MemoryDef");
-  // The new memset is inserted after the memcpy, but it is known that its
-  // defining access is the memset about to be removed which immediately
-  // precedes the memcpy.
+  // The new memset is inserted before the memcpy, and it is known that the
+  // memcpy's defining access is the memset about to be removed.
   auto *LastDef =
       cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(MemCpy));
   auto *NewAccess = MSSAU->createMemoryAccessBefore(
       NewMemSet, LastDef->getDefiningAccess(), LastDef);
   MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);
 
   eraseInstruction(MemSet);
   return true;
bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI) {
   MemoryUseOrDef *MA = MSSA->getMemoryAccess(M);
   // FIXME: Not using getClobberingMemoryAccess() here due to PR54682.
   MemoryAccess *AnyClobber = MA->getDefiningAccess();
   MemoryLocation DestLoc = MemoryLocation::getForDest(M);
   const MemoryAccess *DestClobber =
       MSSA->getWalker()->getClobberingMemoryAccess(AnyClobber, DestLoc, BAA);
 
   // Try to turn a partially redundant memset + memcpy into
-  // memcpy + smaller memset.  We don't need the memcpy size for this.
-  // The memcpy most post-dom the memset, so limit this to the same basic
+  // smaller memset + memcpy.  We don't need the memcpy size for this.
+  // The memcpy must post-dom the memset, so limit this to the same basic
   // block. A non-local generalization is likely not worthwhile.
   if (auto *MD = dyn_cast<MemoryDef>(DestClobber))
     if (auto *MDep = dyn_cast_or_null<MemSetInst>(MD->getMemoryInst()))
       if (DestClobber->getBlock() == M->getParent())
         if (processMemSetMemCpyDependence(M, MDep, BAA))
           return true;
 
   MemoryAccess *SrcClobber = MSSA->getWalker()->getClobberingMemoryAccess(

llvm/test/Transforms/MemCpyOpt/memset-memcpy-dbgloc.ll

+; RUN: opt -passes=memcpyopt -S %s -verify-memoryssa | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+
+@C = external constant [0 x i8]
+
+declare void @llvm.memset.p0.i64(ptr nocapture, i8, i64, i1)
+declare void @llvm.memcpy.p0.p0.i64(ptr nocapture, ptr nocapture readonly, i64, i1)
+
+define void @test_constant(i64 %src_size, ptr %dst, i64 %dst_size, i8 %c) !dbg !5 {
+; CHECK-LABEL: @test_constant(
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp ule i64 [[DST_SIZE:%.*]], [[SRC_SIZE:%.*]], !dbg [[DBG11:![0-9]+]]
+; CHECK-NEXT:    [[TMP2:%.*]] = sub i64 [[DST_SIZE]], [[SRC_SIZE]], !dbg [[DBG11]]
+; CHECK-NEXT:    [[TMP3:%.*]] = select i1 [[TMP1]], i64 0, i64 [[TMP2]], !dbg [[DBG11]]
+; CHECK-NEXT:    [[TMP4:%.*]] = getelementptr i8, ptr [[DST:%.*]], i64 [[SRC_SIZE]], !dbg [[DBG11]]
+; CHECK-NEXT:    call void @llvm.memset.p0.i64(ptr align 1 [[TMP4]], i8 [[C:%.*]], i64 [[TMP3]], i1 false), !dbg [[DBG11]]
+; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr [[DST]], ptr @C, i64 [[SRC_SIZE]], i1 false), !dbg [[DBG12:![0-9]+]]
+; CHECK-NEXT:    ret void, !dbg [[DBG13:![0-9]+]]
+;
+  call void @llvm.memset.p0.i64(ptr %dst, i8 %c, i64 %dst_size, i1 false), !dbg !11
+  call void @llvm.memcpy.p0.p0.i64(ptr %dst, ptr @C, i64 %src_size, i1 false), !dbg !12
+  ret void, !dbg !13
+}
+
+; Validate that the memset is mapped to DILocation for the original memset.
+; CHECK: [[DBG11]] = !DILocation(line: 1,
+; CHECK: [[DBG12]] = !DILocation(line: 2,
+; CHECK: [[DBG13]] = !DILocation(line: 3,
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!2, !3}
+!llvm.module.flags = !{!4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "memset-memcpy-dbgloc.ll", directory: "/")
+!2 = !{i32 3}
+!3 = !{i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "test_constant", linkageName: "test_constant", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9}
+!9 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 3, type: !10)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!11 = !DILocation(line: 1, column: 1, scope: !5)
+!12 = !DILocation(line: 2, column: 1, scope: !5)
+!13 = !DILocation(line: 3, column: 1, scope: !5)