ARM64: Debug info for structure argument missing DW_AT_location

[kamleshbhalui]

Summary

prevent eliminating dbg_val due to COPY.

Fixes this
https://bugs.llvm.org/show_bug.cgi?id=40709

[vsk]

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

177–178

If MI is erased, why should its debug uses not be made undef? What really causes those debug uses to be updated correctly?

[kamleshbhalui]

@vsk thanks for reviewing.

below dump is taken, by debugging the clang with this test

#include<stdio.h>
typedef struct {
double a,b,c,d,e;
}mystruct;

void foo(mystruct ms){
printf("%llu\n",(unsigned long long) &ms);
}

This is the dump before erasing the MI ()

bb.1.entry:
  liveins: $x0
  %0:gpr64(p0) = COPY $x0
  %4:gpr64(p0) = MOVaddr target-flags(aarch64-page) @.str, target-flags(aarch64-pageoff, aarch64-nc) @.str, debug-location !DILocation(line: 0, scope: !9); kk3.c:0
  DBG_VALUE %0:gpr64(p0), $noreg, !"ms", !DIExpression(DW_OP_deref), debug-location !22; kk3.c:5:19 line no:5
  %1:gpr64(s64) = COPY %0:gpr64(p0), debug-location !23; kk3.c:7:1
  ADJCALLSTACKDOWN 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  $x0 = COPY %4:gpr64(p0), debug-location !24; kk3.c:6:5
  $x1 = COPY %1:gpr64(s64), debug-location !24; kk3.c:6:5
  BL @printf, <regmask $fp $lr $b8 $b9 $b10 $b11 $b12 $b13 $b14 $b15 $d8 $d9 $d10 $d11 $d12 $d13 $d14 $d15 $h8 $h9 $h10 $h11 $h12 $h13 $h14 $h15 $s8 $s9 $s10 $s11 $s12 $s13 $s14 and 51 more...>, implicit-def $lr, implicit $sp, implicit $x0, implicit $x1, implicit-def $w0, debug-location !24; kk3.c:6:5
  ADJCALLSTACKUP 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  RET_ReallyLR debug-location !25; kk3.c:8:1

Once MI ( %1:gpr64(s64) = COPY %0:gpr64(p0), debug-location !23; kk3.c:7:1)
is getting erased based on condition at line 176 and It is marked the DBG_VALUE for removal.
This is the dump

# *** IR Dump After InstructionSelect ***:
# Machine code for function foo: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected
Function Live Ins: $x0

bb.1.entry:
  liveins: $x0
  %0:gpr64 = COPY $x0
  %4:gpr64 = MOVaddr target-flags(aarch64-page) @.str, target-flags(aarch64-pageoff, aarch64-nc) @.str, debug-location !DILocation(line: 0, scope: !9); kk3.c:0
  DBG_VALUE $noreg, $noreg, !"ms", !DIExpression(DW_OP_deref), debug-location !22; kk3.c:5:19 line no:5
  ADJCALLSTACKDOWN 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  $x0 = COPY %4:gpr64, debug-location !24; kk3.c:6:5
  $x1 = COPY %0:gpr64, debug-location !24; kk3.c:6:5
  BL @printf, <regmask $fp $lr $b8 $b9 $b10 $b11 $b12 $b13 $b14 $b15 $d8 $d9 $d10 $d11 $d12 $d13 $d14 $d15 $h8 $h9 $h10 $h11 $h12 $h13 $h14 $h15 $s8 $s9 $s10 $s11 $s12 $s13 $s14 and 51 more...>, implicit-def $lr, implicit $sp, implicit $x0, implicit $x1, implicit-def $w0, debug-location !24; kk3.c:6:5
  ADJCALLSTACKUP 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  RET_ReallyLR debug-location !25; kk3.c:8:1

Which makes difficult to debug even unoptimized code.

[kamleshbhalui]

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

177–178

since it is unoptimized it should not mark debug uses as undef.

[vsk]

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

