[DBG] Teach DebugEntityHistoryCalculator about Kill instructions.

[TWeaver]

Summary

Potential Fix for PR38753 - Missing return value issue.

Noop Kill instructions would be classified as a clobber of the register they kill by the debug entity calculator. This would in turn close the range of a preceding debug intrinsic that could potentially have it's range extended past the KILL instructions invocation.

This patch address this issue by making the Debug History Entity Calculator ignore Kill instructions when calculating location list ranges. Two tests failed as result, these have now been fixed up and a new test has been added that specifically tests for this exact case.

Happy reviewing,
Tom W

[TWeaver]

moved Kill instruction check to a more relevant place

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

272–273

At a quick glance one could think that the isKill() condition is related to the above, so I think that the comment needs to be updated (or the isKill() condition moved to its own if statement).

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

272–273

Disregard the above. I should perhaps F5 before submitting. :)

[TWeaver]

Thanks David, it was quickly pointed out to me internally that there's a better place for the check so I rushed to get a quick build and test done before updating.

thanks for your time reviewing this patch.

[djtodoro]

llvm/test/DebugInfo/X86/kill_instruction_doesnt_close_dbg_intrinsic_range.mir

20

The attributes are removed, so we can get rid of the #N.

22

This is unused here, so it can be deleted.

[TWeaver]

removed superflous attributes from function declarations.

[TWeaver]

llvm/test/DebugInfo/X86/kill_instruction_doesnt_close_dbg_intrinsic_range.mir

20

thanks for the spot. removed.

22

thanks for the spot. removed.

[TWeaver]

A very gentle and polite ping.

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

265

this needs a comment. I don't really understand the reasoning in the patch description either, so if you could explain why this is safe/correct, that would help me!

[vsk]

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

265

@TWeaver apologies for the reverse-ping, but I'm also having some trouble following this and think that an explanatory comment would help.

[TWeaver]

not a problem gents, I've been summarising my thoughts and triple checking to make sure this is a sound thing to do and have a response with updated patch and (hopefully) additional tests incoming shortly.

[TWeaver]

Apologies for the lack lustre description and vagueness of all this. Also, this is late coming due to sickness. My deepest and humblest apologies. Hope this clear things up though! enjoy.

Why it's safe to ignore KILL instructions when generating live debug location list ranges.

Essentially KILL instructions are NOOPs. They do not change the bit fields of any of the registers they touch. They merely adjust the liveness of a register or subregister to indicate to future consumers that the liveness of some value in a register has changed.

This is mostly useful for handling cases where, before KILL instructions are inserted, an assignment from a super register to a subregister takes place. A cases where some 64-bit maths takes place to compute a value that is then returned in a 32-bit register (for some arbitrary case) could be as follows:

0 // psuedo for ease of reading.
1 RAX = Do 64-bit computation and store value
2 assign value in RAX to EAX
3 return EAX

step 2 of the above example could, in some cases, be a NOOP and a completely unnecessary step as the computed value already exists in EAX. There is a semantic meaning to assignment from RAX to EAX that we want to preserve, but, we don't want to perform a reassignment into a register that already holds the same value. As a result, this assignment is replaced with a KILL instruction that describes the assignment from RAX to EAX. The above example would look something like this:

0 // psuedo for ease of reading.
1 RAX = Do 64-bit computation and store value
2 EAX = KILL killed RAX, RAX
3 return EAX

This now indicates that the value we originally held in RAX is no longer available past the point of the KILL but without explicitly performing the assignment/move/copy from super-register to sub-register.

This is great for generating live information about values in registers for passes and optmisations that care about the liveness of said values. But in the case of Debugging Information, we want to preserve the liveness of some value for as long as possible. Therefore, it doesn't matter if the live range of some value in a super register is no longer useable for some computation, what matters is that the value of the register remains unchanged and is still safe to read in a debugger.

Any changes to the value of said register by some operation will be picked up by the debug entity history calculator which will soundly terminate whenever it's clobbered, however, a KILL instruction does not actually clobber anything (it's merely a liveness indicator/instruction/adjuster/meta-thing).

[TWeaver]

Added a comment to the KILL instruction check.

Also added another test that shows what "type" of liveness we care about when dealing with KILL instructions.

[djtodoro]

llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_at_return.mir

22 ↗

