[GlobalISel] Lower dbg.declare into indirect DBG_VALUE

[cuviper]

Summary

D31439 changed the semantics of dbg.declare to take the address of a
variable as the first argument, making it indirect. It specifically
updated FastISel for this change here:

https://reviews.llvm.org/D31439#change-WVArzi177jPl

GlobalISel needs to follow suit, or else it will be missing a level of
indirection in the generated debuginfo. This problem was seen in a Rust
debuginfo test on aarch64, since GlobalISel is used at -O0 for aarch64.

https://github.com/rust-lang/rust/issues/49807
https://bugzilla.redhat.com/show_bug.cgi?id=1611597
https://bugzilla.redhat.com/show_bug.cgi?id=1625768

[aprantl]

Note that strictly speaking a dbg.declare is not 100% the same as an indirect DBG_VALUE. A dbg.declare declares a stack slot for a variable, and specifically has a lifetime valid for the entire lexical scope of that variable, whereas the lifetime of a DBG_VALUE only lasts until the next DBG_VALUE for the same variable, the end of the basic block (except where LiveDebugValues can prove it doesn't), or until the value the DBG_Value is pointing to is clobbered, whichever happens first.

Because of this the other instruction selectors enter dbg.declares into the MMI sidetable, which is a map of stack slots.

[aprantl]

Because of LiveDebugValues this patch might still work though.

Does GlobalISel support the MMI side table? Otherwise this seems a fine approach.

[rnk]

In D51749#1226398, @aprantl wrote:

Because of LiveDebugValues this patch might still work though.

Does GlobalISel support the MMI side table? Otherwise this seems a fine approach.

It already uses the (now MachineFunction, no longer MMI) side table, if the argument is a static alloca. The original code with more context:

auto AI = dyn_cast<AllocaInst>(Address);
if (AI && AI->isStaticAlloca()) {
  // Static allocas are tracked at the MF level, no need for DBG_VALUE
  // instructions (in fact, they get ignored if they *do* exist).
  MF->setVariableDbgInfo(DI.getVariable(), DI.getExpression(),
                         getOrCreateFrameIndex(*AI), DI.getDebugLoc());
} else
  MIRBuilder.buildDirectDbgValue(getOrCreateVReg(*Address),
                                 DI.getVariable(), DI.getExpression());
return true;

So, this only affects variables that are declared in not-allocas at -O0, i.e. things passed indirectly with byval, or sret, or by address in the way that non-trivially copyable C++ types are.

This code could be improved to handle more cases where the value in question actually has a frame index. byval and inalloca are the obvious ways that I know of to get those. Looks like this is the code we use in SDAG ISel for it:

} else if (const auto *Arg = dyn_cast<Argument>(
               Address->stripInBoundsConstantOffsets())) {
  FI = FuncInfo.getArgumentFrameIndex(Arg);
}

I think this change could use a more direct test, consider starting with the IR generated for a C++ object, like:

struct NTCopy {
  NTCopy();
  NTCopy(const NTCopy &);
  int x;
};
int foo(NTCopy o) {
  return o.x;
}

test/CodeGen/AArch64/GlobalISel/debug-insts.ll

12 ↗

