Avoid misleading line table when Localizer sinks an instruction to another basic block

[aprantl]

Summary

This patch attaches the source location of the insertion point to the localized instruction, if it comes from a different basic block. Preserving the original source location would create a misleading line table.

Cf. https://www.llvm.org/docs/HowToUpdateDebugInfo.html#when-to-drop-an-instruction-location

rdar://78489600

[vsk]

Is this a case where the guidance (as-written) suggests dropping the location instead?

[aprantl]

In D105162#2850394, @vsk wrote:

Is this a case where the guidance (as-written) suggests dropping the location instead?

Yes. And I wonder if we should update the guidance. Literally dropping the location would result in the last instruction's location being carried forward. We don't want the inserted instruction to keep its location from the other basic block, but associating it with the previous instruction's location would also be wrong since we are inserting it on behalf of the next instruction (the insertion point). Maybe this isn't so much sinking an instruction but expanding an existing instruction?

[aprantl]

For context, the bugreport that motivated this was a sanitizer diagnostic that we want to happen on behalf of the insertion point source location.

[vsk]

In D105162#2854369, @aprantl wrote:

In D105162#2850394, @vsk wrote:

Is this a case where the guidance (as-written) suggests dropping the location instead?

Yes. And I wonder if we should update the guidance. Literally dropping the location would result in the last instruction's location being carried forward. We don't want the inserted instruction to keep its location from the other basic block, but associating it with the previous instruction's location would also be wrong since we are inserting it on behalf of the next instruction (the insertion point). Maybe this isn't so much sinking an instruction but expanding an existing instruction?

Applying UseMI's location also seems wrong in some sense: the only right one would be the original, but it can't be kept. It'd also be awkward if UseMI is moved by some subsequent transform while LocalizedMI isn't.

llvm/lib/CodeGen/GlobalISel/Localizer.cpp

for (auto MOIt = MRI->use_begin(Reg), MOItEnd = MRI->use_end();
       Changed = true;
       auto MBBAndReg = std::make_pair(InsertMBB, Reg);
       auto NewVRegIt = MBBWithLocalDef.find(MBBAndReg);
       if (NewVRegIt == MBBWithLocalDef.end()) {
         // Create the localized instruction.
         MachineInstr *LocalizedMI = MF.CloneMachineInstr(&MI);
         LocalizedInstrs.insert(LocalizedMI);
         MachineInstr &UseMI = *MOUse.getParent();
+        if (UseMI.getParent() != LocalizedMI->getParent())
+          // Attach the source location of the insertion point to the
+          // localized instruction, if it comes from a different basic
+          // block. Preserving the original source location would
+          // create a misleading line table.
+          LocalizedMI->setDebugLoc(UseMI.getDebugLoc());
         if (MRI->hasOneUse(Reg) && !UseMI.isPHI())
           InsertMBB->insert(InsertMBB->SkipPHIsAndLabels(UseMI), LocalizedMI);
         else
           InsertMBB->insert(InsertMBB->SkipPHIsAndLabels(InsertMBB->begin()),
                             LocalizedMI);
 
         // Set a new register for the definition.
         Register NewReg = MRI->createGenericVirtualRegister(MRI->getType(Reg));

llvm/test/DebugInfo/MIR/AArch64/localizer.mir

+# Test that Localizer attaches a debug location to instructions sunk into a new basic block.
+# RUN: llc -O0 -mtriple=aarch64-apple-ios -run-pass=localizer -verify-machineinstrs %s -o - | FileCheck %s -check-prefix=CHECK
+--- |
+  ; clang -target arm64-apple-macos11 -mllvm -stop-before=localizer -c -gline-tables-only 
+  ; extern int j, k;
+  ; void f(int i) {
+  ;   i = 1;
+  ;   if (j)
+  ;     k += 1;
+  ; }
+
+  source_filename = "/tmp/t.c"
+  target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+  target triple = "arm64-apple-macosx11.0.0"
+  
+  @j = external global i32, align 4
+  @k = external global i32, align 4
+  
+  ; Function Attrs: noinline nounwind optnone ssp uwtable
+  define void @f(i32 %i) #0 !dbg !14 {
+  entry:
+    %i.addr = alloca i32, align 4
+    store i32 %i, i32* %i.addr, align 4
+    store i32 1, i32* %i.addr, align 4, !dbg !17
+    %0 = load i32, i32* @j, align 4, !dbg !18
+    %tobool = icmp ne i32 %0, 0, !dbg !18
+    br i1 %tobool, label %if.then, label %if.end, !dbg !18
+  
+  if.then:                                          ; preds = %entry
+    %1 = load i32, i32* @k, align 4, !dbg !19
+    %add = add nsw i32 %1, 1, !dbg !19
+    store i32 %add, i32* @k, align 4, !dbg !19
+    br label %if.end, !dbg !20
+  
+  if.end:                                           ; preds = %if.then, %entry
+    ret void, !dbg !21
+  }
+  
+  attributes #0 = { noinline nounwind optnone ssp uwtable }
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", emissionKind: LineTablesOnly, sysroot: "/")
+  !1 = !DIFile(filename: "t.c", directory: "/tmp")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !14 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 2, type: !16, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !16 = !DISubroutineType(types: !2)
+  !17 = !DILocation(line: 5, column: 5, scope: !14)
+  !18 = !DILocation(line: 6, column: 7, scope: !14)
+  !19 = !DILocation(line: 7, column: 7, scope: !14)
+  !20 = !DILocation(line: 7, column: 5, scope: !14)
+  !21 = !DILocation(line: 8, column: 1, scope: !14)
+
+...
+---
+name:            f
+registers:
+  - { id: 0, class: gpr, preferred-register: '' }
+  - { id: 1, class: gpr, preferred-register: '' }
+  - { id: 2, class: gpr, preferred-register: '' }
+  - { id: 3, class: gpr, preferred-register: '' }
+  - { id: 4, class: gpr, preferred-register: '' }
+  - { id: 5, class: gpr, preferred-register: '' }
+  - { id: 6, class: gpr, preferred-register: '' }
+  - { id: 7, class: gpr, preferred-register: '' }
+  - { id: 8, class: gpr, preferred-register: '' }
+  - { id: 9, class: gpr, preferred-register: '' }
+  - { id: 10, class: _, preferred-register: '' }
+  - { id: 11, class: _, preferred-register: '' }
+  - { id: 12, class: gpr, preferred-register: '' }
+liveins:
+  - { reg: '$w0', virtual-reg: '' }
+fixedStack:      []
+stack:
+  - { id: 0, name: i.addr, type: default, offset: 0, size: 4, alignment: 4, 
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true, 
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.1.entry:
+    successors: %bb.2(0x40000000), %bb.3(0x40000000)
+    liveins: $w0
+  
+    %0:gpr(s32) = COPY $w0
+    %2:gpr(s32) = G_CONSTANT i32 1
+    %4:gpr(p0) = G_GLOBAL_VALUE @j, debug-location !DILocation(line: 0, scope: !14)
+    %5:gpr(s32) = G_CONSTANT i32 0
+    %8:gpr(p0) = G_GLOBAL_VALUE @k, debug-location !DILocation(line: 0, scope: !14)
+    %1:gpr(p0) = G_FRAME_INDEX %stack.0.i.addr
+    G_STORE %0(s32), %1(p0) :: (store 4 into %ir.i.addr)
+    G_STORE %2(s32), %1(p0), debug-location !17 :: (store 4 into %ir.i.addr)
+    %3:gpr(s32) = G_LOAD %4(p0), debug-location !18 :: (dereferenceable load 4 from @j)
+    %12:gpr(s32) = G_ICMP intpred(eq), %3(s32), %5, debug-location !18
+    %6:gpr(s1) = G_TRUNC %12(s32), debug-location !18
+    G_BRCOND %6(s1), %bb.3, debug-location !18
+    G_BR %bb.2, debug-location !18
+  
+  bb.2.if.then:
+    successors: %bb.3(0x80000000)
+    ; CHECK-LABEL: bb.1.if.then:
+    ; CHECK:         %13:gpr(p0) = G_GLOBAL_VALUE @k, debug-location ![[DBG:[0-9]+]]
+    ; CHECK-NEXT:    %7:gpr(s32) = G_LOAD %13(p0), debug-location ![[DBG]]
+    ; CHECK-NEXT:    %14:gpr(s32) = G_CONSTANT i32 1, debug-location ![[DBG]]
+    ; CHECK-NEXT:    %9:gpr(s32) = nsw G_ADD %7, %14, debug-location ![[DBG]]
+    ; CHECK-NEXT:    G_STORE %9(s32), %13(p0), debug-location ![[DBG]]
+    ; CHECK-LABEL: if.end:
+
+    %7:gpr(s32) = G_LOAD %8(p0), debug-location !19 :: (dereferenceable load 4 from @k)
+    %9:gpr(s32) = nsw G_ADD %7, %2, debug-location !19
+    G_STORE %9(s32), %8(p0), debug-location !19 :: (store 4 into @k)
+  
+  bb.3.if.end:
+    RET_ReallyLR debug-location !21
+
+...