[entry values] ARM: Add a describeLoadedValue override (PR45025)

[vsk]

Summary

As a narrow stopgap for the assertion failure described in PR45025, add
a describeLoadedValue override to ARMBaseInstrInfo and use it to detect
copies in which the forwarding reg is a super/sub reg of the copy
destination. For the moment this is unsupported.

Several follow ups are possible:

1. Handle VORRq. At the moment, we do not, because isCopyInstrImpl returns early when !MI.isMoveReg().

2. In the case where forwarding reg is a super-reg of the copy destination, we should be able to describe the forwarding reg as a subreg within the copy destination. I'm not 100% sure about this, but it looks like that's what's done in AArch64InstrInfo.

3. In the case where the forwarding reg is a sub-reg of the copy destination, maybe we could describe the forwarding reg using the copy destinaion and a DW_OP_LLVM_fragment (I guess this should be possible after D75036).

https://bugs.llvm.org/show_bug.cgi?id=45025
rdar://59772698

[aprantl]

You may be able to run delta on the MIR testcases.

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

108

doxygen comment?

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir

47

we probably don't need most of these?

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir

39

or these?

[vsk]

Delete some unnecessary attributes.

[vsk]

It looks like the MIR test cases are fairly reduced now, I can give it a shot if there's obvious cruft left?

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

108

I'm not sure what's best to add, here. It looks like the practice has been to copy the doxygen comment from the base class virtual method declaration, but then the documentation could get out of sync.

[vsk]

Add a doxygen comment for the describeLoadedValue override.

[aprantl]

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

108

Sorry, I missed the override! I'm actually against adding comments to override methods unless they do something unexpected that is not documented in the base class.

[efriedma]

Oh, now I see what's happening, I think. We're trying to get a description of some double register (like d8). We find the VMOVS because it "defines" d8. But even though VMOVS is a COPY, it's a COPY defining s17, not d8. (Actually, I think there's a missing implicit use, since it's combining s16 and s17 to produce d8.)

Given that, I guess this is the right fix.

This is sort of similar to what the AArch64 code is doing, but not really. There, the high bits are implicitly zeroed, so we don't need to worry about finding some other instruction. Here, we would actually have to merge the definition of s16 with the definition of s17 to produce anything useful.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

1040

Could you simplify this to just if (DstReg != Reg)?

[vsk]

Simplify, per Eli's suggestion.

[djtodoro]

LGTM, thanks! (nits included)

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir

41

This is unused, so we can delete it.

72

We can attach only to !23.

81

We do not need all these MF attributes (I think only 'name' and 'callSites').

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir

69

We do not need all these MF attributes (I think only 'name' and 'callSites').

[dstenb]

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

1043–1044

This is a problem we're having for our downstream target also, as parameters may be produced by bundled instructions modifying different parts of the register. I have thought a bit about adding an optional Fragment or SubReg field to ParamLoadedValue, to specify which part of the register that was described, and let collectCallSiteParameters() keep the parameter in the worklist until all parts have been described, but I have not really thought more about how that could be implemented.

[vsk]

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

1043–1044

Interesting. I tried something along these lines to try and have collectCallSiteParameters "merge" the descriptions for different fragments of a forwarding reg. This turned out to be a good bit more complicated than what I had bargained for..

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir

72

I'll clean up the test a bit before committing, but we really need a tool to do this kind of reduction. Having narrow/reduced test cases is important, but doing this can cost a fairly outsized amount of engineering time relative to the benefit it delivers.

The number of debug info contributors has grown a lot the past few years, and I hope it keeps growing. For that to happen though we need for an off-the-shelf tool that can help reduce tests. I'll try to prototype something soon.

[djtodoro]

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir

72

I agree. Such utility could be very useful, and I would like to participate. Thanks!

llvm/lib/CodeGen/TargetInstrInfo.cpp

