Skip to content

[KeyInstr][DwarfDebug] Add is_stmt emission support #133495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
May 13, 2025

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Mar 28, 2025

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.

Copy link
Contributor Author

OCHyams commented Mar 28, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

github-actions bot commented Mar 28, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

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.h
View 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;

@OCHyams OCHyams marked this pull request as ready for review March 28, 2025 18:00
@llvmbot
Copy link
Member

llvmbot commented Mar 28, 2025

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

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.


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:

  • (modified) llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (+198-12)
  • (modified) llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h (+9)
  • (added) llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic-ranks.ll (+68)
  • (added) llvm/test/DebugInfo/KeyInstructions/X86/dwarf-basic.ll (+62)
  • (added) llvm/test/DebugInfo/KeyInstructions/X86/dwarf-buoy-multi-key.mir (+78)
  • (added) llvm/test/DebugInfo/KeyInstructions/X86/dwarf-buoy.mir (+66)
  • (added) llvm/test/DebugInfo/KeyInstructions/X86/dwarf-calls.ll (+117)
  • (added) llvm/test/DebugInfo/KeyInstructions/X86/dwarf-ranks-blocks.ll (+65)
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]

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines 708 to 710
/// Find instructions which should get is_stmt applied because they implement
/// key functionality for a source atom, store results in
/// DwarfDebug::KeyInstructions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines +2099 to +2102
// Skip this if the instruction is Key, else we might accidentally miss an
// is_stmt.
if (!IsKey) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 2483 to 2500
// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines 2359 to 2368
// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
KeyInstructions.insert(Buoy);
KeyInstructions.insert(&MI);

Avoids any doubt about what's being inserted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment to highlight that sometimes MI != Buoy.


uint64_t Group = MI.getDebugLoc()->getAtomGroup();
uint8_t Rank = MI.getDebugLoc()->getAtomRank();
if (Group && Rank) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early-continue instead perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 24 to 27
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
;; 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

$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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this by addressing previous comment, good spot.

@OCHyams OCHyams force-pushed the users/OCHyams/ki-llvm-mir-parser branch from b49eb51 to d2ba768 Compare May 8, 2025 09:38
Base automatically changed from users/OCHyams/ki-llvm-mir-parser to main May 8, 2025 09:40
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 OCHyams force-pushed the users/OCHyams/ki-llvm-dwarf-emission branch from 0b206ae to 0b5166a Compare May 8, 2025 12:48
Copy link
Contributor Author

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +2099 to +2102
// Skip this if the instruction is Key, else we might accidentally miss an
// is_stmt.
if (!IsKey) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


uint64_t Group = MI.getDebugLoc()->getAtomGroup();
uint8_t Rank = MI.getDebugLoc()->getAtomRank();
if (Group && Rank) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 708 to 710
/// Find instructions which should get is_stmt applied because they implement
/// key functionality for a source atom, store results in
/// DwarfDebug::KeyInstructions.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

;; 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

OCHyams and others added 3 commits May 9, 2025 12:16
review nit, use `Group`,  "Avoids the appearance that this could be different from the one earlier."

Co-authored-by: Jeremy Morse <[email protected]>
Copy link
Contributor Author

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 24 to 27
# 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this by addressing previous comment, good spot.

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with an inline question (plus please do apply the smallvector-size)

Comment on lines 2359 to 2368
// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@OCHyams OCHyams merged commit e7547b2 into main May 13, 2025
5 of 9 checks passed
@OCHyams OCHyams deleted the users/OCHyams/ki-llvm-dwarf-emission branch May 13, 2025 16:26
OCHyams added a commit to OCHyams/llvm-project that referenced this pull request May 14, 2025
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
OCHyams added a commit to OCHyams/llvm-project that referenced this pull request May 14, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants