[llvm] [DebugInfo] Don't apply is_stmt on MBB branches that preserve lines (PR #108251)
Stephen Tozer via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 5 10:40:33 PST 2024
https://github.com/SLTozer updated https://github.com/llvm/llvm-project/pull/108251
>From 65ca4aba64c1659564156ee1e8bde6176a8beae6 Mon Sep 17 00:00:00 2001
From: Stephen Tozer <stephen.tozer at sony.com>
Date: Wed, 11 Sep 2024 17:52:08 +0100
Subject: [PATCH 1/5] [DebugInfo] Don't apply is_stmt on MBB branches that
preserve lines
This patch follows on from the changes made in 5fef40c, by adding an
additional heuristic that prevents us from applying the start-of-MBB
is_stmt flag when we can see that, for all direct branches to the MBB,
the last line stepped on before the branch is the same as the first line
of the MBB. This is mainly to prevent certain pathological cases, such as
macros that expand to multiple basic blocks that all have the same source
location, from giving us repeated steps on the same line. This approach
is not comprehensive, since it relies on analyzeBranch to read edges.
---
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | 123 +++++++++++++++++-
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h | 2 +
.../DebugInfo/X86/is_stmt-at-block-start.ll | 62 +++++++++
3 files changed, 182 insertions(+), 5 deletions(-)
create mode 100644 llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index aa44d62da47be9..9c89ff75968795 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2061,10 +2061,10 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
unsigned LastAsmLine =
Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
- bool PrevInstInSameSection =
- (!PrevInstBB ||
- PrevInstBB->getSectionID() == MI->getParent()->getSectionID());
- if (DL == PrevInstLoc && PrevInstInSameSection) {
+ bool PrevInstInDiffBB = PrevInstBB && PrevInstBB != MI->getParent();
+ bool ForceIsStmt =
+ PrevInstInDiffBB && MBBsStartingWithIsStmt.contains(MI->getParent());
+ if (DL == PrevInstLoc && !ForceIsStmt) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
return;
@@ -2121,7 +2121,7 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
// 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)
+ if (DL.getLine() && (DL.getLine() != OldLine || ForceIsStmt))
Flags |= DWARF2_FLAG_IS_STMT;
const MDNode *Scope = DL.getScope();
@@ -2229,6 +2229,119 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// Record beginning of function.
PrologEndLoc = emitInitialLocDirective(
*MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
+
+ // Try to determine what line we would be stepped on before entering each MBB.
+ // This happens in the following ways:
+ // - If this block has a single predecessor, we determine the last line in
+ // the pred block and we have stepped from that.
+ // - If this block has multiple predecessors, we determine the last line in
+ // each of them; if they all agree then we have stepped from that line,
+ // otherwise we do not know what line we have stepped from.
+ // The last line in each MBB is determined as follows:
+ // - If the block contains at least one DebugLoc, we have stepped from the
+ // last one.
+ // - Otherwise, the last line is line 0.
+ // There is one extra rule however: if a predecessor branches to the current
+ // basic block, we only count DebugLocs on or before that branch, so if we're
+ // looking at the MBB %bb.0, which ends with:
+ // JCC_1 %bb.1, 0, !debug-location !1
+ // JMP_1 %bb.2, !debug-location !2
+ // We would consider !1 to be the last loc before %bb.1, and !2 before %bb.2.
+ // We also don't need to make this calculation for any block that doesn't have
+ // a non-0 line number on its first instruction, as we will always emit line 0
+ // without is_stmt for that block regardless.
+ MBBsStartingWithIsStmt.clear();
+
+ // First, collect the last stepped line for each MBB.
+ SmallDenseMap<std::pair<const MachineBasicBlock *, const MachineBasicBlock *>,
+ unsigned>
+ MBBExitLines;
+ const auto *TII = MF->getSubtarget().getInstrInfo();
+
+ // We only need to examine MBBs that could have is_stmt set by this logic.
+ // We use const_cast even though we won't actually modify MF, because some
+ // methods we need take a non-const MBB.
+ SetVector<MachineBasicBlock *> PredMBBsToExamine;
+ SmallVector<MachineBasicBlock *> PotentialIsStmtMBBs;
+ for (auto &MBB : *const_cast<MachineFunction *>(MF)) {
+ if (MBB.pred_empty() || !MBB.begin()->getDebugLoc())
+ continue;
+ unsigned StartLine = MBB.begin()->getDebugLoc()->getLine();
+ if (StartLine == 0)
+ continue;
+ PotentialIsStmtMBBs.push_back(&MBB);
+ for (auto Pred : MBB.predecessors())
+ PredMBBsToExamine.insert(&*Pred);
+ }
+
+ // For each predecessor MBB, we examine the last DebugLoc seen before each
+ // branch or logical fallthrough. We're generous with applying is_stmt; if
+ // there's an edge that TargetInstrInfo::analyzeBranch can't understand, we
+ // simply assume that is_stmt ought to be applied to the successors, since
+ // this rule is mainly intended to avoid excessive stepping on lines that
+ // expand to many lines of code.
+ for (auto *MBB : PredMBBsToExamine) {
+ assert(!MBB->empty() && "Shouldn't be processing empty blocks here.");
+ MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
+ SmallVector<MachineOperand, 4> Cond;
+ if (TII->analyzeBranch(*MBB, TBB, FBB, Cond)) {
+ // We can't determine what DLs this branch's successors use, so skip it.
+ continue;
+ }
+ assert(TBB && "Bad result from analyzeBranch?");
+ auto MI = MBB->rbegin();
+ // For a conditional branch followed by unconditional branch where the
+ // unconditional branch has a DebugLoc, that loc is the outgoing loc to the
+ // false destination only; otherwise, both destinations share an outgoing
+ // loc.
+ if (!Cond.empty() && FBB != nullptr && MBB->back().getDebugLoc()) {
+ assert(MI->isBranch() && "Bad result from analyzeBranch?");
+ MBBExitLines.insert({{MBB, FBB}, MBB->back().getDebugLoc()->getLine()});
+ FBB = nullptr;
+ ++MI;
+ } else if (!Cond.empty() && !FBB) {
+ // For a conditional branch that falls through to the next block, the
+ // fallthrough block is the false branch.
+ FBB = MBB->getLogicalFallThrough();
+ assert(FBB &&
+ "Block ending with just a conditional branch should fallthrough.");
+ }
+
+ // If we don't find an outgoing loc, this block will start with a line 0.
+ unsigned LastLine = 0;
+ while (MI != MBB->rend()) {
+ if (auto DL = MI->getDebugLoc()) {
+ LastLine = DL->getLine();
+ break;
+ }
+ ++MI;
+ }
+ MBBExitLines.insert({{MBB, TBB}, LastLine});
+ if (FBB)
+ MBBExitLines.insert({{MBB, FBB}, LastLine});
+ }
+
+ // Now use the outgoing values to determine the incoming values for each
+ // block.
+ MBBsStartingWithIsStmt.insert(&*MF->begin());
+ for (auto *MBB : PotentialIsStmtMBBs) {
+ assert(!MBB->empty() && "Shouldn't be processing empty blocks here.");
+ if (!MBB->begin()->getDebugLoc())
+ continue;
+ unsigned StartLine = MBB->begin()->getDebugLoc()->getLine();
+ if (StartLine == 0)
+ continue;
+ for (auto Pred : MBB->predecessors()) {
+ auto LineIt = MBBExitLines.find({&*Pred, MBB});
+ // If there is no entry, it means there's a branch here that we couldn't
+ // track, so we can't be sure about what line we're arriving from;
+ // therefore assume that we should use is_stmt.
+ if (LineIt == MBBExitLines.end() || LineIt->second != StartLine) {
+ MBBsStartingWithIsStmt.insert(MBB);
+ break;
+ }
+ }
+ }
}
unsigned
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index 19f5b677bb8d06..80a6c7bd34d914 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -382,6 +382,8 @@ class DwarfDebug : public DebugHandlerBase {
SmallPtrSet<const MDNode *, 2>>;
DenseMap<const DILocalScope *, MDNodeSet> LocalDeclsPerLS;
+ SmallDenseSet<const MachineBasicBlock *> MBBsStartingWithIsStmt;
+
/// If nonnull, stores the current machine function we're processing.
const MachineFunction *CurFn = nullptr;
diff --git a/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll b/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll
new file mode 100644
index 00000000000000..ca9305d476d01f
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll
@@ -0,0 +1,62 @@
+;; Checks that when an instruction at the start of a BasicBlock has the same
+;; DebugLoc as the instruction at the end of the previous BasicBlock, we add
+;; is_stmt to the new line, to ensure that we still step on it if we arrive from
+;; a BasicBlock other than the immediately preceding one, unless all known
+;; predecessor BasicBlocks end with the same line.
+
+; RUN: %llc_dwarf -mtriple=x86_64-unknown-linux -O0 -filetype=obj < %s | llvm-dwarfdump --debug-line - | FileCheck %s
+
+; CHECK: {{0x[0-9a-f]+}} 13 5 {{.+}} is_stmt
+; CHECK-NEXT: {{0x[0-9a-f]+}} 13 5 {{.+}} is_stmt
+
+define void @_Z1fi(i1 %cond) !dbg !21 {
+entry:
+ br i1 %cond, label %if.then2, label %if.else4
+
+if.then2: ; preds = %entry
+ br label %if.end8, !dbg !24
+
+if.else4: ; preds = %entry
+ %0 = load i32, ptr null, align 4, !dbg !24
+ %call5 = call i1 null(i32 %0)
+ ret void
+
+if.end8: ; preds = %if.then2
+ ret void
+}
+
+; CHECK: {{0x[0-9a-f]+}} 113 5 {{.+}} is_stmt
+; CHECK-NOT: {{0x[0-9a-f]+}} 113 5
+
+define void @_Z1gi(i1 %cond) !dbg !31 {
+entry:
+ br i1 %cond, label %if.then2, label %if.else4, !dbg !34
+
+if.then2: ; preds = %entry
+ br label %if.end8, !dbg !34
+
+if.else4: ; preds = %entry
+ %0 = load i32, ptr null, align 4, !dbg !34
+ %call5 = call i1 null(i32 %0)
+ ret void
+
+if.end8: ; preds = %if.then2
+ ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!20}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 20.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/home/gbtozers/dev/upstream-llvm")
+!20 = !{i32 2, !"Debug Info Version", i32 3}
+!21 = distinct !DISubprogram(name: "f", linkageName: "_Z1fi", scope: !1, file: !1, line: 7, type: !22, scopeLine: 7, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!22 = distinct !DISubroutineType(types: !23)
+!23 = !{null}
+!24 = !DILocation(line: 13, column: 5, scope: !25)
+!25 = distinct !DILexicalBlock(scope: !21, file: !1, line: 11, column: 27)
+!31 = distinct !DISubprogram(name: "g", linkageName: "_Z1gi", scope: !1, file: !1, line: 107, type: !32, scopeLine: 7, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!32 = distinct !DISubroutineType(types: !33)
+!33 = !{null}
+!34 = !DILocation(line: 113, column: 5, scope: !35)
+!35 = distinct !DILexicalBlock(scope: !31, file: !1, line: 111, column: 27)
>From 06a10b3cd4611019c066e6cccf9341343965cd09 Mon Sep 17 00:00:00 2001
From: Stephen Tozer <stephen.tozer at sony.com>
Date: Mon, 16 Sep 2024 13:05:50 +0100
Subject: [PATCH 2/5] Fixes and simplification
---
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | 135 +++++++++++----------
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h | 2 +-
2 files changed, 71 insertions(+), 66 deletions(-)
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 9c89ff75968795..04ab9f3955f880 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2061,9 +2061,7 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
unsigned LastAsmLine =
Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
- bool PrevInstInDiffBB = PrevInstBB && PrevInstBB != MI->getParent();
- bool ForceIsStmt =
- PrevInstInDiffBB && MBBsStartingWithIsStmt.contains(MI->getParent());
+ bool ForceIsStmt = ForceIsStmtInstrs.contains(MI);
if (DL == PrevInstLoc && !ForceIsStmt) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
@@ -2093,8 +2091,8 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
// possibly debug information; we want it to have a source location.
// - Instruction is at the top of a block; we don't want to inherit the
// location from the physically previous (maybe unrelated) block.
- if (UnknownLocations == Enable || PrevLabel ||
- (PrevInstBB && PrevInstBB != MI->getParent())) {
+ bool PrevInstInDiffBB = PrevInstBB && PrevInstBB != MI->getParent();
+ if (UnknownLocations == Enable || PrevLabel || PrevInstInDiffBB) {
// Preserve the file and column numbers, if we can, to save space in
// the encoded line table.
// Do not update PrevInstLoc, it remembers the last non-0 line.
@@ -2250,7 +2248,7 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// We also don't need to make this calculation for any block that doesn't have
// a non-0 line number on its first instruction, as we will always emit line 0
// without is_stmt for that block regardless.
- MBBsStartingWithIsStmt.clear();
+ ForceIsStmtInstrs.clear();
// First, collect the last stepped line for each MBB.
SmallDenseMap<std::pair<const MachineBasicBlock *, const MachineBasicBlock *>,
@@ -2262,16 +2260,18 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// We use const_cast even though we won't actually modify MF, because some
// methods we need take a non-const MBB.
SetVector<MachineBasicBlock *> PredMBBsToExamine;
- SmallVector<MachineBasicBlock *> PotentialIsStmtMBBs;
+ SmallDenseMap<MachineBasicBlock *, MachineInstr *> PotentialIsStmtMBBInstrs;
for (auto &MBB : *const_cast<MachineFunction *>(MF)) {
- if (MBB.pred_empty() || !MBB.begin()->getDebugLoc())
+ if (MBB.empty() || MBB.pred_empty())
continue;
- unsigned StartLine = MBB.begin()->getDebugLoc()->getLine();
- if (StartLine == 0)
- continue;
- PotentialIsStmtMBBs.push_back(&MBB);
- for (auto Pred : MBB.predecessors())
- PredMBBsToExamine.insert(&*Pred);
+ for (auto &MI : MBB) {
+ if (MI.getDebugLoc() && MI.getDebugLoc()->getLine()) {
+ for (auto Pred : MBB.predecessors())
+ PredMBBsToExamine.insert(&*Pred);
+ PotentialIsStmtMBBInstrs.insert({&MBB, &MI});
+ break;
+ }
+ }
}
// For each predecessor MBB, we examine the last DebugLoc seen before each
@@ -2281,66 +2281,71 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// this rule is mainly intended to avoid excessive stepping on lines that
// expand to many lines of code.
for (auto *MBB : PredMBBsToExamine) {
- assert(!MBB->empty() && "Shouldn't be processing empty blocks here.");
- MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
- SmallVector<MachineOperand, 4> Cond;
- if (TII->analyzeBranch(*MBB, TBB, FBB, Cond)) {
- // We can't determine what DLs this branch's successors use, so skip it.
+ auto CheckMBBEdge = [&](MachineBasicBlock *Succ, unsigned OutgoingLine) {
+ auto MBBInstrIt = PotentialIsStmtMBBInstrs.find(Succ);
+ if (MBBInstrIt == PotentialIsStmtMBBInstrs.end())
+ return;
+ MachineInstr *MI = MBBInstrIt->second;
+ if (MI->getDebugLoc()->getLine() == OutgoingLine)
+ return;
+ PotentialIsStmtMBBInstrs.erase(MBBInstrIt);
+ ForceIsStmtInstrs.insert(MI);
+ };
+ // If this block is empty, it technically has a fall through but we should
+ // assume it is irrelevant for the purposes of calculating is_stmt.
+ if (MBB->empty())
continue;
- }
- assert(TBB && "Bad result from analyzeBranch?");
- auto MI = MBB->rbegin();
- // For a conditional branch followed by unconditional branch where the
- // unconditional branch has a DebugLoc, that loc is the outgoing loc to the
- // false destination only; otherwise, both destinations share an outgoing
- // loc.
- if (!Cond.empty() && FBB != nullptr && MBB->back().getDebugLoc()) {
- assert(MI->isBranch() && "Bad result from analyzeBranch?");
- MBBExitLines.insert({{MBB, FBB}, MBB->back().getDebugLoc()->getLine()});
- FBB = nullptr;
- ++MI;
- } else if (!Cond.empty() && !FBB) {
- // For a conditional branch that falls through to the next block, the
- // fallthrough block is the false branch.
- FBB = MBB->getLogicalFallThrough();
- assert(FBB &&
- "Block ending with just a conditional branch should fallthrough.");
+ // If we can't determine what DLs this branch's successors use, just treat
+ // all the successors as coming from the last DebugLoc.
+ SmallVector<MachineBasicBlock *, 2> SuccessorBBs;
+ auto MIIt = MBB->rbegin();
+ {
+ MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
+ SmallVector<MachineOperand, 4> Cond;
+ bool AnalyzeFailed = TII->analyzeBranch(*MBB, TBB, FBB, Cond);
+ // For a conditional branch followed by unconditional branch where the
+ // unconditional branch has a DebugLoc, that loc is the outgoing loc to
+ // the the false destination only; otherwise, both destinations share an
+ // outgoing loc.
+ if (!AnalyzeFailed && !Cond.empty() && FBB != nullptr &&
+ MBB->back().getDebugLoc() && MBB->back().getDebugLoc()->getLine()) {
+ unsigned FBBLine = MBB->back().getDebugLoc()->getLine();
+ assert(MIIt->isBranch() && "Bad result from analyzeBranch?");
+ CheckMBBEdge(FBB, FBBLine);
+ FBB = nullptr;
+ ++MIIt;
+ SuccessorBBs.push_back(TBB);
+ } else {
+ // For all other cases, all successors share the last outgoing DebugLoc.
+ SuccessorBBs.assign(MBB->succ_begin(), MBB->succ_end());
+ }
}
// If we don't find an outgoing loc, this block will start with a line 0.
+ // It is possible that we have a block that has no DebugLoc, but acts as a
+ // simple passthrough between two blocks that end and start with the same
+ // line, e.g.:
+ // bb.1:
+ // JMP %bb.2, debug-location !10
+ // bb.2:
+ // JMP %bb.3
+ // bb.3:
+ // $r1 = ADD $r2, $r3, debug-location !10
+ // If these blocks were merged into a single block, we would not attach
+ // is_stmt to the ADD, but with this logic that only checks the immediate
+ // predecessor, we will; we make this tradeoff because doing a full dataflow
+ // analysis would be expensive, and these situations are probably not common
+ // enough for this to be worthwhile.
unsigned LastLine = 0;
- while (MI != MBB->rend()) {
- if (auto DL = MI->getDebugLoc()) {
+ while (MIIt != MBB->rend()) {
+ if (auto DL = MIIt->getDebugLoc(); DL && DL->getLine()) {
LastLine = DL->getLine();
break;
}
- ++MI;
- }
- MBBExitLines.insert({{MBB, TBB}, LastLine});
- if (FBB)
- MBBExitLines.insert({{MBB, FBB}, LastLine});
- }
-
- // Now use the outgoing values to determine the incoming values for each
- // block.
- MBBsStartingWithIsStmt.insert(&*MF->begin());
- for (auto *MBB : PotentialIsStmtMBBs) {
- assert(!MBB->empty() && "Shouldn't be processing empty blocks here.");
- if (!MBB->begin()->getDebugLoc())
- continue;
- unsigned StartLine = MBB->begin()->getDebugLoc()->getLine();
- if (StartLine == 0)
- continue;
- for (auto Pred : MBB->predecessors()) {
- auto LineIt = MBBExitLines.find({&*Pred, MBB});
- // If there is no entry, it means there's a branch here that we couldn't
- // track, so we can't be sure about what line we're arriving from;
- // therefore assume that we should use is_stmt.
- if (LineIt == MBBExitLines.end() || LineIt->second != StartLine) {
- MBBsStartingWithIsStmt.insert(MBB);
- break;
- }
+ ++MIIt;
}
+ for (auto Succ : SuccessorBBs)
+ CheckMBBEdge(Succ, LastLine);
}
}
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index 80a6c7bd34d914..8ee434ab7d4587 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -382,7 +382,7 @@ class DwarfDebug : public DebugHandlerBase {
SmallPtrSet<const MDNode *, 2>>;
DenseMap<const DILocalScope *, MDNodeSet> LocalDeclsPerLS;
- SmallDenseSet<const MachineBasicBlock *> MBBsStartingWithIsStmt;
+ SmallDenseSet<const MachineInstr *> ForceIsStmtInstrs;
/// If nonnull, stores the current machine function we're processing.
const MachineFunction *CurFn = nullptr;
>From 59b4657b9b7640bd3daef7a15f9809d1eecbccce Mon Sep 17 00:00:00 2001
From: Stephen Tozer <stephen.tozer at sony.com>
Date: Thu, 3 Oct 2024 15:24:25 +0100
Subject: [PATCH 3/5] Minor fixups, comment adjustments
---
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | 42 ++++++++--------------
1 file changed, 14 insertions(+), 28 deletions(-)
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 04ab9f3955f880..6415609b31428c 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2228,26 +2228,15 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
PrologEndLoc = emitInitialLocDirective(
*MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
- // Try to determine what line we would be stepped on before entering each MBB.
- // This happens in the following ways:
- // - If this block has a single predecessor, we determine the last line in
- // the pred block and we have stepped from that.
- // - If this block has multiple predecessors, we determine the last line in
- // each of them; if they all agree then we have stepped from that line,
- // otherwise we do not know what line we have stepped from.
- // The last line in each MBB is determined as follows:
- // - If the block contains at least one DebugLoc, we have stepped from the
- // last one.
- // - Otherwise, the last line is line 0.
- // There is one extra rule however: if a predecessor branches to the current
- // basic block, we only count DebugLocs on or before that branch, so if we're
- // looking at the MBB %bb.0, which ends with:
- // JCC_1 %bb.1, 0, !debug-location !1
- // JMP_1 %bb.2, !debug-location !2
- // We would consider !1 to be the last loc before %bb.1, and !2 before %bb.2.
- // We also don't need to make this calculation for any block that doesn't have
- // a non-0 line number on its first instruction, as we will always emit line 0
- // without is_stmt for that block regardless.
+ // For each MBB we try to determine whether and what source line is *always*
+ // the last stepped-on line before entering MBB. Such a line exists if every
+ // predecessor has an instruction with source line N, which is the last valid
+ // source line to be seen before entering MBB, and if N is the same for all
+ // predecessors. If this is true, and the first instruction with a valid
+ // source line in MBB also has source line N, then that instruction should
+ // *not* use is_stmt. Otherwise, the first instruction with a valid source
+ // line in MBB should always use is_stmt, since we may step to it from a
+ // different line.
ForceIsStmtInstrs.clear();
// First, collect the last stepped line for each MBB.
@@ -2266,8 +2255,8 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
continue;
for (auto &MI : MBB) {
if (MI.getDebugLoc() && MI.getDebugLoc()->getLine()) {
- for (auto Pred : MBB.predecessors())
- PredMBBsToExamine.insert(&*Pred);
+ for (auto *Pred : MBB.predecessors())
+ PredMBBsToExamine.insert(Pred);
PotentialIsStmtMBBInstrs.insert({&MBB, &MI});
break;
}
@@ -2275,11 +2264,9 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
}
// For each predecessor MBB, we examine the last DebugLoc seen before each
- // branch or logical fallthrough. We're generous with applying is_stmt; if
+ // branch or logical fallthrough. We're generous with applying is_stmt: if
// there's an edge that TargetInstrInfo::analyzeBranch can't understand, we
- // simply assume that is_stmt ought to be applied to the successors, since
- // this rule is mainly intended to avoid excessive stepping on lines that
- // expand to many lines of code.
+ // assume that all successor MBBs use the last DebugLoc seen.
for (auto *MBB : PredMBBsToExamine) {
auto CheckMBBEdge = [&](MachineBasicBlock *Succ, unsigned OutgoingLine) {
auto MBBInstrIt = PotentialIsStmtMBBInstrs.find(Succ);
@@ -2312,7 +2299,6 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
unsigned FBBLine = MBB->back().getDebugLoc()->getLine();
assert(MIIt->isBranch() && "Bad result from analyzeBranch?");
CheckMBBEdge(FBB, FBBLine);
- FBB = nullptr;
++MIIt;
SuccessorBBs.push_back(TBB);
} else {
@@ -2344,7 +2330,7 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
}
++MIIt;
}
- for (auto Succ : SuccessorBBs)
+ for (auto *Succ : SuccessorBBs)
CheckMBBEdge(Succ, LastLine);
}
}
>From 259515964b7d45533ee3f20e08f43fa02ec2e5ad Mon Sep 17 00:00:00 2001
From: Stephen Tozer <stephen.tozer at sony.com>
Date: Thu, 3 Oct 2024 16:49:11 +0100
Subject: [PATCH 4/5] Re-add unnecessarily removed code
---
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 6415609b31428c..bb1a8c2eee5834 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2061,8 +2061,11 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
unsigned LastAsmLine =
Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
+ bool PrevInstInSameSection =
+ (!PrevInstBB ||
+ PrevInstBB->getSectionID() == MI->getParent()->getSectionID());
bool ForceIsStmt = ForceIsStmtInstrs.contains(MI);
- if (DL == PrevInstLoc && !ForceIsStmt) {
+ if (DL == PrevInstLoc && PrevInstInSameSection && !ForceIsStmt) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
return;
@@ -2091,8 +2094,8 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
// possibly debug information; we want it to have a source location.
// - Instruction is at the top of a block; we don't want to inherit the
// location from the physically previous (maybe unrelated) block.
- bool PrevInstInDiffBB = PrevInstBB && PrevInstBB != MI->getParent();
- if (UnknownLocations == Enable || PrevLabel || PrevInstInDiffBB) {
+ if (UnknownLocations == Enable || PrevLabel ||
+ (PrevInstBB && PrevInstBB != MI->getParent())) {
// Preserve the file and column numbers, if we can, to save space in
// the encoded line table.
// Do not update PrevInstLoc, it remembers the last non-0 line.
>From 84359bbb939c8fdaae0deda17d601695d5ab11a6 Mon Sep 17 00:00:00 2001
From: Stephen Tozer <stephen.tozer at sony.com>
Date: Tue, 5 Nov 2024 18:17:32 +0000
Subject: [PATCH 5/5] Address review comments
---
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp | 71 ++++++++++++++--------
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h | 2 +
2 files changed, 47 insertions(+), 26 deletions(-)
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index bb1a8c2eee5834..d233e12956296b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2212,27 +2212,15 @@ DwarfDebug::emitInitialLocDirective(const MachineFunction &MF, unsigned CUID) {
return PrologEndLoc;
}
-// Gather pre-function debug information. Assumes being called immediately
-// after the function entry point has been emitted.
-void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
- CurFn = MF;
-
- auto *SP = MF->getFunction().getSubprogram();
- assert(LScopes.empty() || SP == LScopes.getCurrentFunctionScope()->getScopeNode());
- if (SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug)
- return;
-
- DwarfCompileUnit &CU = getOrCreateDwarfCompileUnit(SP->getUnit());
-
- Asm->OutStreamer->getContext().setDwarfCompileUnitID(
- getDwarfCompileUnitIDForLineTable(CU));
-
- // Record beginning of function.
- PrologEndLoc = emitInitialLocDirective(
- *MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
+/// 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
+/// ForceIsStmtInstrs.
+void DwarfDebug::findForceIsStmtInstrs(const MachineFunction *MF) {
+ ForceIsStmtInstrs.clear();
- // For each MBB we try to determine whether and what source line is *always*
- // the last stepped-on line before entering MBB. Such a line exists if every
+ // Here we try to find MBBs where the last line stepped on before entering
+ // it is always the same, and which line that is. Such a line exists if every
// predecessor has an instruction with source line N, which is the last valid
// source line to be seen before entering MBB, and if N is the same for all
// predecessors. If this is true, and the first instruction with a valid
@@ -2240,8 +2228,6 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// *not* use is_stmt. Otherwise, the first instruction with a valid source
// line in MBB should always use is_stmt, since we may step to it from a
// different line.
- ForceIsStmtInstrs.clear();
-
// First, collect the last stepped line for each MBB.
SmallDenseMap<std::pair<const MachineBasicBlock *, const MachineBasicBlock *>,
unsigned>
@@ -2251,7 +2237,7 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// We only need to examine MBBs that could have is_stmt set by this logic.
// We use const_cast even though we won't actually modify MF, because some
// methods we need take a non-const MBB.
- SetVector<MachineBasicBlock *> PredMBBsToExamine;
+ SmallSetVector<MachineBasicBlock *, 4> PredMBBsToExamine;
SmallDenseMap<MachineBasicBlock *, MachineInstr *> PotentialIsStmtMBBInstrs;
for (auto &MBB : *const_cast<MachineFunction *>(MF)) {
if (MBB.empty() || MBB.pred_empty())
@@ -2281,9 +2267,19 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
PotentialIsStmtMBBInstrs.erase(MBBInstrIt);
ForceIsStmtInstrs.insert(MI);
};
- // If this block is empty, it technically has a fall through but we should
- // assume it is irrelevant for the purposes of calculating is_stmt.
- if (MBB->empty())
+ // If this block is empty, we conservatively assume that its fallthrough
+ // successor needs is_stmt; we could check MBB's predecessors to see if it
+ // has a consistent entry line, but this seems unlikely to be worthwhile.
+ if (MBB->empty()) {
+ for (auto *Succ : MBB->successors())
+ CheckMBBEdge(Succ, 0);
+ continue;
+ }
+ // If MBB has no successors that are in the "potential" set, due to one or
+ // more of them having confirmed is_stmt, we can skip this check early.
+ if (none_of(MBB->successors(), [&](auto *SuccMBB) {
+ return PotentialIsStmtMBBInstrs.contains(SuccMBB);
+ }))
continue;
// If we can't determine what DLs this branch's successors use, just treat
// all the successors as coming from the last DebugLoc.
@@ -2338,6 +2334,29 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
}
}
+// Gather pre-function debug information. Assumes being called immediately
+// after the function entry point has been emitted.
+void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
+ CurFn = MF;
+
+ auto *SP = MF->getFunction().getSubprogram();
+ assert(LScopes.empty() ||
+ SP == LScopes.getCurrentFunctionScope()->getScopeNode());
+ if (SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug)
+ return;
+
+ DwarfCompileUnit &CU = getOrCreateDwarfCompileUnit(SP->getUnit());
+
+ Asm->OutStreamer->getContext().setDwarfCompileUnitID(
+ getDwarfCompileUnitIDForLineTable(CU));
+
+ // Record beginning of function.
+ PrologEndLoc = emitInitialLocDirective(
+ *MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
+
+ findForceIsStmtInstrs(MF);
+}
+
unsigned
DwarfDebug::getDwarfCompileUnitIDForLineTable(const DwarfCompileUnit &CU) {
// Set DwarfDwarfCompileUnitID in MCContext to the Compile Unit this function
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index 8ee434ab7d4587..c1353c35c86265 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -699,6 +699,8 @@ class DwarfDebug : public DebugHandlerBase {
/// Emit the reference to the section.
void emitSectionReference(const DwarfCompileUnit &CU);
+ void findForceIsStmtInstrs(const MachineFunction *MF);
+
protected:
/// Gather pre-function debug information.
void beginFunctionImpl(const MachineFunction *MF) override;
More information about the llvm-commits
mailing list