[debug-info] MemCpyOpt should merge the debug location when replace multiple block-local instruction with a new memset

[yuanboli233]

Summary

The bug is proposed here: https://bugs.llvm.org/show_bug.cgi?id=49270
Nobody has commented on the bug yet, but I think it clearly deviates from the description in our LLVM how-to-update-debug-info guide here: https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-merge-instruction-locations

"(Merges should be applied when) peephole optimizations which combine multiple instructions together, like (add (mul A B) C) => llvm.fma.f32(A, B, C)".

Multiple store instructions in the same basic block are merged into one memset instruction:

In the original ll file:

%arrayidx = getelementptr inbounds i32, i32* %P, i64 1, !dbg !13
  call void @llvm.dbg.value(metadata i32* %arrayidx, metadata !9, metadata !DIExpression()), !dbg !13
  store i32 0, i32* %arrayidx, align 4, !dbg !14
  %add.ptr = getelementptr inbounds i32, i32* %P, i64 2, !dbg !15
  call void @llvm.dbg.value(metadata i32* %add.ptr, metadata !11, metadata !DIExpression()), !dbg !15
  %0 = bitcast i32* %add.ptr to i8*, !dbg !16
  call void @llvm.dbg.value(metadata i8* %0, metadata !12, metadata !DIExpression()), !dbg !16
  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false), !dbg !17
  ret void, !dbg !18

The ll file after apply -memcpyopt pass:

%arrayidx = getelementptr inbounds i32, i32* %P, i64 1, !dbg !13
  call void @llvm.dbg.value(metadata i32* %arrayidx, metadata !9, metadata !DIExpression()), !dbg !13
  %add.ptr = getelementptr inbounds i32, i32* %P, i64 2, !dbg !14
  call void @llvm.dbg.value(metadata i32* %add.ptr, metadata !11, metadata !DIExpression()), !dbg !14
  %0 = bitcast i32* %add.ptr to i8*, !dbg !15
  call void @llvm.dbg.value(metadata i8* %0, metadata !12, metadata !DIExpression()), !dbg !15
  %1 = bitcast i32* %arrayidx to i8*, !dbg !16
  call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 15, i1 false), !dbg !17
  ret void, !dbg !16

There are two stores: "stores i32 0, i32* %arrayidx, align 4, !dbg !14" and "tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false), !dbg !17".

They are merged into one memset instruction " call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 15, i1 false), !dbg !17".

The new memset instruction only takes the debug location from one of the merged instructions. However, it should take the merged debug location from the two instructions.

[yuanboli233]

Sorry for failing the tests, will check the problem and revise to pass the unit tests, it is not happening in my local machine.

[djtodoro]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

488

Please use llvm::SmallVector instead.

Please use camel naming convention here.

[yuanboli233]

Thanks for the suggestions from @djtodoro! Format revised and changed to SmallVector.

[djtodoro]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

492

typo: Dgb

Any how, we don't need the variable, just to it in one line.

[yuanboli233]

Thanks for pointing it out, I have made it to one line.

[jmorse]

This is a good find; style nit and test comments inline.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

491

I think for single-line loops we omit the curly-braces? Run it through clang-format-patch and see what it says.

llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll

3

The test should check that the memset call has this metadata attached to it.

8–16

IMO, there should be more than one store to ensure the new code is being fully covered. Ideally:

• One set of stores with identical DebugLocs, coalesced into a memset with that DebugLoc,
• Another set of stores with differing DebugLocs, that should produce a memset where the DebugLoc is for "line: 0", which is what getMergedLocations should produce I think.

25–27

Un-necessary attributes should be deleted from debug-info tests

[yuanboli233]

Updated the test case to include both the preserving case and the merge case.

Code format updated as well.

Thanks for the suggestions from @jmorse!

[djtodoro]

llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll

3

(Super nit)
Instead of ; COM: for comments in tests, we use ;;

42

Since we deleted the attributes, we don't need the #0 anymore.

[yuanboli233]

Thanks for the suggestion from @djtodoro, updated the diff

[jmorse]

LGTM

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