(On Diff #234063)

Unused, so it can be deleted.

28 ↗

(On Diff #234063)

You can trim the (PS4 clang version 99.99.0.80625 eb745f63 checking eb745f63f19fe3fe07db98b2e183809e65117646) part as well.

llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_still_clobbers.mir

23 ↗

(On Diff #234063)

Unused, so it can be deleted.

29 ↗

(On Diff #234063)

Same as above.

[TWeaver]

removed vendor specific string from debug information

[TWeaver]

removed unused declarations for stack protection.

[TWeaver]

llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_still_clobbers.mir

23 ↗

(On Diff #234063)

do you mean the newline or the stackprotector? I've removed the stack protector but this comment may have been on the wrong line.

29 ↗

(On Diff #234063)

I can't remove this line without the tests failing.

[djtodoro]

Thanks for addressing the comments.

llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_still_clobbers.mir

23 ↗

(On Diff #234063)

I was going to mark the line for the stackprotector. I don’t know how it came here :)

29 ↗

(On Diff #234063)

The comment was referring the vendor name for the compiler version. Thanks.

[vsk]

In D70497#1785975, @TWeaver wrote:

Essentially KILL instructions are NOOPs. They do not change the bit fields of any of the registers they touch. They merely adjust the liveness of a register or subregister to indicate to future consumers that the liveness of some value in a register has changed.
[snip]

Thanks for explaining, this makes sense to me. Any reason to not use MI.isMetaInstruction() as the guard? That would cause CFI/lifetime/EH_LABEL instructions to be skipped in addition to debug/kill instructions - would that be reasonable?

[aprantl]

In D70497#1785975, @TWeaver wrote:

Apologies for the lack lustre description and vagueness of all this. Also, this is late coming due to sickness. My deepest and humblest apologies. Hope this clear things up though! enjoy.

Why it's safe to ignore KILL instructions when generating live debug location list ranges.

Essentially KILL instructions are NOOPs. They do not change the bit fields of any of the registers they touch. They merely adjust the liveness of a register or subregister to indicate to future consumers that the liveness of some value in a register has changed.

Can you put this into a comment on the condition?

[TWeaver]

Thanks for explaining, this makes sense to me. Any reason to not use MI.isMetaInstruction() as the guard? That would cause CFI/lifetime/EH_LABEL instructions to be skipped in addition to debug/kill instructions - would that be reasonable?

This suggestion makes sense to me, reading from the description of 'isMetaInstruction()' it would seem that it covers any instruction that doesn't produce any real output in the finished program.

I've tried this check and there's no new test failures and I feel satisfied that it's safe to do based on the description.

I've had to rework the comment suggestion from Adrian however as we're now ignoring any meta instruction rather than just debug intrinsics and KILL instructions.

[TWeaver]

Added isMetaInstruction() Check in place of debug intrinsic and KILL check
Added a new comment explaining why this is safe to do.

[vsk]

Thanks, lgtm as well.

[dstenb]

What is the status of this patch? Do you need help with landing it?

[TWeaver]

Hello Everyone,

small mistake on my part. This patch was submitted on the 20/12/2019 with the following git commit hash 453dc4d7ec5a3c3d8f54fc358bc5673834516d48

You can see the patch for yourselves at the following link:

https://reviews.llvm.org/rG453dc4d7ec5a3c3d8f54fc358bc5673834516d48

Thanks again and apologies for the mistake.
Tom W

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

for (const auto &MI : MBB) {
             "Expected inlined-at fields to agree");
         // When collecting debug information for labels, there is no MCSymbol
         // generated for it. So, we keep MachineInstr in DbgLabels in order
         // to query MCSymbol afterward.
         InlinedEntity L(RawLabel, MI.getDebugLoc()->getInlinedAt());
         DbgLabels.addInstr(L, MI);
       }
 
-      if (MI.isDebugInstr())
+      if (MI.isDebugInstr() || MI.isKill())
         continue;
 
       // Not a DBG_VALUE instruction. It may clobber registers which describe
       // some variables.
       for (const MachineOperand &MO : MI.operands()) {
         if (MO.isReg() && MO.isDef() && MO.getReg()) {
           // Ignore call instructions that claim to clobber SP. The AArch64
           // backend does this for aggregate function arguments.
           if (MI.isCall() && MO.getReg() == SP)
             continue;
           // If this is a virtual register, only clobber it since it doesn't
           // have aliases.
           if (Register::isVirtualRegister(MO.getReg()))
             clobberRegisterUses(RegVars, MO.getReg(), DbgValues, LiveEntries,
                                 MI);
           // If this is a register def operand, it may end a debug value

llvm/test/DebugInfo/COFF/pieces.ll

 ; because both fields of 'o' are zero flowing into this block. However, it
 ; appears livedebugvalues doesn't account for fragments.
 
 ; ASM-LABEL: pad_right: # @pad_right
 ; ASM:         movq    %rcx, %rax
 ; ASM: [[pad_right_tmp:\.Ltmp[0-9]+]]:
 ; ASM:         #DEBUG_VALUE: pad_right:o <- [DW_OP_LLVM_fragment 32 32] $eax
 ; ASM:         retq
+; ASM: [[pad_right_end:\.Lfunc_end1]]:
 
 
 ; ASM-LABEL: pad_left: # @pad_left
 ; ASM:         .cv_loc 2 1 24 3                # t.c:24:3
 ; ASM:         movq    %rcx, %rax
 ; ASM: [[pad_left_tmp:\.Ltmp[0-9]+]]:
 ; ASM:         #DEBUG_VALUE: pad_left:o <- [DW_OP_LLVM_fragment 0 32] $eax
 ; ASM:         retq
+; ASM: [[pad_left_end:\.Lfunc_end2]]:
 
 
 ; ASM-LABEL: nested: # @nested
 ; ASM:         #DEBUG_VALUE: nested:o <- [DW_OP_deref] $rcx
 ; ASM:         movl    12(%rcx), %eax
 ; ASM: [[p_start:\.Ltmp[0-9]+]]:
 ; ASM:         #DEBUG_VALUE: nested:p <- [DW_OP_LLVM_fragment 32 32] $eax
 ; ASM:         retq

 ; OBJ:       }
 ; OBJ:       ProcEnd {
 ; OBJ:       }
 
 ; ASM-LABEL:  .short  4423                    # Record kind: S_GPROC32_ID
 ; ASM:        .asciz  "pad_right"             # Function name
 ; ASM:        .short  4414                    # Record kind: S_LOCAL
 ; ASM:        .asciz  "o"
-; ASM:        .cv_def_range    [[pad_right_tmp]] [[pad_right_tmp]], subfield_reg, 17, 4
+; ASM:        .cv_def_range    [[pad_right_tmp]] [[pad_right_end]], subfield_reg, 17, 4
 
 ; OBJ-LABEL: GlobalProcIdSym {
 ; OBJ:         Kind: S_GPROC32_ID (0x1147)
 ; OBJ:         DisplayName: pad_right
 ; OBJ:       }
 ; OBJ:       LocalSym {
 ; OBJ:         VarName: o
 ; OBJ:       }
 ; OBJ:       DefRangeSubfieldRegisterSym {
 ; OBJ:         Register: EAX (0x11)
 ; OBJ:         MayHaveNoName: 0
 ; OBJ:         OffsetInParent: 4
 ; OBJ:         LocalVariableAddrRange {
 ; OBJ:         }
 ; OBJ:       }
 ; OBJ:       ProcEnd {
 ; OBJ:       }
 
 ; ASM-LABEL:  .short  4423                    # Record kind: S_GPROC32_ID
 ; ASM:        .asciz  "pad_left"              # Function name
 ; ASM:        .short  4414                    # Record kind: S_LOCAL
 ; ASM:        .asciz  "o"
-; ASM:        .cv_def_range    [[pad_left_tmp]] [[pad_left_tmp]], subfield_reg, 17, 0
+; ASM:        .cv_def_range    [[pad_left_tmp]] [[pad_left_end]], subfield_reg, 17, 0
 
 ; OBJ-LABEL: GlobalProcIdSym {
 ; OBJ:         Kind: S_GPROC32_ID (0x1147)
 ; OBJ:         DisplayName: pad_left
 ; OBJ:       }
 ; OBJ:       LocalSym {
 ; OBJ:         VarName: o
 ; OBJ:       }

llvm/test/DebugInfo/X86/kill_instruction_doesnt_close_dbg_intrinsic_range.mir

+--- |
+  ; RUN: llc %s -start-before=cfi-instr-inserter -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
+
+  ; Test that KILL instructions no longer terminate the instruction range of a
+  ; Debug Intrinsic calculated by DbgEntityHistoryCalculator.
+
+  source_filename = ".\\test.cpp"
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-scei-ps4"
+  
+  define hidden i32 @main(i32 %arg, i8** nocapture readnone %argv) local_unnamed_addr #0 !dbg !8 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %arg, metadata !19, metadata !DIExpression()), !dbg !22
+    call void @llvm.dbg.value(metadata i8** %argv, metadata !20, metadata !DIExpression()), !dbg !22
+    %add = shl nsw i32 %arg, 1, !dbg !23
+    call void @llvm.dbg.value(metadata i32 %add, metadata !21, metadata !DIExpression()), !dbg !22
+    ret i32 %add, !dbg !24
+  }
+  
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+  
+  declare void @llvm.stackprotector(i8*, i8**) #2
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5, !6}
+  !llvm.ident = !{!7}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 10.0.0 (PS4 clang version 99.99.0.80625 eb745f63 checking eb745f63f19fe3fe07db98b2e183809e65117646)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, debugInfoForProfiling: true, nameTableKind: None)
+  !1 = !DIFile(filename: "test.cpp", directory: "F:\\test")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 2}
+  !6 = !{i32 7, !"PIC Level", i32 2}
+  !7 = !{!"clang version 10.0.0"}
+  !8 = distinct !DISubprogram(name: "main", scope: !9, file: !9, line: 1, type: !10, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !18)
+  !9 = !DIFile(filename: ".\\test.cpp", directory: "F:\\test")
+  !10 = !DISubroutineType(types: !11)
+  !11 = !{!12, !13, !14}
+  !12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !13 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !12)
+  !14 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !15, size: 64)
+  !15 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !16, size: 64)
+  !16 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !17)
+  !17 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+  !18 = !{!19, !20, !21}
+  !19 = !DILocalVariable(name: "arg", arg: 1, scope: !8, file: !9, line: 1, type: !13)
+  !20 = !DILocalVariable(name: "argv", arg: 2, scope: !8, file: !9, line: 1, type: !14)
+  !21 = !DILocalVariable(name: "result", scope: !8, file: !9, line: 2, type: !12)
+  !22 = !DILocation(line: 0, scope: !8)
+  !23 = !DILocation(line: 2, scope: !8)
+  !24 = !DILocation(line: 3, scope: !8)
+
+...
+---
+name:            main
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$edi', virtual-reg: '' }
+frameInfo:
+  maxAlignment:    1
+body:             |
+  bb.0.entry:
+    liveins: $edi
+
+    renamable $edi = KILL killed $edi, implicit-def $rdi
+    renamable $eax = LEA64_32r killed renamable $rdi, 1, renamable $rdi, 0, $noreg, debug-location !23
+    DBG_VALUE $eax, $noreg, !21, !DIExpression(), debug-location !22
+    ; CHECK-LABEL: DW_TAG_variable
+    ; CHECK: DW_AT_location        (0x00000000:
+    ; CHECK:  [0x0000000000000003,  0x0000000000000004): DW_OP_reg0 RAX)
+    ; CHECK-NEXT: DW_AT_name    ("result")
+    renamable $eax = KILL killed $eax, implicit-def $rax
+    RETQ killed $eax, debug-location !24
+
+...

llvm/test/DebugInfo/X86/pieces-3.ll

 ;    }
 ;
 ; CHECK: DW_TAG_formal_parameter [3]
 ; CHECK-NEXT:   DW_AT_location [DW_FORM_data4]        (
 ; CHECK-NEXT:     [0x0000000000000000, 0x0000000000000007): DW_OP_reg5 RDI, DW_OP_piece 0x8, DW_OP_piece 0x4, DW_OP_reg4 RSI, DW_OP_piece 0x4
 ; CHECK-NEXT:     [0x0000000000000007, 0x0000000000000009): DW_OP_reg5 RDI, DW_OP_piece 0x8
 ; CHECK-NEXT:   DW_AT_name {{.*}}"outer"
 ; CHECK: DW_TAG_variable
-; CHECK-NEXT:   DW_AT_name {{.*}}"i1"
-; CHECK-NOT:    DW_AT_location
+; CHECK-NEXT:   DW_AT_location [DW_FORM_data4]        (0x00000044
+; CHECK-NEXT:     [0x0000000000000007, 0x0000000000000009): DW_OP_reg0 RAX, DW_OP_piece 0x4)
+; CHECK-NEXT:   "i1"
 
 ; ModuleID = '/Volumes/Data/llvm/test/DebugInfo/X86/sroasplit-2.ll'
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 
 ; Function Attrs: nounwind ssp uwtable
 define i32 @foo(i64 %outer.coerce0, i64 %outer.coerce1) #0 !dbg !4 {
   call void @llvm.dbg.value(metadata i64 %outer.coerce0, metadata !24, metadata !25), !dbg !26