[OPT_DPG][LIVEDEBUGVALUES] Teach Live Debug Values About Meta Instructions

[TWeaver]

Summary

Previously LiveDebugValues pass would consider meta instructions that 'fiddle' with liveness of registers as register definitions when transfering register defs. This would mean that, for example, a KILL instruction would cause LiveDebugValues to terminate the range of an earlier DBG_VALUE instruction resulting in the none propogation of said DBG_VALUE instructions into later blocks.

This patch adds the check and a helpful comment, fixes a test that previously tested for the broken behaviour by coincidence and adds a test specifically for this.

Thanks for your time reviewing!

[djtodoro]

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

What is the intention here?

[TWeaver]

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

The KILL was being picked up as a def in the test and was causing a DBG_VALUE instruction to be created. This was tested for in the check list. Ignoring KILL instructions means that livedebugvalues wasn't creating the second entry DBG_VALUE for ARG_A anymore.

In order to further preserve what I figured was intended behaviour in the test I changed it to a concrete register def.

The original entry value patch, SFAIK, had nothing to do with KILL/meta instructions. I figured the KILL instruction was there by coincidence rather than intention.

happy to reconsider this if that's not the case.

Cheers
Tom W

[vsk]

LGTM, but please wait for another +1.

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

This seems like a reasonable change, we just want a real def here. We could just remove the second CHECK for ARG_A, but that arguably drops code coverage.

[djtodoro]

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

I see the result, but I don't think we should change the code generated here, since it was produced from the C code provided in the source code of the test. Now we have the two real identical defs for the $edi..
Anyhow, the main intention in the test was to test the ARG_B doesn't have the entry_val generated, since its entry value gets modified, and it seems reasonable to me to just remove the first entry_val check for the ARG_A.
@vsk Sounds reasonable?

@TWeaver Thanks for this! Besides this, lgtm! (but I am not listed as a reviewer)

[TWeaver]

I missed adding you as a reviewer @djtodoro , my apologies. This has been rectified.

[TWeaver]

KILL instruction is no reinstated and failing check line has been removed.

[vsk]

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

Seems fine to me.

[dstenb]

(Just minor comments about tags used in the to-be (?) commit message.)

There is a typo in [OPT_DPG]. OPT[_-]DBG seems to be uncommon. I'd personally prefer [DebugInfo] as that is an established tag. I'd also prefer writing LIVEDEBUGVALUES in camel case instead: [LiveDebugValues].

[TWeaver]

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

great, that works for me.

It was always a case of remove the check or change the instructions.

happy to have gotten a more definitive decision on which one was the best approach.

I'll get this changed out re-uploaded. an official LGTM would be great and I'll get his pushed today.

thanks again!
Tom W

[TWeaver]

Hi David,

thanks for your review and for spotting out the typos.

I'll ensure to use the correct tags from here on out and of course use the camel case spelling for LiveDebugValues.

I'll probably land this tomorrow as it's home time soon and I'd like to avoid being potentially pinged by build bots all evening.

thanks again
Tom W

[djtodoro]

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

106

Good! Thanks!

llvm/lib/CodeGen/LiveDebugValues.cpp