for (const MemsetRange &Range : Ranges) {
     StartPtr = Range.StartPtr;
 
     AMemSet = Builder.CreateMemSet(StartPtr, ByteVal, Range.End - Range.Start,
                                    MaybeAlign(Range.Alignment));
     LLVM_DEBUG(dbgs() << "Replace stores:\n"; for (Instruction *SI
                                                    : Range.TheStores) dbgs()
                                               << *SI << '\n';
                dbgs() << "With: " << *AMemSet << '\n');
-    if (!Range.TheStores.empty())
-      AMemSet->setDebugLoc(Range.TheStores[0]->getDebugLoc());
+    if (!Range.TheStores.empty()) {
+      SmallVector<const DILocation *, 4> StoreLocs;
+      for (auto *StoreInst : Range.TheStores)
+        StoreLocs.push_back(StoreInst->getDebugLoc().get());
+      AMemSet->setDebugLoc(DebugLoc(DILocation::getMergedLocations(StoreLocs)));
+    }
 
     if (MSSAU) {
       assert(LastMemDef && MemInsertPoint &&
              "Both LastMemDef and MemInsertPoint need to be set");
       auto *NewDef =
           cast<MemoryDef>(MemInsertPoint->getMemoryInst() == &*BI
                               ? MSSAU->createMemoryAccessBefore(
                                     AMemSet, LastMemDef, MemInsertPoint)

llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll

+; RUN: opt -memcpyopt -S < %s | FileCheck %s
+;; In test3 function, this test case checks whether the generated new memset
+;; preserves the debug locations.
+;; In test4 function, this test case checks whether the generated new memset
+;; merges the debug locations.
+; CHECK: define void @test3
+; CHECK: call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 19, i1 false), !dbg ![[LOC1:[0-9]+]]
+source_filename = "abc_debug.ll"
+; CHECK: define void @test4
+; CHECK: call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 19, i1 false), !dbg ![[LOC2:[0-9]+]]
+; CHECK: ![[LOC1]] = !DILocation(line: 5, column: 1, scope
+; CHECK: ![[LOC2]] = !DILocation(line: 0, scope:
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @test3(i32* %P) !dbg !6 {
+entry:
+  %arrayidx = getelementptr inbounds i32, i32* %P, i64 1, !dbg !8
+  %add.ptrone = getelementptr inbounds i32, i32* %P, i64 2, !dbg !9
+  %add.ptrtwo = getelementptr inbounds i32, i32* %P, i64 3, !dbg !10
+  %0 = bitcast i32* %add.ptrtwo to i8*, !dbg !11
+  store i32 0, i32* %arrayidx, align 4, !dbg !12
+  store i32 0, i32* %add.ptrone, align 4, !dbg !12
+  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false), !dbg !12
+  ret void, !dbg !15
+}
+
+define void @test4(i32* %P) !dbg !16 {
+entry:
+  %arrayidx = getelementptr inbounds i32, i32* %P, i64 1, !dbg !17
+  %add.ptrone = getelementptr inbounds i32, i32* %P, i64 2, !dbg !18
+  %add.ptrtwo = getelementptr inbounds i32, i32* %P, i64 3, !dbg !19
+  %0 = bitcast i32* %add.ptrtwo to i8*, !dbg !20
+  store i32 0, i32* %arrayidx, align 4, !dbg !21
+  store i32 0, i32* %add.ptrone, align 4, !dbg !22
+  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false), !dbg !23
+  ret void, !dbg !24
+}
+
+; Function Attrs: argmemonly nofree nosync nounwind willreturn writeonly
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) 
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+!llvm.debugify = !{!4, !5}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !3)
+!2 = !DIFile(filename: "<stdin>", directory: "/")
+!3 = !{}
+!4 = !{i32 16}
+!5 = !{i32 0}
+!6 = distinct !DISubprogram(name: "test3", linkageName: "test3", scope: null, file: !2, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !3)
+!7 = !DISubroutineType(types: !3)
+!8 = !DILocation(line: 1, column: 1, scope: !6)
+!9 = !DILocation(line: 2, column: 1, scope: !6)
+!10 = !DILocation(line: 3, column: 1, scope: !6)
+!11 = !DILocation(line: 4, column: 1, scope: !6)
+!12 = !DILocation(line: 5, column: 1, scope: !6)
+!15 = !DILocation(line: 8, column: 1, scope: !6)
+!16 = distinct !DISubprogram(name: "test4", linkageName: "test4", scope: null, file: !2, line: 9, type: !7, scopeLine: 9, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !3)
+!17 = !DILocation(line: 9, column: 1, scope: !16)
+!18 = !DILocation(line: 10, column: 1, scope: !16)
+!19 = !DILocation(line: 11, column: 1, scope: !16)
+!20 = !DILocation(line: 12, column: 1, scope: !16)
+!21 = !DILocation(line: 13, column: 1, scope: !16)
+!22 = !DILocation(line: 14, column: 1, scope: !16)
+!23 = !DILocation(line: 15, column: 1, scope: !16)
+!24 = !DILocation(line: 16, column: 1, scope: !16)