177–178

I'm not sure there's something special about unoptimized code here. GIsel seems to eliminate redundant copies even at -O0 (maybe to reduce compile time).

Oh, I think I see what's happening now. When GISel sees a redundant copy, it deletes it /after/ replacing vreg uses of DstReg with SrcReg, at which point valid debug uses of SrcReg are accidentally deleted.

Prior to the replaceRegWith call, are all debug uses of DstReg guaranteed to be dominated by the def of SrcReg? If so, could you leave a comment explaining why?

If not, for this particular test case, I'd expect that swapping the lines MRI.replaceRegWith(DstReg, SrcReg); and MI.eraseFromParentAndMarkDBGValuesForRemoval(); would also cause the test to pass.

[dsanders]

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

177–178

since it is unoptimized it should not mark debug uses as undef.

Whether it's optimized or not isn't really the important factor there. It's whether the data remains valid (or is repairable) or not. Having looked at your test case, I don't see a reason why the data is invalid so we should keep it.

I'm not sure there's something special about unoptimized code here. GIsel seems to eliminate redundant copies even at -O0 (maybe to reduce compile time).

It's partly compile time and partly because they block matches that span multiple MI's.

If not, for this particular test case, I'd expect that swapping the lines MRI.replaceRegWith(DstReg, SrcReg); and MI.eraseFromParentAndMarkDBGValuesForRemoval(); would also cause the test to pass.

Are you sure? I think it would have the same effect as it's the MI.eraseFromParentAndMarkDBGValuesForRemoval() would still remove the %1 = COPY ... and the DBG_VALUE %0, ... and then replace the remaining %1's with %0. This would update the DBG_VALUE %0 to DBG_VALUE %1 but it would then be deleted anyway as it's already been scheduled.

Prior to the replaceRegWith call, are all debug uses of DstReg guaranteed to be dominated by the def of SrcReg? If so, could you leave a comment explaining why?

I'm not convinced that's guaranteed but i also doubt it matters. The effect would be to bind multiple source variables to the same register and I believe that's ok.

[kamleshbhalui]

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

177–178

Yes reordering this two statements

MRI.replaceRegWith(DstReg, SrcReg); MI.eraseFromParentAndMarkDBGValuesForRemoval();

to

MI.eraseFromParentAndMarkDBGValuesForRemoval();
MRI.replaceRegWith(DstReg, SrcReg);

solves the problems(preserve the dbg_value).

[kamleshbhalui]

reversed the ordering.

[dsanders]

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

177–178

I've tried it now and I can see that it does work for this test case but not for a slight variation of it. If the input contains DEBUG_VALUE %1 as in

t.mir5 KBDownload

then the original code produces:

DBG_VALUE $noreg, $noreg, !21, !DIExpression(DW_OP_deref), debug-location !22
DBG_VALUE $noreg, $noreg, <0x7f9725c2ba20>, !DIExpression(DW_OP_deref), debug-location !22

