Skip to content

[DebugInfo] Preserve line and column number when merging debug info. #129960

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.

Sign up for GitHub

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 7 commits into from
Apr 4, 2025
Merged

[DebugInfo] Preserve line and column number when merging debug info. #129960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cb23baa
[DebugInfo] Preserve line and column number when merging debug info.
Mar 5, 2025
df04e25
Address comments.
Mar 5, 2025
e43ffd1
Update docs to mention SamplePGO debug info usage.
Mar 14, 2025
ffea531
Fix typo.
Mar 18, 2025
713ae85
Enhance the documentation for SamplePGO.
Mar 24, 2025
4ba8427
Address comments.
Mar 31, 2025
7129758
Fix formatting.
Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions llvm/lib/IR/DebugInfoMetadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Type.h"
#include "llvm/IR/Value.h"
#include "llvm/Support/CommandLine.h"

#include <numeric>
#include <optional>
Expand All @@ -34,6 +35,12 @@ cl::opt<bool> EnableFSDiscriminator(
cl::desc("Enable adding flow sensitive discriminators"));
} // namespace llvm

// When true, preserves line and column number by picking one of the merged
Copy link
Collaborator

@rnk rnk Mar 31, 2025

Choose a reason for hiding this comment

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

I'd like to have a more descriptive name for this option, since we are discarding information, we're not preserving half of the merged source location. I came up with some suggestions:

  • -merge-source-locs-by-picking
  • -pick-merged-source-locations
  • -merge-source-locations-with-selection
  • -merge-source-locations-by-choice
  • -no-approximate-merged-source-locs

I'm really just trying various synonyms for "pick", "select", "choose".

Copy link
Author

@snehasish snehasish Mar 31, 2025

Choose a reason for hiding this comment

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

Done, "pick-merged-source-locations" sounds good to me.

// location info in a deterministic manner to assist sample based PGO.
static cl::opt<bool> PreserveMergedDebugInfo(
"preserve-merged-debug-info", cl::init(false), cl::Hidden,
cl::desc("Preserve line and column number when merging locations."));