(On Diff #164278)

This test is strange, it declares the variable twice, once in %in.addr (correct) and again in %in (incorrect).

20 ↗

(On Diff #164278)

This change makes sense, though. The VLA is at some stack pointer offset. I assume p0 is the SP reg.

[cuviper]

In D51749#1226417, @rnk wrote:

I think this change could use a more direct test, consider starting with the IR generated for a C++ object, like:

struct NTCopy {
  NTCopy();
  NTCopy(const NTCopy &);
  int x;
};
int foo(NTCopy o) {
  return o.x;
}

Thanks for this simple example -- it does indeed produce a bad DEBUG_VALUE of $x0 before, now fixed to an indirect [$x0+0]. I'll add this as a test.

test/CodeGen/AArch64/GlobalISel/debug-insts.ll

12 ↗

(On Diff #164278)

This test is strange, it declares the variable twice, once in %in.addr (correct) and again in %in (incorrect).

Yeah, I don't know why. Clang only produces the %in.addr version for void debug_declare(int in). Then you don't get any DBG_VALUE output for this function, but I think that's probably OK, and in.addr has CHECK lines above. I can remove the plain %in one.

20 ↗

(On Diff #164278)

I assume p0 is the SP reg.

Yes, the lines up to %6 compute the VLA size, and then:

$sp = COPY %6(p0)
%7:_(p0) = COPY %6(p0)
DBG_VALUE debug-use %7(p0), 0, !14, !DIExpression(), debug-location !15

[cuviper]

• Removed the duplicate (and incorrect) declaration in debug-insts.ll debug_declare
• Added debug-cpp.ll which more directly targets this change.

[rnk]

lgtm

llvm/trunk/lib/CodeGen/GlobalISel/IRTranslator.cpp

assert(DI.getVariable()->isValidLocationForIntrinsic(
                MIRBuilder.getDebugLoc()) &&
            "Expected inlined-at fields to agree");
     auto AI = dyn_cast<AllocaInst>(Address);
     if (AI && AI->isStaticAlloca()) {
       // Static allocas are tracked at the MF level, no need for DBG_VALUE
       // instructions (in fact, they get ignored if they *do* exist).
       MF->setVariableDbgInfo(DI.getVariable(), DI.getExpression(),
                              getOrCreateFrameIndex(*AI), DI.getDebugLoc());
-    } else
-      MIRBuilder.buildDirectDbgValue(getOrCreateVReg(*Address),
+    } else {
+      // A dbg.declare describes the address of a source variable, so lower it
+      // into an indirect DBG_VALUE.
+      MIRBuilder.buildIndirectDbgValue(getOrCreateVReg(*Address),
                                        DI.getVariable(), DI.getExpression());
+    }
     return true;
   }
   case Intrinsic::dbg_label: {
     const DbgLabelInst &DI = cast<DbgLabelInst>(CI);
     assert(DI.getLabel() && "Missing label");
 
     assert(DI.getLabel()->isValidLocationForIntrinsic(
                MIRBuilder.getDebugLoc()) &&

llvm/trunk/test/CodeGen/AArch64/GlobalISel/debug-cpp.ll

+; RUN: llc -global-isel -mtriple=aarch64 %s -stop-after=irtranslator -o - | FileCheck %s
+; RUN: llc -mtriple=aarch64 -global-isel --global-isel-abort=0 -o /dev/null
+
+; struct NTCopy {
+;   NTCopy();
+;   NTCopy(const NTCopy &);
+;   int x;
+; };
+; int foo(NTCopy o) {
+;   return o.x;
+; }
+
+; ModuleID = 'ntcopy.cpp'
+source_filename = "ntcopy.cpp"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
+%struct.NTCopy = type { i32 }
+
+; CHECK-LABEL: name: _Z3foo6NTCopy
+; CHECK: DBG_VALUE debug-use %{{[0-9]+}}(p0), 0, !23, !DIExpression(), debug-location !24
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @_Z3foo6NTCopy(%struct.NTCopy* %o) #0 !dbg !7 {
+entry:
+  call void @llvm.dbg.declare(metadata %struct.NTCopy* %o, metadata !23, metadata !DIExpression()), !dbg !24
+  %x = getelementptr inbounds %struct.NTCopy, %struct.NTCopy* %o, i32 0, i32 0, !dbg !25
+  %0 = load i32, i32* %x, align 4, !dbg !25
+  ret i32 %0, !dbg !26
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind optnone }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "ntcopy.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 8.0.0 "}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foo6NTCopy", scope: !1, file: !1, line: 6, type: !8, isLocal: false, isDefinition: true, scopeLine: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !11}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "NTCopy", file: !1, line: 1, size: 32, flags: DIFlagTypePassByReference, elements: !12, identifier: "_ZTS6NTCopy")
+!12 = !{!13, !14, !18}
+!13 = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: !11, file: !1, line: 4, baseType: !10, size: 32)
+!14 = !DISubprogram(name: "NTCopy", scope: !11, file: !1, line: 2, type: !15, isLocal: false, isDefinition: false, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null, !17}
+!17 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!18 = !DISubprogram(name: "NTCopy", scope: !11, file: !1, line: 3, type: !19, isLocal: false, isDefinition: false, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false)
+!19 = !DISubroutineType(types: !20)
+!20 = !{null, !17, !21}
+!21 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !22, size: 64)
+!22 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !11)
+!23 = !DILocalVariable(name: "o", arg: 1, scope: !7, file: !1, line: 6, type: !11)
+!24 = !DILocation(line: 6, column: 16, scope: !7)
+!25 = !DILocation(line: 7, column: 12, scope: !7)
+!26 = !DILocation(line: 7, column: 3, scope: !7)

llvm/trunk/test/CodeGen/AArch64/GlobalISel/debug-insts.ll

 ; RUN: llc -global-isel -mtriple=aarch64 %s -stop-after=irtranslator -o - | FileCheck %s
 ; RUN: llc -mtriple=aarch64 -global-isel --global-isel-abort=0 -o /dev/null
 
 ; CHECK-LABEL: name: debug_declare
 ; CHECK: stack:
 ; CHECK:    - { id: {{.*}}, name: in.addr, type: default, offset: 0, size: {{.*}}, alignment: {{.*}},
 ; CHECK-NEXT: callee-saved-register: '', callee-saved-restored: true,
 ; CHECK-NEXT: debug-info-variable: '!11', debug-info-expression: '!DIExpression()',
-; CHECK: DBG_VALUE debug-use %0(s32), debug-use $noreg, !11, !DIExpression(), debug-location !12
 define void @debug_declare(i32 %in) #0 !dbg !7 {
 entry:
   %in.addr = alloca i32, align 4
   store i32 %in, i32* %in.addr, align 4
   call void @llvm.dbg.declare(metadata i32* %in.addr, metadata !11, metadata !DIExpression()), !dbg !12
-  call void @llvm.dbg.declare(metadata i32 %in, metadata !11, metadata !DIExpression()), !dbg !12
   ret void, !dbg !12
 }
 
 ; CHECK-LABEL: name: debug_declare_vla
-; CHECK: DBG_VALUE debug-use %{{[0-9]+}}(p0), debug-use $noreg, !14, !DIExpression(), debug-location !15
+; CHECK: DBG_VALUE debug-use %{{[0-9]+}}(p0), 0, !14, !DIExpression(), debug-location !15
 define void @debug_declare_vla(i32 %in) #0 !dbg !13 {
 entry:
   %vla.addr = alloca i32, i32 %in
   call void @llvm.dbg.declare(metadata i32* %vla.addr, metadata !14, metadata !DIExpression()), !dbg !15
   ret void, !dbg !15
 }
 
 ; CHECK-LABEL: name: debug_value