[KeyInstr][DwarfDebug] Add is_stmt emission support #133495
Conversation
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp llvm/lib/CodeGen/AsmPrinter/DwarfDebug.hView the diff from clang-format here.diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index d8f76a0e2..a83d3434a 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2097,7 +2097,8 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
(!PrevInstBB ||
PrevInstBB->getSectionID() == MI->getParent()->getSectionID());
bool ForceIsStmt = ForceIsStmtInstrs.contains(MI);
- if (PrevInstInSameSection && !ForceIsStmt && DL.isSameSourceLocation(PrevInstLoc)) {
+ if (PrevInstInSameSection && !ForceIsStmt &&
+ DL.isSameSourceLocation(PrevInstLoc)) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
return;
|
|
@llvm/pr-subscribers-debuginfo Author: Orlando Cazalet-Hyams (OCHyams) ChangesInterpret Key Instructions metadata to determine is_stmt placement. The lowest rank (highest precedent) instructions in each {InlinedAt, atomGroup} Patch is 35.53 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133495.diff 8 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 39f1299a24e81..71abef1d2383b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -17,6 +17,7 @@
#include "DwarfExpression.h"
#include "DwarfUnit.h"
#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/Twine.h"
@@ -170,6 +171,9 @@ static cl::opt<DwarfDebug::MinimizeAddrInV5> MinimizeAddrInV5Option(
"Stuff")),
cl::init(DwarfDebug::MinimizeAddrInV5::Default));
+static cl::opt<bool> KeyInstructionsAreStmts("dwarf-use-key-instructions",
+ cl::Hidden, cl::init(false));
+
static constexpr unsigned ULEB128PadSize = 4;
void DebugLocDwarfExpression::emitOp(uint8_t Op, const char *Comment) {
@@ -2069,6 +2073,10 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
unsigned LastAsmLine =
Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
+ bool IsKey = false;
+ if (KeyInstructionsAreStmts && DL && DL.getLine())
+ IsKey = KeyInstructions.contains(MI);
+
if (!DL && MI == PrologEndLoc) {
// In rare situations, we might want to place the end of the prologue
// somewhere that doesn't have a source location already. It should be in
@@ -2087,13 +2095,18 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
return;
- // We have an explicit location, same as the previous location.
- // But we might be coming back to it after a line 0 record.
- if ((LastAsmLine == 0 && DL.getLine() != 0) || Flags) {
- // Reinstate the source location but not marked as a statement.
- RecordSourceLine(DL, Flags);
+
+ // Skip this if the instruction is Key, else we might accidentally miss an
+ // is_stmt.
+ if (!IsKey) {
+ // We have an explicit location, same as the previous location.
+ // But we might be coming back to it after a line 0 record.
+ if ((LastAsmLine == 0 && DL.getLine() != 0) || Flags) {
+ // Reinstate the source location but not marked as a statement.
+ RecordSourceLine(DL, Flags);
+ }
+ return;
}
- return;
}
if (!DL) {
@@ -2136,11 +2149,17 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
Flags |= DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT;
PrologEndLoc = nullptr;
}
- // If the line changed, we call that a new statement; unless we went to
- // line 0 and came back, in which case it is not a new statement.
- unsigned OldLine = PrevInstLoc ? PrevInstLoc.getLine() : LastAsmLine;
- if (DL.getLine() && (DL.getLine() != OldLine || ForceIsStmt))
- Flags |= DWARF2_FLAG_IS_STMT;
+
+ if (KeyInstructionsAreStmts) {
+ if (IsKey)
+ Flags |= DWARF2_FLAG_IS_STMT;
+ } else {
+ // If the line changed, we call that a new statement; unless we went to
+ // line 0 and came back, in which case it is not a new statement.
+ unsigned OldLine = PrevInstLoc ? PrevInstLoc.getLine() : LastAsmLine;
+ if (DL.getLine() && (DL.getLine() != OldLine || ForceIsStmt))
+ Flags |= DWARF2_FLAG_IS_STMT;
+ }
RecordSourceLine(DL, Flags);
@@ -2333,6 +2352,170 @@ DwarfDebug::emitInitialLocDirective(const MachineFunction &MF, unsigned CUID) {
return PrologEndLoc;
}
+void DwarfDebug::findKeyInstructions(const MachineFunction *MF) {
+ // New function - reset KeyInstructions.
+ KeyInstructions.clear();
+
+ // The current candidate is_stmt instructions for each source atom.
+ // Map {(InlinedAt, Group): (Rank, Instructions)}.
+ DenseMap<std::pair<DILocation *, uint32_t>,
+ std::pair<uint16_t, SmallVector<const MachineInstr *>>>
+ GroupCandidates;
+
+ // For each instruction:
+ // * Skip insts without DebugLoc, AtomGroup or AtomRank, and line zeros.
+ // * Check if insts in this group have been seen already in GroupCandidates.
+ // * If this instr rank is equal, add this instruction to KeyInstructions.
+ // Remove existing instructions from KeyInstructions if they have the
+ // same parent.
+ // * If this instr rank is higher (lower precedence), ignore it.
+ // * If this instr rank is lower (higher precedence), erase existing
+ // instructions from KeyInstructions. Add this instr to KeyInstructions.
+
+ for (auto &MBB : *MF) {
+ // Rather than apply is_stmt directly to Key Instructions, we "float"
+ // is_stmt up to the 1st instruction with the same line number in a
+ // contiguous block. That instruction is called the "buoy". The
+ // buoy gets reset if we encouner an instruction with an atom
+ // group.
+ const MachineInstr *Buoy = nullptr;
+ // The atom group number associated with Buoy which may be 0 if we haven't
+ // encountered an atom group yet in this blob of instructions with the same
+ // line number.
+ uint64_t BuoyAtom = 0;
+
+ for (auto &MI : MBB) {
+ if (MI.isMetaInstruction())
+ continue;
+
+ if (!MI.getDebugLoc() || !MI.getDebugLoc().getLine())
+ continue;
+
+ // Reset the Buoy to this instruciton if it has a different line number.
+ if (!Buoy ||
+ Buoy->getDebugLoc().getLine() != MI.getDebugLoc().getLine()) {
+ Buoy = &MI;
+ BuoyAtom = 0;
+ }
+
+ // Call instructions are handled specially - we always mark them as key
+ // regardless of atom info.
+ const auto &TII =
+ *MI.getParent()->getParent()->getSubtarget().getInstrInfo();
+ if (MI.isCall() || TII.isTailCall(MI)) {
+ assert(MI.getDebugLoc() && "Unexpectedly missing DL");
+
+ // Calls are always key.
+ KeyInstructions.insert(Buoy);
+
+ uint64_t Group = MI.getDebugLoc()->getAtomGroup();
+ uint8_t Rank = MI.getDebugLoc()->getAtomRank();
+ if (Group && Rank) {
+ auto *InlinedAt = MI.getDebugLoc()->getInlinedAt();
+ auto &[CandidateRank, CandidateInsts] = GroupCandidates[{InlinedAt, Group}];
+
+ // This looks similar to the non-call handling code, except that
+ // we don't put the call into CandidateInsts so that they can't be
+ // made un-key. As a result, we also have to take special care not
+ // to erase the is_stmt from the buoy, and prevent that happening
+ // in the future.
+
+ if (CandidateRank == Rank) {
+ // We've seen other instructions in this group of this rank. Discard
+ // ones we've seen in this block, keep the others.
+ assert(!CandidateInsts.empty());
+ SmallVector<const MachineInstr *> Insts;
+ Insts.reserve(CandidateInsts.size());
+ for (auto &PrevInst : CandidateInsts) {
+ if (PrevInst->getParent() != MI.getParent())
+ Insts.push_back(PrevInst);
+ else if (PrevInst != Buoy)
+ KeyInstructions.erase(PrevInst);
+ }
+
+ if (Insts.empty()) {
+ CandidateInsts.clear();
+ CandidateRank = 0;
+ } else {
+ CandidateInsts = std::move(Insts);
+ }
+
+ } else if (CandidateRank > Rank) {
+ // We've seen other instructions in this group of lower precedence
+ // (higher rank). Discard them.
+ for (auto *Supplanted : CandidateInsts) {
+ // Don't erase the is_stmt we're using for this call.
+ if (Supplanted != Buoy)
+ KeyInstructions.erase(Supplanted);
+ }
+ CandidateInsts.clear();
+ CandidateRank = 0;
+ }
+ }
+
+ // Avoid floating any future is_stmts up to the call.
+ Buoy = nullptr;
+ continue;
+ }
+
+ auto *InlinedAt = MI.getDebugLoc()->getInlinedAt();
+ uint64_t Group = MI.getDebugLoc()->getAtomGroup();
+ uint8_t Rank = MI.getDebugLoc()->getAtomRank();
+ if (!Group || !Rank)
+ continue;
+
+ // Don't let is_stmts float past instructions from different source atoms.
+ if (BuoyAtom && BuoyAtom != Group) {
+ Buoy = &MI;
+ BuoyAtom = MI.getDebugLoc()->getAtomGroup();
+ }
+
+ auto &[CandidateRank, CandidateInsts] = GroupCandidates[{InlinedAt, Group}];
+
+ if (CandidateRank == 0) {
+ // This is the first time we're seeing an instruction in this atom
+ // group. Add it to the map.
+ assert(CandidateInsts.empty());
+ CandidateRank = Rank;
+ CandidateInsts.push_back(Buoy);
+
+ } else if (CandidateRank == Rank) {
+ // We've seen other instructions in this group of this rank. Discard
+ // ones we've seen in this block, keep the others, add this one.
+ assert(!CandidateInsts.empty());
+ SmallVector<const MachineInstr *> Insts;
+ Insts.reserve(CandidateInsts.size() + 1);
+ for (auto &PrevInst : CandidateInsts) {
+ if (PrevInst->getParent() != MI.getParent())
+ Insts.push_back(PrevInst);
+ else
+ KeyInstructions.erase(PrevInst);
+ }
+ Insts.push_back(Buoy);
+ CandidateInsts = std::move(Insts);
+
+ } else if (CandidateRank > Rank) {
+ // We've seen other instructions in this group of lower precedence
+ // (higher rank). Discard them, add this one.
+ assert(!CandidateInsts.empty());
+ CandidateRank = Rank;
+ for (auto *Supplanted : CandidateInsts)
+ KeyInstructions.erase(Supplanted);
+ CandidateInsts = {Buoy};
+
+ } else {
+ // We've seen other instructions in this group with higher precedence
+ // (lower rank). Discard this one.
+ assert(Rank != 0 && CandidateRank < Rank && CandidateRank != 0);
+ continue;
+ }
+ KeyInstructions.insert(Buoy);
+ assert(!BuoyAtom || BuoyAtom == MI.getDebugLoc()->getAtomGroup());
+ BuoyAtom = MI.getDebugLoc()->getAtomGroup();
+ }
+ }
+}
+
/// For the function \p MF, finds the set of instructions which may represent a
/// change in line number from one or more of the preceding MBBs. Stores the
/// resulting set of instructions, which should have is_stmt set, in
@@ -2491,7 +2674,10 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
PrologEndLoc = emitInitialLocDirective(
*MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
- findForceIsStmtInstrs(MF);
+ if (KeyInstructionsAreStmts)
+ findKeyInstructions(MF);
+ else
+ findForceIsStmtInstrs(MF);
}
unsigned
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index 58e6d39f76ae0..cd232a8e94ecf 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -464,6 +464,10 @@ class DwarfDebug : public DebugHandlerBase {
};
private:
+ /// Instructions which should get is_stmt applied because they implement key
+ /// functionality for a source atom.
+ SmallDenseSet<const MachineInstr *> KeyInstructions;
+
/// Force the use of DW_AT_ranges even for single-entry range lists.
MinimizeAddrInV5 MinimizeAddr = MinimizeAddrInV5::Disabled;
@@ -701,6 +705,11 @@ class DwarfDebug : public DebugHandlerBase {
void findForceIsStmtInstrs(const MachineFunction *MF);
+ /// Find instructions which should get is_stmt applied because they implement
+ /// key functionality for a source atom, store results in
+ /// DwarfDebug::KeyInstructions.
+ void findKeyInstructions(const MachineFunction *MF);
+
protected:
/// Gather pre-function debug information.
void beginFunctionImpl(const MachineFunction *MF) override;
diff --git a/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic-ranks.ll b/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic-ranks.ll
new file mode 100644
index 0000000000000..71ecf1dc41238
--- /dev/null
+++ b/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic-ranks.ll
@@ -0,0 +1,68 @@
+; RUN: llc %s --filetype=obj -o - --dwarf-use-key-instructions \
+; RUN: | llvm-objdump -d - --no-show-raw-insn \
+; RUN: | FileCheck %s --check-prefix=OBJ
+
+; RUN: llc %s --filetype=obj -o - --dwarf-use-key-instructions \
+; RUN: | llvm-dwarfdump - --debug-line \
+; RUN: | FileCheck %s --check-prefix=DBG
+
+; OBJ: 0000000000000000 <_Z1fPiii>:
+; OBJ-NEXT: 0: pushq %rbp
+; OBJ-NEXT: 1: pushq %r14
+; OBJ-NEXT: 3: pushq %rbx
+; OBJ-NEXT: 4: movl %edx, %ebx
+; OBJ-NEXT: 6: movl %esi, %ebp
+; OBJ-NEXT: 8: movq %rdi, %r14
+; OBJ-NEXT: b: callq 0x10 <_Z1fPiii+0x10>
+; OBJ-NEXT: 10: addl %ebx, %ebp
+; OBJ-NEXT: 12: movl %ebp, (%r14)
+; OBJ-NEXT: 15: movl %ebp, %eax
+; OBJ-NEXT: 17: popq %rbx
+; OBJ-NEXT: 18: popq %r14
+; OBJ-NEXT: 1a: popq %rbp
+
+; DBG: Address Line Column File ISA Discriminator OpIndex Flags
+; DBG-NEXT: ------------------ ------ ------ ------ --- ------------- ------- -------------
+; DBG-NEXT: 0x0000000000000000 3 0 0 0 0 0 is_stmt
+; DBG-NEXT: 0x000000000000000b 4 0 0 0 0 0 is_stmt prologue_end
+; DBG-NEXT: 0x0000000000000010 6 0 0 0 0 0
+; DBG-NEXT: 0x0000000000000012 5 0 0 0 0 0 is_stmt
+; DBG-NEXT: 0x0000000000000015 7 0 0 0 0 0 is_stmt
+; DBG-NEXT: 0x0000000000000017 7 0 0 0 0 0 epilogue_begin
+; DBG-NEXT: 0x000000000000001c 7 0 0 0 0 0 end_sequence
+
+;; 1. [[gnu::nodebug]] void prologue_end();
+;; 2.
+;; 3. int f(int *a, int b, int c) {
+;; 4. prologue_end();
+;; 5. *a =
+;; 6. b + c;
+;; 7. return *a;
+;; 8. }
+
+;; The add and store are in the same goup (1). The add (line 6) has lower
+;; precedence (rank 2) so should not get is_stmt applied.
+target triple = "x86_64-unknown-linux-gnu"
+
+define hidden noundef i32 @_Z1fPiii(ptr %a, i32 %b, i32 %c) local_unnamed_addr !dbg !11 {
+entry:
+ tail call void @_Z12prologue_endv(), !dbg !DILocation(line: 4, scope: !11)
+ %add = add nsw i32 %c, %b, !dbg !DILocation(line: 6, scope: !11, atomGroup: 1, atomRank: 2)
+ store i32 %add, ptr %a, align 4, !dbg !DILocation(line: 5, scope: !11, atomGroup: 1, atomRank: 1)
+ ret i32 %add, !dbg !DILocation(line: 7, scope: !11, atomGroup: 2, atomRank: 1)
+}
+
+declare void @_Z12prologue_endv() local_unnamed_addr #1
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+!llvm.ident = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_17, file: !1, producer: "clang version 19.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = !{!"clang version 19.0.0"}
+!11 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 3, type: !12, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!12 = !DISubroutineType(types: !13)
+!13 = !{}
diff --git a/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic.ll b/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic.ll
new file mode 100644
index 0000000000000..e3b0184a837f8
--- /dev/null
+++ b/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic.ll
@@ -0,0 +1,62 @@
+; RUN: llc %s --filetype=obj -o - --dwarf-use-key-instructions \
+; RUN: | llvm-objdump -d - --no-show-raw-insn \
+; RUN: | FileCheck %s --check-prefix=OBJ
+
+; RUN: llc %s --filetype=obj -o - --dwarf-use-key-instructions \
+; RUN: | llvm-dwarfdump - --debug-line \
+; RUN: | FileCheck %s --check-prefix=DBG
+
+; OBJ: 0000000000000000 <_Z1fi>:
+; OBJ-NEXT: 0: leal 0x1(%rdi), %eax
+; OBJ-NEXT: 3: retq
+; OBJ: 0000000000000010 <_Z1gi>:
+; OBJ-NEXT: 10: leal 0x1(%rdi), %eax
+; OBJ-NEXT: 13: retq
+
+; DBG: Address Line Column File ISA Discriminator OpIndex Flags
+; DBG-NEXT: ------------------ ------ ------ ------ --- ------------- ------- -------------
+; DBG-NEXT: 0x0000000000000000 2 0 0 0 0 0 is_stmt prologue_end
+; DBG-NEXT: 0x0000000000000003 3 0 0 0 0 0 is_stmt
+; DBG-NEXT: 0x0000000000000010 2 0 0 0 0 0 is_stmt prologue_end
+; DBG-NEXT: 0x0000000000000013 6 0 0 0 0 0 is_stmt
+
+;; 1. int f(int a) {
+;; 2. int x = a + 1;
+;; 3. return x;
+;; 4. }
+;; 5. int g(int b) {
+;; 6. return f(b);
+;; 7. }
+;;
+;; Both functions contain 2 instructions in unique atom groups. In f we see
+;; groups 1 and 3, and in g we see {!18, 1} and 1. All of these instructions
+;; should get is_stmt.
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define hidden noundef i32 @_Z1fi(i32 noundef %a) local_unnamed_addr !dbg !11 {
+entry:
+ %add = add nsw i32 %a, 1, !dbg !DILocation(line: 2, scope: !11, atomGroup: 1, atomRank: 2)
+ ret i32 %add, !dbg !DILocation(line: 3, scope: !11, atomGroup: 3, atomRank: 1)
+}
+
+define hidden noundef i32 @_Z1gi(i32 noundef %b) local_unnamed_addr !dbg !16 {
+entry:
+ %add.i = add nsw i32 %b, 1, !dbg !DILocation(line: 2, scope: !11, inlinedAt: !18, atomGroup: 1, atomRank: 2)
+ ret i32 %add.i, !dbg !DILocation(line: 6, scope: !16, atomGroup: 1, atomRank: 1)
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+!llvm.ident = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_17, file: !1, producer: "clang version 19.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = !{!"clang version 19.0.0"}
+!11 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 1, type: !12, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!12 = !DISubroutineType(types: !13)
+!13 = !{}
+!16 = distinct !DISubprogram(name: "g", scope: !1, file: !1, line: 5, type: !12, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!18 = distinct !DILocation(line: 6, scope: !16)
diff --git a/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-buoy-multi-key.mir b/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-buoy-multi-key.mir
new file mode 100644
index 0000000000000..c8459b4ced600
--- /dev/null
+++ b/llvm/test/DebugInfo/KeyInstructions/X86/dwarf-buoy-multi-key.mir
@@ -0,0 +1,78 @@
+# RUN: llc %s --start-after=livedebugvalues --dwarf-use-key-instructions --filetype=obj -o - \
+# RUN: | llvm-objdump -d - --no-show-raw-insn \
+# RUN: | FileCheck %s --check-prefix=OBJ
+
+# RUN: llc %s --start-after=livedebugvalues --dwarf-use-key-instructions --filetype=obj -o - \
+# RUN: | llvm-dwarfdump - --debug-line \
+# RUN: | FileCheck %s --check-prefix=DBG
+
+# OBJ: 0000000000000000 <_Z1fPiii>:
+# OBJ-NEXT: 0: movl $0x0, %ebx
+# OBJ-NEXT: 5: movl $0x1, %ebx
+# OBJ-NEXT: a: movl $0x2, %ebx
+# OBJ-NEXT: f: movl $0x3, %ebx
+# OBJ-NEXT: 14: movl $0x4, %eax
+# OBJ-NEXT: 19: movl $0x5, %eax
+# OBJ-NEXT: 1e: movl $0x6, %eax
+# OBJ-NEXT: 23: movl $0x7, %eax
+# OBJ-NEXT: 28: retq
+
+# DBG: Address Line Column File ISA Discriminator OpIndex Flags
+# DBG-NEXT: ------------------ ------ ------ ------ --- ------------- ------- -------------
+# DBG-NEXT: 0x0000000000000000 1 0 0 0 0 0 is_stmt prologue_end
+# DBG-NEXT: 0x0000000000000005 2 0 0 0 0 0 is_stmt
+# DBG-NEXT: 0x000000000000000a 2 0 0 0 0 0
+# DBG-NEXT: 0x000000000000000f 2 0 0 0 0 0
+# DBG-NEXT: 0x0000000000000014 2 0 0 0 0 0
+# DBG-NEXT: 0x0000000000000019 2 0 0 0 0 0
+# DBG-NEXT: 0x000000000000001e 2 0 0 0 ...
[truncated]
|
There was a problem hiding this comment.
I get the impression that GroupCandidates and KeyInstructions are being kept strictly in sync; thus couldn't one instead just load KeyInstructions from GroupCandidates once the scan is complete? This avoids filling up the dense map with tombstones.
Am I right in understanding that the buoy means the "least precedence" instruction will get the is_stmt if the highest precedence appears after it in the contiguous blob? (Seems fine, just making sure I understand).
On the whole, the computation function feels like it could be simpler, but in some intangible way I'm not immediately sure of. (Still reading the tests).
| /// Find instructions which should get is_stmt applied because they implement | ||
| /// key functionality for a source atom, store results in | ||
| /// DwarfDebug::KeyInstructions. |
There was a problem hiding this comment.
Wording nit; I'd prefer "Compute instructions which should..." because it's not a pure matter of searching, it's a substantive exploration of... stuff? I guess there's a lattice involved if we think about it hard enough.
| // Skip this if the instruction is Key, else we might accidentally miss an | ||
| // is_stmt. | ||
| if (!IsKey) { |
There was a problem hiding this comment.
Can we not fold this test, and the comment, into the existing test? "If we have an explicit non-key location....". (Avoids some un-necessary indentation).
There was a problem hiding this comment.
That puts the return in an awkward position - we also want to skip that if IsKey is true.
| if (!MI.getDebugLoc() || !MI.getDebugLoc().getLine()) | ||
| continue; | ||
|
|
||
| // Reset the Buoy to this instruciton if it has a different line number. |
There was a problem hiding this comment.
| // Reset the Buoy to this instruciton if it has a different line number. | |
| // Reset the Buoy to this instruction if it has a different line number. |
| // We've seen other instructions in this group of this rank. Discard | ||
| // ones we've seen in this block, keep the others, add this one. | ||
| assert(!CandidateInsts.empty()); | ||
| SmallVector<const MachineInstr *> Insts; | ||
| Insts.reserve(CandidateInsts.size() + 1); | ||
| for (auto &PrevInst : CandidateInsts) { | ||
| if (PrevInst->getParent() != MI.getParent()) | ||
| Insts.push_back(PrevInst); | ||
| else | ||
| KeyInstructions.erase(PrevInst); | ||
| } | ||
| Insts.push_back(Buoy); | ||
| CandidateInsts = std::move(Insts); |
There was a problem hiding this comment.
An alternative perhaps: only permit zero or one MachineInstrs from a block in CandidateInsts, checked by an EXPENSIVE_CHECKS check. Then we either:
- If there are zero instructions present from the same block, append the new one,
- If there's one instruction present from the same block, overwrite it,
Hopefully avoiding there ever being more than one instruction from the same block? Unless they can enter via some other route.
There was a problem hiding this comment.
Also: this adds the buoy each time, so presumably if you have three identically-sourced instructions in a row, you get three buoys in the candidate list?
There was a problem hiding this comment.
Does the first comment / question still apply to this new version of the code?
Also: this adds the buoy each time, so presumably if you have three identically-sourced instructions in a row, you get three buoys in the candidate list?
If they've got the same atom number, no, there's just one. If they're identical except the atom number yes there will be multiple. That's by design, because IMO we should convey that there's multiple key instructions at the source position and let the debugger do what it wants with that info. (Happy to alter the design if needed, just saying this is working as intended)
| // The current candidate is_stmt instructions for each source atom. | ||
| // Map {(InlinedAt, Group): (Rank, Instructions)}. | ||
| DenseMap<std::pair<DILocation *, uint32_t>, | ||
| std::pair<uint16_t, SmallVector<const MachineInstr *>>> | ||
| GroupCandidates; |
There was a problem hiding this comment.
This will be big due to each dense-map element containing a SmallVector of pointers; IMO we need to do some profiling and set a SmallVector allocation size.
There was a problem hiding this comment.
Oh, good point.
Building X86ISelLowering.cpp with -O2 -g gives these numbers for .size() (at the end of the function, hence some empties):
Mean: 1.0316807795462797
Standard Deviation: 0.5124221522830779
Size, Count
0, 3798
1, 77059
2, 3656
3, 565
4, 116
5, 43
6, 17
7, 14
8, 6
9, 78
10, 14
11, 5
12, 2
13, 3
14, 2
15, 1
18, 1
21, 1
24, 1
Looking at capacity to account for growing then clearing:
Small size 1:
Mean: 1.1314547392338052
Standard Deviation: 0.7145833142818651
Capacity, Count
1, 80836
3, 4241
7, 163
13, 135
14, 2
15, 1
16, 1
18, 1
22, 1
24, 1
Small size 2:
Mean: 2.044950399962522
Standard Deviation: 0.5464689255410543
Capacity, Count
2, 84507
5, 730
11, 3
13, 135
14, 2
15, 1
16, 1
18, 1
22, 1
24, 1
I'll see if compile-time-tracker can help us pick 1 or 2.
There was a problem hiding this comment.
Seems much of a muchness. http://llvm-compile-time-tracker.com/compare.php?from=864910b2187803587013f0c6a8ed57361670dc08&to=4ca6a9fa05d51d62b27a48125f97d92812f0972d&stat=instructions%3Au#. The clang build indicates a preference for small-size of 2 rather than 1 when looking at max rss, but I'm not sure I trust that number as I'm pretty sure the clang self host build is release + thinLTO (no -g).
There was a problem hiding this comment.
Welp, one or two is better than the default SmallVector calculation of "several"
| assert(MI.getDebugLoc() && "Unexpectedly missing DL"); | ||
|
|
||
| // Calls are always key. | ||
| KeyInstructions.insert(Buoy); |
There was a problem hiding this comment.
| KeyInstructions.insert(Buoy); | |
| KeyInstructions.insert(&MI); |
Avoids any doubt about what's being inserted.
There was a problem hiding this comment.
Updated the comment to highlight that sometimes MI != Buoy.
|
|
||
| uint64_t Group = MI.getDebugLoc()->getAtomGroup(); | ||
| uint8_t Rank = MI.getDebugLoc()->getAtomRank(); | ||
| if (Group && Rank) { |
There was a problem hiding this comment.
Tests: I'd personally prefer the input source and explanation at the top of the file, although this is a style thing.
My understanding of this code is that within a basic block, it should be possible for there to be two sequences of instructions of equal group and rank that both get a buoy and is_stmt, if they're separated by some other atom group. Perhaps I'm wrong; but if I'm right, it probably wants explicit test coverage.
There's a risk that the code being generated is brittle and frequently needs updating; I don't know what probability of that we find acceptable, let's just see how often it needs updating I guess.
| # DBG-NEXT: 0x000000000000000a 2 0 0 0 0 0 | ||
| # DBG-NEXT: 0x000000000000000f 2 0 0 0 0 0 | ||
| # DBG-NEXT: 0x0000000000000014 2 0 0 0 0 0 | ||
| # DBG-NEXT: 0x0000000000000019 2 0 0 0 0 0 |
There was a problem hiding this comment.
Shouldn't these line-entries all fold together into one, or is there some data keeping them distinct? AFAUI there's no need for multiple records if we can just describe a single range.
There was a problem hiding this comment.
Good spot. Fixed in commit "fix line entries not folding together properly". That also fixes a bug where a line should have been is_stmt but wasn't (accidentally floated is_stmt past a different atom, which was unintended)
| ;; Check the 2nd call (line 4) gets is_stmt applied despite being part of group | ||
| ;; 1 and having lower precedence than the add. Check that the add stil gets | ||
| ;; is_stmt applied. | ||
| ;; There are two is_stmt line 4 entries are is_stmt because we don't float |
There was a problem hiding this comment.
| ;; There are two is_stmt line 4 entries are is_stmt because we don't float | |
| ;; There are two is_stmt line 4 entries because we don't float |
| $ebx = MOV32ri 2, debug-location !DILocation(line: 2, scope: !5, atomGroup: 1, atomRank: 2) | ||
| $ebx = MOV32ri 3, debug-location !DILocation(line: 2, scope: !5, atomGroup: 1, atomRank: 1) | ||
| $eax = MOV32ri 4, debug-location !DILocation(line: 2, scope: !5) | ||
| $eax = MOV32ri 5, debug-location !DILocation(line: 2, scope: !5, atomGroup: 2, atomRank: 1) |
There was a problem hiding this comment.
My understanding is that this instruction should get an is_stmt in its own entry because it's a separate span of atom group 2, is that right? But doesn't (address 0x19 above?) as far as I can tell.
There was a problem hiding this comment.
Fixed this by addressing previous comment, good spot.
OCHyams
force-pushed
the
users/OCHyams/ki-llvm-mir-parser
branch
from
b49eb51 to
d2ba768
Compare
Interpret Key Instructions metadata to determine is_stmt placement.
The lowest rank (highest precedent) instructions in each {InlinedAt, atomGroup}
set are candidates for is_stmt. Only the last instruction in each set in a given
block gets is_stmt. Calls always get is_stmt.
OCHyams
force-pushed
the
users/OCHyams/ki-llvm-dwarf-emission
branch
from
0b206ae to
0b5166a
Compare
There was a problem hiding this comment.
I get the impression that GroupCandidates and KeyInstructions are being kept strictly in sync; thus couldn't one instead just load KeyInstructions from GroupCandidates once the scan is complete? This avoids filling up the dense map with tombstones.
Good observation. That has enabled some code simplification too.
Am I right in understanding that the buoy means the "least precedence" instruction will get the is_stmt if the highest precedence appears after it in the contiguous blob? (Seems fine, just making sure I understand).
Yep that's right.
On the whole, the computation function feels like it could be simpler, but in some intangible way I'm not immediately sure of. (Still reading the tests).
This part of the code changed and grew a few times during prototyping; despite efforts to keep it away from the clean/final version some of that cruft evidentially made it into this patch. Looking at it with fresh eyes I've simplified it a bunch (I've split the clean up into few commits so it's easy to track how things moved about).
| // Skip this if the instruction is Key, else we might accidentally miss an | ||
| // is_stmt. | ||
| if (!IsKey) { |
There was a problem hiding this comment.
That puts the return in an awkward position - we also want to skip that if IsKey is true.
| if (!MI.getDebugLoc() || !MI.getDebugLoc().getLine()) | ||
| continue; | ||
|
|
||
| // Reset the Buoy to this instruciton if it has a different line number. |
|
|
||
| uint64_t Group = MI.getDebugLoc()->getAtomGroup(); | ||
| uint8_t Rank = MI.getDebugLoc()->getAtomRank(); | ||
| if (Group && Rank) { |
| /// Find instructions which should get is_stmt applied because they implement | ||
| /// key functionality for a source atom, store results in | ||
| /// DwarfDebug::KeyInstructions. |
| ;; Check the 2nd call (line 4) gets is_stmt applied despite being part of group | ||
| ;; 1 and having lower precedence than the add. Check that the add stil gets | ||
| ;; is_stmt applied. | ||
| ;; There are two is_stmt line 4 entries are is_stmt because we don't float |
review nit, use `Group`, "Avoids the appearance that this could be different from the one earlier." Co-authored-by: Jeremy Morse <[email protected]>
There was a problem hiding this comment.
Tests: I'd personally prefer the input source and explanation at the top of the file, although this is a style thing.
Done
My understanding of this code is that within a basic block, it should be possible for there to be two sequences of instructions of equal group and rank that both get a buoy and is_stmt, if they're separated by some other atom group. Perhaps I'm wrong; but if I'm right, it probably wants explicit test coverage.
That is tested in dwarf-buoy-multi-key.mir (but was broken, now fixed).
There's a risk that the code being generated is brittle and frequently needs updating; I don't know what probability of that we find acceptable, let's just see how often it needs updating I guess.
The tests are not fun to read and were not fun to write, and probably won't be particularly fun to maintain as you have pointed out. Not sure this can be massively improved with the current strategy and I couldn't think of a different way to test it, sadly.
| # DBG-NEXT: 0x000000000000000a 2 0 0 0 0 0 | ||
| # DBG-NEXT: 0x000000000000000f 2 0 0 0 0 0 | ||
| # DBG-NEXT: 0x0000000000000014 2 0 0 0 0 0 | ||
| # DBG-NEXT: 0x0000000000000019 2 0 0 0 0 0 |
There was a problem hiding this comment.
Good spot. Fixed in commit "fix line entries not folding together properly". That also fixes a bug where a line should have been is_stmt but wasn't (accidentally floated is_stmt past a different atom, which was unintended)
| $ebx = MOV32ri 2, debug-location !DILocation(line: 2, scope: !5, atomGroup: 1, atomRank: 2) | ||
| $ebx = MOV32ri 3, debug-location !DILocation(line: 2, scope: !5, atomGroup: 1, atomRank: 1) | ||
| $eax = MOV32ri 4, debug-location !DILocation(line: 2, scope: !5) | ||
| $eax = MOV32ri 5, debug-location !DILocation(line: 2, scope: !5, atomGroup: 2, atomRank: 1) |
There was a problem hiding this comment.
Fixed this by addressing previous comment, good spot.
| // The current candidate is_stmt instructions for each source atom. | ||
| // Map {(InlinedAt, Group): (Rank, Instructions)}. | ||
| DenseMap<std::pair<DILocation *, uint32_t>, | ||
| std::pair<uint16_t, SmallVector<const MachineInstr *>>> | ||
| GroupCandidates; |
There was a problem hiding this comment.
Welp, one or two is better than the default SmallVector calculation of "several"
| # DBG-NEXT: 0x0000000000000019 2 0 0 0 0 0 is_stmt | ||
| # DBG-NEXT: 0x000000000000001e 2 0 0 0 0 0 is_stmt | ||
| # DBG-NEXT: 0x0000000000000023 2 0 0 0 0 0 is_stmt | ||
| # DBG-NEXT: 0x0000000000000029 2 0 0 0 0 0 is_stmt end_sequence |
There was a problem hiding this comment.
Is this setting is_stmt on something that isn't an instruction (after retq)? It's end_sequence here, but could this leak out into different functions under different circumstances? Or it's just an off-by-one.
There was a problem hiding this comment.
It's "normal" - I think the end_sequence entry probably leaves all the other registers untouched to avoid unnecessary bloat? Replicating the is_stmt end_sequence with simple input without key instructions: https://godbolt.org/z/TsbTM8j8q
Interpret Key Instructions metadata to determine is_stmt placement.
The lowest rank (highest precedent) instructions in each {InlinedAt, atomGroup}
set are candidates for is_stmt. Only the last instruction in each set in a given
block gets is_stmt. Calls always get is_stmt.
RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
Interpret Key Instructions metadata to determine is_stmt placement.
The lowest rank (highest precedent) instructions in each {InlinedAt, atomGroup}
set are candidates for is_stmt. Only the last instruction in each set in a given
block gets is_stmt. Calls always get is_stmt.
RFC: https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668
Interpret Key Instructions metadata to determine is_stmt placement.
The lowest rank (highest precedent) instructions in each {InlinedAt, atomGroup}
set are candidates for is_stmt. Only the last instruction in each set in a given
block gets is_stmt. Calls always get is_stmt.