(the hex address there is just because the DILocalVariable isn't referenced in the LLVM-IR), losing both %0 and %1's DBG_VALUE. The reordered code produces:

DBG_VALUE %0, $noreg, !21, !DIExpression(DW_OP_deref), debug-location !22
DBG_VALUE $noreg, $noreg, <0x7fae7b42ba20>, !DIExpression(DW_OP_deref), debug-location !22

preserving the %0 one but still losing %1's. Meanwhile your original patch produces:

DBG_VALUE %0, $noreg, !21, !DIExpression(DW_OP_deref), debug-location !22
DBG_VALUE %0, $noreg, <0x7ff15dd21d40>, !DIExpression(DW_OP_deref), debug-location !22

which looks the most correct to me

[vsk]

Thanks Daniel. If it's safe not to mark dbg values for removal here, that seems like the best option.

[kamleshbhalui]

rebased and updated diff to original patch.
Thanks @vsk and @dsanders for reviewing.

[dsanders]

LGTM

[kamleshbhalui]

I do not have commit access.
can someone commit this?

[dstenb]

In D73159#1839877, @kamleshbhalui wrote:

I do not have commit access.
can someone commit this?

I don't have time for the build bots today, but I can commit it tomorrow morning (European time) unless someone else picks it up before.

[kamleshbhalui]

thanks @dstenb .

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

for (auto MII = std::prev(MBB.end()), Begin = MBB.begin(); !ReachedBegin;) {
         continue;
       Register SrcReg = MI.getOperand(1).getReg();
       Register DstReg = MI.getOperand(0).getReg();
       if (Register::isVirtualRegister(SrcReg) &&
           Register::isVirtualRegister(DstReg)) {
         auto SrcRC = MRI.getRegClass(SrcReg);
         auto DstRC = MRI.getRegClass(DstReg);
         if (SrcRC == DstRC) {
-          MRI.replaceRegWith(DstReg, SrcReg);
           MI.eraseFromParentAndMarkDBGValuesForRemoval();
+          MRI.replaceRegWith(DstReg, SrcReg);
         }
       }
     }
   }
 
 #ifndef NDEBUG
   const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
   // Now that selection is complete, there are no more generic vregs.  Verify

llvm/test/DebugInfo/AArch64/kk3.ll

+; RUN: llc -global-isel=1 -filetype=obj -mtriple=aarch64-linux-gnu -o - %s | llvm-dwarfdump -v - | FileCheck %s
+;
+; CHECK: .debug_info contents:
+; CHECK: DW_TAG_formal_parameter
+; CHECK: DW_AT_location
+
+
+%struct.mystruct = type { double, double, double, double, double }
+
+@.str = private unnamed_addr constant [5 x i8] c"%llu\00", align 1
+define dso_local void @foo(%struct.mystruct* %ms) !dbg !9 {
+entry:
+  call void @llvm.dbg.declare(metadata %struct.mystruct* %ms, metadata !21, metadata !DIExpression()), !dbg !22
+  %0 = ptrtoint %struct.mystruct* %ms to i64, !dbg !23
+  %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0), i64 %0), !dbg !24
+  ret void, !dbg !25
+}
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+declare dso_local i32 @printf(i8*, ...)
+
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: ".", directory: ".")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIBasicType(name: "long long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!5 = !{i32 7, !"Dwarf Version", i32 4}
+!6 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = !{i32 1, !"wchar_size", i32 4}
+!8 = !{!"clang version 10.0.0"}
+!9 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !10, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!10 = !DISubroutineType(types: !11)
+!11 = !{null, !12}
+!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "mystruct", file: !1, line: 3, baseType: !13)
+!13 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !1, line: 1, size: 320, elements: !14)
+!14 = !{!15, !17, !18, !19, !20}
+!15 = !DIDerivedType(tag: DW_TAG_member, name: "a", scope: !13, file: !1, line: 2, baseType: !16, size: 64)
+!16 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!17 = !DIDerivedType(tag: DW_TAG_member, name: "b", scope: !13, file: !1, line: 2, baseType: !16, size: 64, offset: 64)
+!18 = !DIDerivedType(tag: DW_TAG_member, name: "c", scope: !13, file: !1, line: 2, baseType: !16, size: 64, offset: 128)
+!19 = !DIDerivedType(tag: DW_TAG_member, name: "d", scope: !13, file: !1, line: 2, baseType: !16, size: 64, offset: 192)
+!20 = !DIDerivedType(tag: DW_TAG_member, name: "e", scope: !13, file: !1, line: 2, baseType: !16, size: 64, offset: 256)
+!21 = !DILocalVariable(name: "ms", arg: 1, scope: !9, file: !1, line: 5, type: !12)
+!22 = !DILocation(line: 5, column: 19, scope: !9)
+!23 = !DILocation(line: 7, column: 1, scope: !9)
+!24 = !DILocation(line: 6, column: 5, scope: !9)
+!25 = !DILocation(line: 8, column: 1, scope: !9)