void LiveDebugValues::insertTransferDebugPair(
   }
   llvm_unreachable("Invalid transfer kind");
 }
 
 /// A definition of a register may mark the end of a range.
 void LiveDebugValues::transferRegisterDef(
     MachineInstr &MI, OpenRangesSet &OpenRanges, VarLocMap &VarLocIDs,
     TransferMap &Transfers) {
+
+  // Meta Instructions do not affect the debug liveness of any register they
+  // define.
+  if (MI.isMetaInstruction())
+    return;
+
   MachineFunction *MF = MI.getMF();
   const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
   unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
   SparseBitVector<> KillSet;
   for (const MachineOperand &MO : MI.operands()) {
     // Determine whether the operand is a register def.  Assume that call
     // instructions never clobber SP, because some backends (e.g., AArch64)
     // never list SP in the regmask.

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir

bb.0.entry:
     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !20
     DBG_VALUE $esi, $noreg, !17, !DIExpression(), debug-location !20
     DBG_VALUE $edx, $noreg, !18, !DIExpression(), debug-location !20
     frame-setup PUSH64r killed $rbx, implicit-def $rsp, implicit $rsp
     CFI_INSTRUCTION def_cfa_offset 16
     CFI_INSTRUCTION offset $rbx, -16
     $ebx = MOV32rr $esi
     DBG_VALUE $ebx, $noreg, !17, !DIExpression(), debug-location !20
-    renamable $edi = KILL $edi, implicit-def $rdi
+    $edi = MOV32ri 5, debug-location !22
     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !20
     renamable $edx = LEA64_32r killed renamable $rdi, 1, $noreg, 2, $noreg, debug-location !21
     DBG_VALUE $edx, $noreg, !19, !DIExpression(), debug-location !20
     $edi = MOV32ri 5, debug-location !22
     $esi = MOV32ri 6, debug-location !22
     CALL64pcrel32 @fn1, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit $esi, implicit $edx, implicit-def $rsp, implicit-def $ssp, debug-location !22
     DBG_VALUE $ebx, $noreg, !17, !DIExpression(DW_OP_plus_uconst, 7, DW_OP_stack_value), debug-location !20
     renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !23
     CMP32ri8 killed renamable $ebx, 10, implicit-def $eflags, debug-location !23
     renamable $al = SETCCr 12, implicit killed $eflags, implicit killed $eax, implicit-def $eax, debug-location !23
     $rbx = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !26
     CFI_INSTRUCTION def_cfa_offset 8, debug-location !26
     RETQ $eax, debug-location !26
 
 ...

llvm/test/DebugInfo/MIR/X86/livedebugvalues-ignores-metaInstructions.mir

+--- |
+  ; RUN: llc %s -march=x86-64 -run-pass=livedebugvalues -o - | FileCheck %s -implicit-check-not=DBG_VALUE
+
+  ; Check that live-debug-values ignores meta instructions.
+
+  ; CHECK-LABEL: bb.0.entry:
+  ; CHECK:       DBG_VALUE $ebx, $noreg, !16, !DIExpression(), debug-location !17
+  ; CHECK-LABEL: bb.1.bb1:
+  ; CHECK:       DBG_VALUE $ebx, $noreg, !16, !DIExpression(), debug-location !17
+  ; CHECK-LABEL: bb.2.bb2:
+  ; CHECK:       DBG_VALUE $ebx, $noreg, !16, !DIExpression(), debug-location !17
+  ; CHECK-LABEL: bb.3.bb3:
+  ; CHECK:       DBG_VALUE $ebx, $noreg, !16, !DIExpression(), debug-location !17
+
+  define i32 @_Z8bb_to_bb() local_unnamed_addr !dbg !12 {
+  entry:
+    br label %bb1, !dbg !17
+  bb1:
+    br label %bb2, !dbg !17
+  bb2:
+    br label %bb3, !dbg !17
+  bb3:
+    ret i32 0, !dbg !17
+  }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!7, !8, !9, !10}
+  !llvm.ident = !{!11}
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3, debugInfoForProfiling: true, nameTableKind: None)
+  !1 = !DIFile(filename: "main.cpp", directory: "F:\")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DIGlobalVariableExpression(var: !5, expr: !DIExpression())
+  !5 = distinct !DIGlobalVariable(name: "start", scope: !0, file: !1, line: 4, type: !6, isLocal: false, isDefinition: true)
+  !6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !7 = !{i32 2, !"Dwarf Version", i32 4}
+  !8 = !{i32 2, !"Debug Info Version", i32 3}
+  !9 = !{i32 1, !"wchar_size", i32 2}
+  !10 = !{i32 7, !"PIC Level", i32 2}
+  !11 = !{!"clang version 10.0.0"}
+  !12 = distinct !DISubprogram(name: "bb_to_bb", linkageName: "bb_to_bb", scope: !1, file: !1, line: 6, type: !13, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !15)
+  !13 = !DISubroutineType(types: !14)
+  !14 = !{!6, !6}
+  !15 = !{!16}
+  !16 = !DILocalVariable(name: "myVar", scope: !12, file: !1, line: 7, type: !6)
+  !17 = !DILocation(line: 10, scope: !12)
+
+...
+---
+name:            _Z8bb_to_bb
+body:             |
+  bb.0.entry:
+    successors: %bb.1
+    $ebx = MOV32ri 0, debug-location !17
+    DBG_VALUE $ebx, $noreg, !16, !DIExpression(), debug-location !17
+  bb.1.bb1:
+    successors: %bb.2
+    renamable $ebx = KILL $ebx
+  bb.2.bb2:
+    successors: %bb.3
+    renamable $ebx = KILL $ebx
+  bb.3.bb3:
+    RETQ $eax, debug-location !17
+...