TargetInstrInfo::describeLoadedValue(const MachineInstr &MI,
   // To simplify the sub-register handling, verify that we only need to
   // consider physical registers.
   assert(MF->getProperties().hasProperty(
       MachineFunctionProperties::Property::NoVRegs));
 
   if (auto DestSrc = isCopyInstr(MI)) {
     Register DestReg = DestSrc->Destination->getReg();
 
+    // If the copy destination is the forwarding reg, describe the forwarding
+    // reg using the copy source as the backup location. Example:
+    //
+    //   x0 = MOV x7
+    //   call callee(x0)      ; x0 described as x7
     if (Reg == DestReg)
       return ParamLoadedValue(*DestSrc->Source, Expr);
 
     // Cases where super- or sub-registers needs to be described should
     // be handled by the target's hook implementation.
     assert(!TRI->isSuperOrSubRegisterEq(Reg, DestReg) &&
            "TargetInstrInfo::describeLoadedValue can't describe super- or "
            "sub-regs for copy instructions");

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

MachineInstr *commuteInstructionImpl(MachineInstr &MI, bool NewMI,
                                        unsigned OpIdx1,
                                        unsigned OpIdx2) const override;
   /// If the specific machine instruction is an instruction that moves/copies
   /// value from one register to another register return destination and source
   /// registers as machine operands.
   Optional<DestSourcePair>
   isCopyInstrImpl(const MachineInstr &MI) const override;
 
+  /// Specialization of \ref TargetInstrInfo::describeLoadedValue, used to
+  /// enhance debug entry value descriptions for ARM targets.
+  Optional<ParamLoadedValue> describeLoadedValue(const MachineInstr &MI,
+                                                 Register Reg) const override;
+
 public:
   // Return whether the target has an explicit NOP encoding.
   bool hasNOP() const;
 
   // Return the non-pre/post incrementing version of 'Opc'. Return 0
   // if there is not such an opcode.
   virtual unsigned getUnindexedOpcode(unsigned Opc) const = 0;
 

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

ARMBaseInstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
   // the same register.
   if (!MI.isMoveReg() ||
       (MI.getOpcode() == ARM::VORRq &&
        MI.getOperand(1).getReg() != MI.getOperand(2).getReg()))
     return None;
   return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
 }
 
+Optional<ParamLoadedValue>
+ARMBaseInstrInfo::describeLoadedValue(const MachineInstr &MI,
+                                      Register Reg) const {
+  if (auto DstSrcPair = isCopyInstrImpl(MI)) {
+    Register DstReg = DstSrcPair->Destination->getReg();
+
+    // TODO: We don't handle cases where the forwarding reg is narrower/wider
+    // than the copy registers. Consider for example:
+    //
+    //   s16 = VMOVS s0
+    //   s17 = VMOVS s1
+    //   call @callee(d0)
+    //
+    // We'd like to describe the call site value of d0 as d8, but this requires
+    // gathering and merging the descriptions for the two VMOVS instructions.
+    //
+    // We also don't handle the reverse situation, where the forwarding reg is
+    // narrower than the copy destination:
+    //
+    //   d8 = VMOVD d0
+    //   call @callee(s1)
+    //
+    // We need to produce a fragment description (the call site value of s1 is
+    // /not/ just d8).
+    if (DstReg != Reg)
+      return None;
+  }
+  return TargetInstrInfo::describeLoadedValue(MI, Reg);
+}
+
 const MachineInstrBuilder &
 ARMBaseInstrInfo::AddDReg(MachineInstrBuilder &MIB, unsigned Reg,
                           unsigned SubIdx, unsigned State,
                           const TargetRegisterInfo *TRI) const {
   if (!SubIdx)
     return MIB.addReg(Reg, State);
 
   if (Register::isPhysicalRegister(Reg))

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir

+# RUN: llc -O1 -debug-entry-values -filetype=obj -mtriple thumbv7em-apple-unknown-macho -start-after=machineverifier %s -o %t.o
+# RUN: llvm-dwarfdump %t.o | FileCheck %s
+
+# Crash test, reduced from:
+#
+# a(float);
+# b(double c) {
+#   d();
+#   a(c);
+# }
+#
+# Some minor surgery was performed on the MIR to remove a call to a truncating
+# cast builtin between the calls to "d" and "a", and to force d0 as the copy dst
+# and s0 as the forwarding reg.
+#
+# This was done to test the case where the copy dst (d0) is a super-reg of the
+# forwarding reg (s0).
+
+# CHECK: DW_TAG_GNU_call_site
+# CHECK-NEXT: DW_AT_abstract_origin {{.*}} "a"
+# CHECK-NOT: call_site_parameter
+
+--- |
+  target datalayout = "e-m:o-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "thumbv7em-apple-unknown-macho"
+  ; Function Attrs: nounwind optsize uwtable
+  define arm_aapcs_vfpcc i32 @b(double %c) local_unnamed_addr #0 !dbg !16 {
+  entry:
+    call void @llvm.dbg.value(metadata double %c, metadata !21, metadata !DIExpression()), !dbg !22
+    %call = tail call arm_aapcs_vfpcc i32 bitcast (i32 (...)* @d to i32 ()*)(), !dbg !23
+    %conv = fptrunc double %c to float, !dbg !24
+    %call1 = tail call arm_aapcs_vfpcc i32 @a(float %conv), !dbg !25
+    ret i32 undef, !dbg !26
+  }
+  declare arm_aapcs_vfpcc i32 @d(...) local_unnamed_addr #0
+  declare !dbg !4 arm_aapcs_vfpcc i32 @a(float) local_unnamed_addr #0
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  declare void @llvm.stackprotector(i8*, i8**)
+
+  attributes #0 = { "disable-tail-calls"="false" "frame-pointer"="all" }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!10, !11, !12, !13, !14}
+  !llvm.ident = !{!15}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0 ([e-mail]:apple/llvm-project.git 6203ec90d0bb0fd705fc0ad00ee263e5579aa709)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+  !1 = !DIFile(filename: "jnyn_freeBSD.c", directory: "/Users/vsk/tmp/jnyn")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "a", scope: !5, file: !5, line: 1, type: !6, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DIFile(filename: "unreduced2.c", directory: "/Users/vsk/tmp/jnyn")
+  !6 = !DISubroutineType(types: !7)
+  !7 = !{!8, !9}
+  !8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !9 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+  !10 = !{i32 7, !"Dwarf Version", i32 4}
+  !11 = !{i32 2, !"Debug Info Version", i32 3}
+  !12 = !{i32 1, !"wchar_size", i32 4}
+  !13 = !{i32 1, !"min_enum_size", i32 4}
+  !14 = !{i32 7, !"PIC Level", i32 2}
+  !15 = !{!"clang version 10.0.0 ([e-mail]:apple/llvm-project.git 6203ec90d0bb0fd705fc0ad00ee263e5579aa709)"}
+  !16 = distinct !DISubprogram(name: "b", scope: !5, file: !5, line: 2, type: !17, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !20)
+  !17 = !DISubroutineType(types: !18)
+  !18 = !{!8, !19}
+  !19 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+  !20 = !{!21}
+  !21 = !DILocalVariable(name: "c", arg: 1, scope: !16, file: !5, line: 2, type: !19)
+  !22 = !DILocation(line: 0, scope: !16)
+  !23 = !DILocation(line: 3, column: 3, scope: !16)
+  !24 = !DILocation(line: 4, column: 5, scope: !16)
+  !25 = !DILocation(line: 4, column: 3, scope: !16)
+  !26 = !DILocation(line: 5, column: 1, scope: !16)
+
+...
+---
+name:            b
+callSites:
+  - { bb: 0, offset: 12, fwdArgRegs: [] }
+  - { bb: 0, offset: 15, fwdArgRegs:
+      - { arg: 0, reg: '$s0' } }
+body:             |
+  bb.0.entry:
+    liveins: $d0, $lr, $d8
+
+    DBG_VALUE $d0, $noreg, !21, !DIExpression(), debug-location !22
+    frame-setup tPUSH 14, $noreg, $r7, killed $lr, implicit-def $sp, implicit $sp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 8
+    frame-setup CFI_INSTRUCTION offset $lr, -4
+    frame-setup CFI_INSTRUCTION offset $r7, -8
+    $r7 = frame-setup tMOVr $sp, 14, $noreg
+    frame-setup CFI_INSTRUCTION def_cfa_register $r7
+    $sp = frame-setup VSTMDDB_UPD $sp, 14, $noreg, killed $d8
+    frame-setup CFI_INSTRUCTION offset $d8, -16
+    $s16 = VMOVS killed $s0, 14, $noreg
+    $s17 = VMOVS killed $s1, 14, $noreg, implicit-def $d8
+    DBG_VALUE $d8, $noreg, !21, !DIExpression(), debug-location !22
+    tBL 14, $noreg, @d, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp, implicit-def dead $r0, debug-location !23
+    $d0 = VMOVD killed $d8, 14, $noreg, debug-location !24
+    DBG_VALUE $d0, $noreg, !21, !DIExpression(), debug-location !22
+    tTAILJMPd @a, 14, $noreg, implicit $sp, implicit $sp, implicit killed $s0, debug-location !25
+
+...

llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir

+# RUN: llc -O1 -debug-entry-values -filetype=obj -mtriple thumbv7em-apple-unknown-macho -start-after=machineverifier %s -o %t.o
+# RUN: llvm-dwarfdump %t.o | FileCheck %s
+
+# Crash test, reduced from:
+#
+# a(double);
+# b(double c) {
+#   d();
+#   a(c);
+# }
+
+# CHECK: DW_TAG_GNU_call_site
+# CHECK-NEXT: DW_AT_abstract_origin {{.*}} "a"
+# CHECK-NOT: call_site_parameter
+
+--- |
+  ; ModuleID = 'unreduced.c'
+  source_filename = "unreduced.c"
+  target datalayout = "e-m:o-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "thumbv7em-apple-unknown-macho"
+  ; Function Attrs: nounwind optsize uwtable
+  define arm_aapcs_vfpcc i32 @b(double %c) local_unnamed_addr #0 !dbg !16 {
+  entry:
+    call void @llvm.dbg.value(metadata double %c, metadata !18, metadata !DIExpression()), !dbg !19
+    %call = tail call arm_aapcs_vfpcc i32 bitcast (i32 (...)* @d to i32 ()*)(), !dbg !20
+    %call1 = tail call arm_aapcs_vfpcc i32 @a(double %c), !dbg !21
+    ret i32 undef, !dbg !22
+  }
+  declare arm_aapcs_vfpcc i32 @d(...) local_unnamed_addr #0
+  declare !dbg !4 arm_aapcs_vfpcc i32 @a(double) local_unnamed_addr #0
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  declare void @llvm.stackprotector(i8*, i8**)
+
+  attributes #0 = { "disable-tail-calls"="false" "frame-pointer"="all" }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!10, !11, !12, !13, !14}
+  !llvm.ident = !{!15}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0 ([e-mail]:apple/llvm-project.git 6203ec90d0bb0fd705fc0ad00ee263e5579aa709)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+  !1 = !DIFile(filename: "jnyn_freeBSD.c", directory: "/Users/vsk/tmp/jnyn")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "a", scope: !5, file: !5, line: 1, type: !6, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DIFile(filename: "unreduced.c", directory: "/Users/vsk/tmp/jnyn")
+  !6 = !DISubroutineType(types: !7)
+  !7 = !{!8, !9}
+  !8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !9 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+  !10 = !{i32 7, !"Dwarf Version", i32 4}
+  !11 = !{i32 2, !"Debug Info Version", i32 3}
+  !12 = !{i32 1, !"wchar_size", i32 4}
+  !13 = !{i32 1, !"min_enum_size", i32 4}
+  !14 = !{i32 7, !"PIC Level", i32 2}
+  !15 = !{!"clang version 10.0.0 ([e-mail]:apple/llvm-project.git 6203ec90d0bb0fd705fc0ad00ee263e5579aa709)"}
+  !16 = distinct !DISubprogram(name: "b", scope: !5, file: !5, line: 2, type: !6, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !17)
+  !17 = !{!18}
+  !18 = !DILocalVariable(name: "c", arg: 1, scope: !16, file: !5, line: 2, type: !9)
+  !19 = !DILocation(line: 0, scope: !16)
+  !20 = !DILocation(line: 3, column: 3, scope: !16)
+  !21 = !DILocation(line: 4, column: 3, scope: !16)
+  !22 = !DILocation(line: 5, column: 1, scope: !16)
+
+...
+---
+name:            b
+callSites:
+  - { bb: 0, offset: 12, fwdArgRegs: [] }
+  - { bb: 0, offset: 18, fwdArgRegs:
+      - { arg: 0, reg: '$d0' } }
+body:             |
+  bb.0.entry:
+    liveins: $d0, $lr, $d8
+
+    DBG_VALUE $d0, $noreg, !18, !DIExpression(), debug-location !19
+    frame-setup tPUSH 14, $noreg, $r7, killed $lr, implicit-def $sp, implicit $sp
+    frame-setup CFI_INSTRUCTION def_cfa_offset 8
+    frame-setup CFI_INSTRUCTION offset $lr, -4
+    frame-setup CFI_INSTRUCTION offset $r7, -8
+    $r7 = frame-setup tMOVr $sp, 14, $noreg
+    frame-setup CFI_INSTRUCTION def_cfa_register $r7
+    $sp = frame-setup VSTMDDB_UPD $sp, 14, $noreg, killed $d8
+    frame-setup CFI_INSTRUCTION offset $d8, -16
+    $s16 = VMOVS killed $s0, 14, $noreg
+    $s17 = VMOVS killed $s1, 14, $noreg, implicit-def $d8
+    DBG_VALUE $d8, $noreg, !18, !DIExpression(), debug-location !19
+    tBL 14, $noreg, @d, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp, implicit-def dead $r0, debug-location !20
+    $s0 = VMOVS killed $s16, 14, $noreg, debug-location !21
+    $s1 = VMOVS killed $s17, 14, $noreg, implicit-def $d0, debug-location !21
+    DBG_VALUE $d0, $noreg, !18, !DIExpression(), debug-location !19
+    $sp = VLDMDIA_UPD $sp, 14, $noreg, def $d8, debug-location !21
+    $sp = t2LDMIA_UPD $sp, 14, $noreg, def $r7, def $lr, debug-location !21
+    tTAILJMPd @a, 14, $noreg, implicit $sp, implicit $sp, implicit killed $d0, debug-location !21
+
+...