uint32_t DIType::getAlignInBits() const {
return (getTag() == dwarf::DW_TAG_LLVM_ptrauth_type ? 0 : SubclassData32);
}
Expand Down Expand Up @@ -125,6 +132,14 @@ DILocation *DILocation::getMergedLocation(DILocation *LocA, DILocation *LocB) {
if (LocA == LocB)
return LocA;

if (PreserveMergedDebugInfo) {
auto A = std::make_tuple(LocA->getLine(), LocA->getColumn(),
Copy link
Collaborator

@rnk rnk Mar 31, 2025

Choose a reason for hiding this comment

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

Should filenames be compared here? DILocation has getFilename and getDirectory, which return StringRefs, so you can fill them in here in a reasonably straightforward way.

However, given the use case, would it be reasonable to always pick LocA?

Regardless, I think adding directories and files would follow the principle of least surprise.

Copy link
Author

@snehasish snehasish Mar 31, 2025

Choose a reason for hiding this comment

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

Sure, I've added getFilename and getDirectory to the comparison. Note that SamplePGO doesn't rely on file and directory today when using instruction locations.

would it be reasonable to always pick LocA

I think the pick policy should be independent of the order of the parameters passed to this function. So getMergedLocation(A, B) == getMergedLocation(B, A).

Copy link
Contributor

@rlavaee rlavaee May 2, 2025

Choose a reason for hiding this comment

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

nit: Does std::forward_as_tuple work here (to avoid copies)?

Copy link
Author

@snehasish snehasish May 2, 2025

Choose a reason for hiding this comment

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

No, the only place it would be useful is if getFilename and getDirectory returned string, but they return StringRef. Also you'd have to rewrite this to do the comparison without the auto A and auto B vars or you'd introduce lifetime issues if you just replaced make_tuple with forward_as_tuple.

LocA->getDiscriminator());
auto B = std::make_tuple(LocB->getLine(), LocB->getColumn(),
LocB->getDiscriminator());
return A < B ? LocA : LocB;
}

LLVMContext &C = LocA->getContext();

using LocVec = SmallVector<const DILocation *>;
Expand Down
65 changes: 65 additions & 0 deletions llvm/test/DebugInfo/preserve-merged-debug-info.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
; RUN: opt %s -passes=simplifycfg -hoist-common-insts -preserve-merged-debug-info -S | FileCheck %s
Copy link
Contributor

@OCHyams OCHyams Mar 6, 2025

Choose a reason for hiding this comment

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

I think the test could be made more robust by adding CHECKs to show that simplifycfg has done the hoisting (at the moment, I think this test would pass even if the hoisting didn't take place?).

Possibly also worth checking with -preserve-merged-debug-info=false too (that the instruction gets a merged loc) to avoid this test spuriously passing if, for whatever reason, the default behaviour changes to a non-merged-loc for this test? YMMV.

Copy link
Author

@snehasish snehasish Mar 6, 2025

Choose a reason for hiding this comment

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

Done for both, thanks for the suggestion.

; CHECK: tail call i32 @bar{{.*!dbg !}}[[TAG:[0-9]+]]
; CHECK: ![[TAG]] = !DILocation(line: 9, column: 16, scope: !9)

Copy link
Member

@jmorse jmorse Mar 6, 2025

Choose a reason for hiding this comment

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

A file-level comment recording the intention of this test will assist future developers who cause this test to fail.

Copy link
Author

@snehasish snehasish Mar 6, 2025

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@rnk rnk Mar 31, 2025

Choose a reason for hiding this comment

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

You know, we have unit tests for this API with decent fixtures:

llvm-project/llvm/unittests/IR/MetadataTest.cpp

Line 921 in a8d2d16

TEST_F(DILocationTest, Merge) {

Maybe this is more trouble than it's worth, but you could make the cl::opt non-static, declare it in the unittest, modify it directly (PreserveMergedDebugInfo = true/false) and test it that way. No strong preference on my part, though,

Copy link
Author

@snehasish snehasish Mar 31, 2025

Choose a reason for hiding this comment

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

I'd prefer to leave this as is since the current test also covers the SimplifyCfg transformations usage of getMergedLocation . This is the case we found to be particularly impactful to preserve.

; ModuleID = '../llvm/test/DebugInfo/Inputs/debug-info-merge-call.c'
source_filename = "../llvm/test/DebugInfo/Inputs/debug-info-merge-call.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: nounwind uwtable
define dso_local i32 @test(i32 noundef %n) local_unnamed_addr #0 !dbg !9 {
entry:
%call = tail call i32 @foo(i32 noundef %n) #2, !dbg !12
%cmp1 = icmp sgt i32 %n, 100, !dbg !13
br i1 %cmp1, label %if.then, label %if.else, !dbg !13

if.then: ; preds = %entry
%call2 = tail call i32 @bar(i32 noundef %n) #2, !dbg !14
%add = add nsw i32 %call2, %call, !dbg !15
br label %if.end, !dbg !16

if.else: ; preds = %entry
%call4 = tail call i32 @bar(i32 noundef %n) #2, !dbg !17
br label %if.end

if.end: ; preds = %if.else, %if.then
%r.0 = phi i32 [ %add, %if.then ], [ %call4, %if.else ], !dbg !18
ret i32 %r.0, !dbg !19
}

declare !dbg !20 i32 @foo(i32 noundef) local_unnamed_addr #1

declare !dbg !21 i32 @bar(i32 noundef) local_unnamed_addr #1

attributes #0 = { nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #2 = { nounwind }
Copy link
Member

@jmorse jmorse Mar 6, 2025

Choose a reason for hiding this comment

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

Nit -- if we can remove or reduce these attributes that'd be appreciated, it's a future maintenance burden when attributes change otherwise.

Copy link
Author

@snehasish snehasish Mar 6, 2025

Choose a reason for hiding this comment

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

Done.


!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!2, !3, !4, !5, !6, !7}
!llvm.ident = !{!8}

!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 21.0.0git ([email protected]:snehasish/llvm-project.git 6ce41db6b0275d060d6e60f88b96a1657024345c)", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "../llvm/test/DebugInfo/Inputs/debug-info-merge-call.c", directory: "/usr/local/google/home/snehasishk/working/llvm-project/build-assert", checksumkind: CSK_MD5, checksum: "ac1be6c40dad11691922d600f9d55c55")
!2 = !{i32 7, !"Dwarf Version", i32 5}
!3 = !{i32 2, !"Debug Info Version", i32 3}
!4 = !{i32 1, !"wchar_size", i32 4}
!5 = !{i32 8, !"PIC Level", i32 2}
!6 = !{i32 7, !"PIE Level", i32 2}
!7 = !{i32 7, !"uwtable", i32 2}
!8 = !{!"clang version 21.0.0git ([email protected]:snehasish/llvm-project.git 6ce41db6b0275d060d6e60f88b96a1657024345c)"}
!9 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 5, type: !10, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
!10 = !DISubroutineType(types: !11)
!11 = !{}
!12 = !DILocation(line: 7, column: 13, scope: !9)
!13 = !DILocation(line: 8, column: 8, scope: !9)
!14 = !DILocation(line: 9, column: 16, scope: !9)
!15 = !DILocation(line: 9, column: 14, scope: !9)
!16 = !DILocation(line: 10, column: 3, scope: !9)
!17 = !DILocation(line: 11, column: 10, scope: !9)
!18 = !DILocation(line: 0, scope: !9)
!19 = !DILocation(line: 13, column: 3, scope: !9)
!20 = !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
!21 = !DISubprogram(name: "bar", scope: !1, file: !1, line: 1, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)