[LiveDebugValues] Close range for previous variable's location when deducing new variable's location

[NikolaPrica]

Summary

When LiveDebugValues deduces new variable's location from spill, restore or register copy instruction it should close old variable's location. Otherwise we can have multiple block output locations for same variable. That could lead to inserting two DBG_VALUEs for same variable to the beginning of the successor block which results to ignoring of first DBG_VALUE.

• fission-ranges.ll part which references llvm.dbg.value debuging location is changed in order to give function scope range to variables "b" and "e".

[aprantl]

lib/CodeGen/LiveDebugValues.cpp

436 ↗

(On Diff #200501)

previous variable's -> this variable's previous

Does this do the right thing for two fragments of the same variable where one of them has a longer live range than the other? Or is this a non-issue here?

[NikolaPrica]

lib/CodeGen/LiveDebugValues.cpp

436 ↗

(On Diff #200501)

I think it does the right thing. It does the same thing as transferDebugValue. Variable's locations are related to pair of DILocalVariable and DILocation thus this closes DILocalVariable location with matching DILocation.

[NikolaPrica]

• Update comment
• Changed label for test check-prefix

[jmorse]

This looks good to me (I'll leave it for more comments).

lib/CodeGen/LiveDebugValues.cpp

436 ↗

(On Diff #200501)

NB: as it stands, I don't believe LiveDebugValues handles fragments at all, which I've written up in https://bugs.llvm.org/show_bug.cgi?id=41979

(Thus this patch neither helps nor harms fragment handling).

[aprantl]

lib/CodeGen/LiveDebugValues.cpp

436 ↗

(On Diff #200501)

Huh.

It does the same thing as transferDebugValue.

Looks like this is correct and LiveDebugValues can't propagate more than the last fragment of a variable. Unfortunate, but it does simplify the code. Carry on.

[wolfgangp]

test/DebugInfo/X86/fission-ranges.ll

16 ↗

(On Diff #200682)

I would feel slightly better if we could make sure that these DBG_VALUEs occur in the correct blocks, or at least in different blocks. Perhaps with checks for block labels in between.

[NikolaPrica]

-Make sure DBG_VALUEs from the test case occur in different blocks.

[dstenb]

lib/CodeGen/LiveDebugValues.cpp

712 ↗

(On Diff #200981)

Can this be VLS | OpenRanges.getVarLocs(); now?

test/DebugInfo/X86/fission-ranges.ll

14 ↗

(On Diff #200981)

Perhaps it does not make a difference in this case, but I think it in general is preferred to use CHECK-LABEL directives for block label checks.

[NikolaPrica]

lib/CodeGen/LiveDebugValues.cpp

712 ↗

(On Diff #200981)

Unfortunately that returns SparseBitVector object, we can't deduce from that whether changes were made.

[probinson]

test/DebugInfo/X86/fission-ranges.ll

14 ↗

(On Diff #200981)

The LABEL suffix is widely misunderstood to mean "this is a label."
It would be better to match a longer string such as bb.6: to ensure it matches the label definition rather than a reference.

[dstenb]

test/DebugInfo/X86/fission-ranges.ll

14 ↗

(On Diff #200981)

Right, but in cases like this it seems that label directives can help make FileCheck emit more pinpointed error messages (since the label strings are expected to be unique in the input)? It needs a longer string to match the definition rather than a reference though, as you point out.

# CHECK{,-LABEL}: bb.6:
# CHECK: DBG_VALUE $rsp
# CHECK{,-LABEL}: bb.7:
# CHECK: DBG_VALUE $rsp

bb.6:
bb.7:
DBG_VALUE $rsp

With normal check directives:

a.sh:3:10: error: CHECK: expected string not found in input
# CHECK: bb.7:
         ^
<stdin>:4:1: note: scanning from here

^

With label directives for the basic block labels:

a.sh:2:10: error: CHECK: expected string not found in input
# CHECK: DBG_VALUE $rsp
         ^
<stdin>:2:1: note: scanning from here
bb.7:
^

[NikolaPrica]

• Address comments for test labels.
• Perform conditional copy for new OutLocs set.

[NikolaPrica]

• Rebase

Is this ready to land now?

[aprantl]

I think so.

llvm/lib/CodeGen/LiveDebugValues.cpp

 void LiveDebugValues::insertTransferDebugPair(
     MachineInstr &MI, OpenRangesSet &OpenRanges, TransferMap &Transfers,
     VarLocMap &VarLocIDs, unsigned OldVarID, TransferKind Kind,
     unsigned NewReg) {
   const MachineInstr *DebugInstr = &VarLocIDs[OldVarID].MI;
   MachineFunction *MF = MI.getParent()->getParent();
   MachineInstr *NewDebugInstr;
 
-  auto ProcessVarLoc = [&MI, &OpenRanges, &Transfers,
+  auto ProcessVarLoc = [&MI, &OpenRanges, &Transfers, &DebugInstr,
                         &VarLocIDs](VarLoc &VL, MachineInstr *NewDebugInstr) {
     unsigned LocId = VarLocIDs.insert(VL);
+
+    // Close this variable's previous location range.
+    DebugVariable V(DebugInstr->getDebugVariable(),
+                    DebugInstr->getDebugLoc()->getInlinedAt());
+    OpenRanges.erase(V);
+
     OpenRanges.insert(LocId, VL.Var);
     // The newly created DBG_VALUE instruction NewDebugInstr must be inserted
     // after MI. Keep track of the pairing.
     TransferDebugPair MIP = {&MI, NewDebugInstr};
     Transfers.push_back(MIP);
   };
 
   // End all previous ranges of Var.
bool LiveDebugValues::transferTerminatorInst(MachineInstr &MI,
   LLVM_DEBUG(for (unsigned ID
                   : OpenRanges.getVarLocs()) {
     // Copy OpenRanges to OutLocs, if not already present.
     dbgs() << "Add to OutLocs in MBB #" << CurMBB->getNumber() << ":  ";
     VarLocIDs[ID].dump();
   });
   VarLocSet &VLS = OutLocs[CurMBB];
   Changed = VLS |= OpenRanges.getVarLocs();
+  // New OutLocs set may be different due to spill, restore or register
+  // copy instruction processing.
+  if (Changed)
+    VLS = OpenRanges.getVarLocs();
   OpenRanges.clear();
   return Changed;
 }
 
 /// This routine creates OpenRanges and OutLocs.
 bool LiveDebugValues::process(MachineInstr &MI, OpenRangesSet &OpenRanges,
                               VarLocInMBB &OutLocs, VarLocMap &VarLocIDs,
                               TransferMap &Transfers, bool transferChanges) {

llvm/test/DebugInfo/X86/fission-ranges.ll

 ; RUN: llc -split-dwarf-file=foo.dwo -O0 %s -mtriple=x86_64-unknown-linux-gnu -filetype=obj -o %t
 ; RUN: llvm-dwarfdump -v %t | FileCheck %s
 ; RUN: llvm-objdump -h %t | FileCheck --check-prefix=HDR %s
 
 ; RUN: llc -dwarf-version=5 -O0 %s -mtriple=x86_64-unknown-linux-gnu -filetype=obj -o %t
 ; RUN: llvm-dwarfdump -v %t | FileCheck --check-prefix=V5RNGLISTS %s
 
+; RUN: llc -O0 %s -mtriple=x86_64-unknown-linux-gnu -stop-after=livedebugvalues -o -| FileCheck --check-prefix=CHECK-MIR %s
+
+; LiveDebugValues should produce DBG_VALUEs for variable "b" in successive
+; blocks once we recognize that it is spilled.
+; CHECK-MIR: ![[BDIVAR:[0-9]+]] = !DILocalVariable(name: "b"
+; CHECK-MIR: DBG_VALUE $rsp, 0, ![[BDIVAR]], !DIExpression(DW_OP_constu, 32, DW_OP_minus)
+; CHECK-MIR-LABEL: bb.6.for.inc13:
+; CHECK-MIR: DBG_VALUE $rsp, 0, ![[BDIVAR]], !DIExpression(DW_OP_constu, 32, DW_OP_minus)
+; CHECK-MIR-LABEL: bb.7.for.inc16:
+; CHECK-MIR: DBG_VALUE $rsp, 0, ![[BDIVAR]], !DIExpression(DW_OP_constu, 32, DW_OP_minus)
+
+
 ; CHECK: .debug_info contents:
 ; CHECK: DW_TAG_compile_unit
 ; CHECK-NEXT: DW_AT_stmt_list
 ; CHECK-NEXT: DW_AT_comp_dir
 ; CHECK-NEXT: DW_AT_GNU_dwo_name
 ; CHECK-NEXT: DW_AT_GNU_dwo_id
 ; CHECK-NEXT: DW_AT_GNU_ranges_base
 ; CHECK-NEXT: DW_AT_low_pc

 ; CHECK-NOT: Beginning address offset
 ; CHECK: .debug_loc.dwo contents:
 
 ; Don't assume these locations are entirely correct - feel free to update them
 ; if they've changed due to a bugfix, change in register allocation, etc.
 
 ; CHECK:      [[A]]:
 ; CHECK-NEXT:   Addr idx 2 (w/ length 169): DW_OP_consts +0, DW_OP_stack_value
-; CHECK-NEXT:   Addr idx 3 (w/ length 25): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 3 (w/ length 15): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 4 (w/ length 18): DW_OP_breg7 RSP-8
 ; CHECK:      [[E]]:
-; CHECK-NEXT:   Addr idx 4 (w/ length 19): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 5 (w/ length 23): DW_OP_reg0 RAX
 ; CHECK:      [[B]]:
-; CHECK-NEXT:   Addr idx 5 (w/ length 17): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 6 (w/ length 15): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 7 (w/ length 66): DW_OP_breg7 RSP-32
 ; CHECK:      [[D]]:
-; CHECK-NEXT:   Addr idx 6 (w/ length 17): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 8 (w/ length 15): DW_OP_reg0 RAX
+; CHECK-NEXT:   Addr idx 9 (w/ length 42): DW_OP_breg7 RSP-20
 
 ; Make sure we don't produce any relocations in any .dwo section (though in particular, debug_info.dwo)
 ; HDR-NOT: .rela.{{.*}}.dwo
 
 ; Make sure we have enough stuff in the debug_addr to cover the address indexes
-; (6 is the last index in debug_loc.dwo, making 7 entries of 8 bytes each, 7 * 8
-; == 56 base 10 == 38 base 16)
+; (9 is the last index in debug_loc.dwo, making 10 entries of 8 bytes each,
+; 10 * 8 == 80 base 10 == 50 base 16)
 
-; HDR: .debug_addr 00000038
+; HDR: .debug_addr 00000050
 ; HDR-NOT: .rela.{{.*}}.dwo
 
 ; Check for the existence of a DWARF v5-style range list table in the .debug_rnglists
 ; section and that the compile unit has a DW_AT_rnglists_base attribute.
 ; The table should contain at least one rangelist with at least 2 individual ranges.
 
 ; V5RNGLISTS:      .debug_info contents:
 ; V5RNGLISTS:      DW_TAG_compile_unit
for.cond7.preheader:                              ; preds = %for.inc10, %for.cond4.preheader
   br label %for.body9, !dbg !39
 
 for.body9:                                        ; preds = %for.body9, %for.cond7.preheader
   %and2 = phi i32 [ %and.lcssa5, %for.cond7.preheader ], [ %and, %for.body9 ], !dbg !40
   %e.01 = phi i32 [ 0, %for.cond7.preheader ], [ %inc, %for.body9 ]
   tail call void @llvm.dbg.value(metadata i32* @c, metadata !19, metadata !DIExpression()), !dbg !40
   %and = and i32 %and2, 1, !dbg !32
   %inc = add i32 %e.01, 1, !dbg !39
-  tail call void @llvm.dbg.value(metadata i32 %inc, metadata !18, metadata !DIExpression()), !dbg !39
+  tail call void @llvm.dbg.value(metadata i32 %inc, metadata !18, metadata !DIExpression()), !dbg !42
   %exitcond = icmp eq i32 %inc, 30, !dbg !39
   br i1 %exitcond, label %for.inc10, label %for.body9, !dbg !39
 
 for.inc10:                                        ; preds = %for.body9
   %inc11 = add nsw i32 %b.03, 1, !dbg !38
-  tail call void @llvm.dbg.value(metadata i32 %inc11, metadata !15, metadata !DIExpression()), !dbg !38
+  tail call void @llvm.dbg.value(metadata i32 %inc11, metadata !15, metadata !DIExpression()), !dbg !42
   %exitcond11 = icmp eq i32 %inc11, 30, !dbg !38
   br i1 %exitcond11, label %for.inc13, label %for.cond7.preheader, !dbg !38
 
 for.inc13:                                        ; preds = %for.inc10
   %inc14 = add i32 %d.06, 1, !dbg !37
   tail call void @llvm.dbg.value(metadata i32 %inc14, metadata !16, metadata !DIExpression()), !dbg !37
   %exitcond12 = icmp eq i32 %inc14, 30, !dbg !37
   br i1 %exitcond12, label %for.inc16, label %for.cond4.preheader, !dbg !37