Skip to content

Fix performance bug in buildLocationList #109343

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 3 commits into from
Oct 31, 2024
Merged

Fix performance bug in buildLocationList #109343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53c0140
Fix performance bug in buildLocationList
tmsri Sep 19, 2024
11a5026
Const value variable within a scope test.
tmsri Sep 23, 2024
5c8bec4
Two more test cases where DW_AT_const cannot be used.
tmsri Oct 30, 2024
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
13 changes: 8 additions & 5 deletions llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1737,6 +1737,12 @@ void AsmPrinter::emitFunctionBody() {
bool IsEHa = MMI->getModule()->getModuleFlag("eh-asynch");

bool CanDoExtraAnalysis = ORE->allowExtraAnalysis(DEBUG_TYPE);
// Create a slot for the entry basic block section so that the section
// order is preserved when iterating over MBBSectionRanges.
if (!MF->empty())
MBBSectionRanges[MF->front().getSectionID()] =
MBBSectionRange{CurrentFnBegin, nullptr};

for (auto &MBB : *MF) {
// Print a label for the basic block.
emitBasicBlockStart(MBB);
Expand Down Expand Up @@ -2000,11 +2006,8 @@ void AsmPrinter::emitFunctionBody() {
}
for (auto &Handler : Handlers)
Handler->markFunctionEnd();

assert(!MBBSectionRanges.contains(MF->front().getSectionID()) &&
"Overwrite section range");
MBBSectionRanges[MF->front().getSectionID()] =
MBBSectionRange{CurrentFnBegin, CurrentFnEnd};
// Update the end label of the entry block's section.
MBBSectionRanges[MF->front().getSectionID()].EndLabel = CurrentFnEnd;

// Print out jump tables referenced by the function.
emitJumpTableInfo();
Expand Down
38 changes: 20 additions & 18 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/DebugInfoMetadata.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/Module.h"
Expand Down Expand Up @@ -1772,18 +1773,14 @@ bool DwarfDebug::buildLocationList(SmallVectorImpl<DebugLocEntry> &DebugLoc,
// span each individual section in the range from StartLabel to EndLabel.
if (Asm->MF->hasBBSections() && StartLabel == Asm->getFunctionBegin() &&
!Instr->getParent()->sameSection(&Asm->MF->front())) {
const MCSymbol *BeginSectionLabel = StartLabel;

for (const MachineBasicBlock &MBB : *Asm->MF) {
if (MBB.isBeginSection() && &MBB != &Asm->MF->front())
BeginSectionLabel = MBB.getSymbol();

if (MBB.sameSection(Instr->getParent())) {
DebugLoc.emplace_back(BeginSectionLabel, EndLabel, Values);
for (const auto &[MBBSectionId, MBBSectionRange] :
Asm->MBBSectionRanges) {
if (Instr->getParent()->getSectionID() == MBBSectionId) {
DebugLoc.emplace_back(MBBSectionRange.BeginLabel, EndLabel, Values);
break;
}
if (MBB.isEndSection())
DebugLoc.emplace_back(BeginSectionLabel, MBB.getEndSymbol(), Values);
DebugLoc.emplace_back(MBBSectionRange.BeginLabel,
MBBSectionRange.EndLabel, Values);
}
} else {
DebugLoc.emplace_back(StartLabel, EndLabel, Values);
Expand Down Expand Up @@ -1824,22 +1821,27 @@ bool DwarfDebug::buildLocationList(SmallVectorImpl<DebugLocEntry> &DebugLoc,
RangeMBB = &Asm->MF->front();
else
RangeMBB = Entries.begin()->getInstr()->getParent();
auto RangeIt = Asm->MBBSectionRanges.find(RangeMBB->getSectionID());
assert(RangeIt != Asm->MBBSectionRanges.end() &&
"Range MBB not found in MBBSectionRanges!");
auto *CurEntry = DebugLoc.begin();
auto *NextEntry = std::next(CurEntry);
auto NextRangeIt = std::next(RangeIt);
while (NextEntry != DebugLoc.end()) {
// Get the last machine basic block of this section.
while (!RangeMBB->isEndSection())
RangeMBB = RangeMBB->getNextNode();
if (!RangeMBB->getNextNode())
if (NextRangeIt == Asm->MBBSectionRanges.end())
return false;
// CurEntry should end the current section and NextEntry should start
// the next section and the Values must match for these two ranges to be
// merged.
if (CurEntry->getEndSym() != RangeMBB->getEndSymbol() ||
NextEntry->getBeginSym() != RangeMBB->getNextNode()->getSymbol() ||
// merged. Do not match the section label end if it is the entry block
// section. This is because the end label for the Debug Loc and the
// Function end label could be different.
if ((RangeIt->second.EndLabel != Asm->getFunctionEnd() &&
CurEntry->getEndSym() != RangeIt->second.EndLabel) ||
NextEntry->getBeginSym() != NextRangeIt->second.BeginLabel ||
Comment on lines +1835 to +1840
Copy link
Member

Choose a reason for hiding this comment

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

Stupid question perhaps but: the comment talks about not matching stuff in the entry block, but the new comparison is against getFunctionEnd() which I anticipate is the exit block. Shouldn't that be fetching the MCLabel for the end-of-the-entry block instead?

(There's some overlap between the current "Entry" and the "entry" block, so I might have confused myself)

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, MBBSectionRanges stores the ranges for each section. For the entry section, the end label is the Function End Label, Asm->getFunctionEnd(). The entry section is actually the original function section.

Now, take a look at this assembly:

	.section	.text._Z4testv,"ax",@progbits
	.globl	_Z4testv                        # -- Begin function _Z4testv
	.type	_Z4testv,@function
_Z4testv:                               # @_Z4testv
.Lfunc_begin0:
	.loc	0 3 0                           # ex4.cc:3:0
# %bb.0:                                # %entry
	pushq	%rax
.Ltmp0:
	#DEBUG_VALUE: test:i <- 7
	.loc	0 7 5 prologue_end              # ex4.cc:7:5
	callq	_Z2f1v@PLT
        ...
.LBB_END0_0:
	.cfi_endproc
	.section	.text._Z4testv,"ax",@progbits,unique,1
_Z4testv.__part.1:                      # %if.then
	.section	.text._Z4testv,"ax",@progbits
.Lfunc_end0:

The entry block section uses label ".Lfunc_end0" which is Asm->getFunctionEnd(). There is an additional label that is created immediately after the end of the entry section ".LBB_END0_0" which is what the debug loc uses, like an alias. In this case and this case alone, these two labels won't match even though semantically, they are the same. So, I exclude that match.

CurEntry and entry block are different things. Sorry for the use of the overloaded entry.

Copy link
Member

Choose a reason for hiding this comment

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

Cool -- awkward terminology, but I can see what's going on.

CurEntry->getValues() != NextEntry->getValues())
return false;
RangeMBB = RangeMBB->getNextNode();
RangeIt = NextRangeIt;
NextRangeIt = std::next(RangeIt);
CurEntry = NextEntry;
NextEntry = std::next(CurEntry);
}
Expand Down
92 changes: 92 additions & 0 deletions llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-6.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=4 --basic-block-sections=none -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=4 --basic-block-sections=all -filetype=obj -o - | llvm-dwarfdump - | FileCheck --check-prefix=SECTIONS %s
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=5 --basic-block-sections=none -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=5 --basic-block-sections=all -filetype=obj -o - | llvm-dwarfdump - | FileCheck --check-prefix=SECTIONS %s

; CHECK: DW_TAG_variable
; CHECK-NEXT: DW_AT_location
; CHECK-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
; CHECK-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +8, DW_OP_stack_value
; CHECK-NEXT: DW_AT_name ("i")

; SECTIONS: DW_TAG_variable
; SECTIONS-NEXT: DW_AT_location
; SECTIONS-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
; SECTIONS-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +8, DW_OP_stack_value
; SECTIONS-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +8, DW_OP_stack_value
; SECTIONS-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +8, DW_OP_stack_value
; SECTIONS-NEXT: DW_AT_name ("i")

; Source to generate the IR below:
; void f1();
; extern bool b;
; void test() {
; // i is not a const throughout the whole scope and should
; // not use DW_AT_const_value
; int i = 7;
; f1();
; i = 8;
; if (b)
; f1();
; }
; $ clang++ -S loclist_section.cc -O2 -g -emit-llvm

@b = external local_unnamed_addr global i8, align 1

; Function Attrs: mustprogress uwtable
define dso_local void @_Z4testv() local_unnamed_addr #0 !dbg !10 {
entry:
#dbg_value(i32 7, !14, !DIExpression(), !16)
tail call void @_Z2f1v(), !dbg !17
#dbg_value(i32 8, !14, !DIExpression(), !16)
%0 = load i8, ptr @b, align 1, !dbg !18, !tbaa !20, !range !24, !noundef !25
%loadedv = trunc nuw i8 %0 to i1, !dbg !18
br i1 %loadedv, label %if.then, label %if.end, !dbg !26

if.then: ; preds = %entry
tail call void @_Z2f1v(), !dbg !27
br label %if.end, !dbg !27

if.end: ; preds = %if.then, %entry
ret void, !dbg !28
}

declare !dbg !29 void @_Z2f1v() local_unnamed_addr #1

attributes #0 = { mustprogress 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" }

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

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 20.0.0git ([email protected]:)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "loclist_section.cc", directory: "Examples/debug_loc", checksumkind: CSK_MD5, checksum: "67769a94389681c8a6da481e2f358abb")
!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 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
!9 = !{!"clang version 20.0.0git ([email protected]:.../llvm-project.git 7c3256280a78b0505ae4d43985c4d3239451a151)"}
!10 = distinct !DISubprogram(name: "test", linkageName: "_Z4testv", scope: !1, file: !1, line: 3, type: !11, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
!11 = !DISubroutineType(types: !12)
!12 = !{null}
!13 = !{!14}
!14 = !DILocalVariable(name: "i", scope: !10, file: !1, line: 6, type: !15)
!15 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!16 = !DILocation(line: 0, scope: !10)
!17 = !DILocation(line: 7, column: 5, scope: !10)
!18 = !DILocation(line: 9, column: 9, scope: !19)
!19 = distinct !DILexicalBlock(scope: !10, file: !1, line: 9, column: 9)
!20 = !{!21, !21, i64 0}
!21 = !{!"bool", !22, i64 0}
!22 = !{!"omnipotent char", !23, i64 0}
!23 = !{!"Simple C++ TBAA"}
!24 = !{i8 0, i8 2}
!25 = !{}
!26 = !DILocation(line: 9, column: 9, scope: !10)
!27 = !DILocation(line: 10, column: 7, scope: !19)
!28 = !DILocation(line: 11, column: 1, scope: !10)
!29 = !DISubprogram(name: "f1", linkageName: "_Z2f1v", scope: !1, file: !1, line: 1, type: !11, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
129 changes: 129 additions & 0 deletions llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-7.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=4 --basic-block-sections=none -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=4 --basic-block-sections=all -filetype=obj -o - | llvm-dwarfdump - | FileCheck --check-prefix=SECTIONS %s
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=5 --basic-block-sections=none -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
; RUN: llc %s -mtriple=x86_64-unknown-linux-gnu --dwarf-version=5 --basic-block-sections=all -filetype=obj -o - | llvm-dwarfdump - | FileCheck --check-prefix=SECTIONS %s

; CHECK: DW_TAG_lexical_block
; CHECK-NEXT: DW_AT_low_pc
; CHECK-NEXT: DW_AT_high_pc
; CHECK: DW_TAG_variable
; CHECK-NEXT: DW_AT_const_value (7)
; CHECK-NEXT: DW_AT_name ("i")

; SECTIONS: DW_TAG_lexical_block
; SECTIONS-NEXT: DW_AT_ranges
; SECTIONS: DW_TAG_variable
; SECTIONS-NEXT: DW_AT_const_value (7)
; SECTIONS-NEXT: DW_AT_name ("i")

; Test to check that a variable declared within a scope that has basic block
; sections still produces DW_AT_const_value.
; Source to generate the IR below:

; void f1(int *);
; extern bool b;
; int test() {
; // i is const throughout the whole scope and should
; // use DW_AT_const_value. The scope creates basic
; // block sections and should use DW_AT_ranges.
; int j = 10;
; {
; int i = 7;
; f1(&j);
; if (b)
; f1(&j);
; }
; return j;
; }
;
; clang++ -S scoped_section_const.cc -g -O2 -emit-llvm

@b = external local_unnamed_addr global i8, align 1

; Function Attrs: mustprogress uwtable
define dso_local noundef i32 @_Z4testv() local_unnamed_addr #0 !dbg !9 {
%1 = alloca i32, align 4
call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %1) #4, !dbg !17
call void @llvm.dbg.value(metadata i32 10, metadata !14, metadata !DIExpression()), !dbg !18
store i32 10, ptr %1, align 4, !dbg !19, !tbaa !20
call void @llvm.dbg.value(metadata i32 7, metadata !15, metadata !DIExpression()), !dbg !24
call void @llvm.dbg.value(metadata ptr %1, metadata !14, metadata !DIExpression(DW_OP_deref)), !dbg !18
call void @_Z2f1Pi(ptr noundef nonnull %1), !dbg !25
%2 = load i8, ptr @b, align 1, !dbg !26, !tbaa !28, !range !30, !noundef !31
%3 = icmp eq i8 %2, 0, !dbg !26
br i1 %3, label %5, label %4, !dbg !32

4: ; preds = %0
call void @llvm.dbg.value(metadata ptr %1, metadata !14, metadata !DIExpression(DW_OP_deref)), !dbg !18
call void @_Z2f1Pi(ptr noundef nonnull %1), !dbg !33
br label %5, !dbg !33

5: ; preds = %4, %0
%6 = load i32, ptr %1, align 4, !dbg !34, !tbaa !20
call void @llvm.dbg.value(metadata i32 %6, metadata !14, metadata !DIExpression()), !dbg !18
call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %1) #4, !dbg !35
ret i32 %6, !dbg !36
}

; Function Attrs: mustprogress nocallback nofree nosync nounwind willreturn memory(argmem: readwrite)
declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture) #1

declare !dbg !37 void @_Z2f1Pi(ptr noundef) local_unnamed_addr #2

; Function Attrs: mustprogress nocallback nofree nosync nounwind willreturn memory(argmem: readwrite)
declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture) #1

; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
declare void @llvm.dbg.value(metadata, metadata, metadata) #3

attributes #0 = { mustprogress uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { mustprogress nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
attributes #2 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #3 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
attributes #4 = { nounwind }

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

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "Debian clang version 16.0.6 (26)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "scoped_section_const.cc", directory: "", checksumkind: CSK_MD5, checksum: "0406492d2e2e38af35d9ea210ba1f24b")
!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 = !{!"Debian clang version 16.0.6 (26)"}
!9 = distinct !DISubprogram(name: "test", linkageName: "_Z4testv", scope: !1, file: !1, line: 3, type: !10, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
!10 = !DISubroutineType(types: !11)
!11 = !{!12}
!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!13 = !{!14, !15}
!14 = !DILocalVariable(name: "j", scope: !9, file: !1, line: 6, type: !12)
!15 = !DILocalVariable(name: "i", scope: !16, file: !1, line: 8, type: !12)
!16 = distinct !DILexicalBlock(scope: !9, file: !1, line: 7, column: 5)
!17 = !DILocation(line: 6, column: 5, scope: !9)
!18 = !DILocation(line: 0, scope: !9)
!19 = !DILocation(line: 6, column: 9, scope: !9)
!20 = !{!21, !21, i64 0}
!21 = !{!"int", !22, i64 0}
!22 = !{!"omnipotent char", !23, i64 0}
!23 = !{!"Simple C++ TBAA"}
!24 = !DILocation(line: 0, scope: !16)
!25 = !DILocation(line: 9, column: 7, scope: !16)
!26 = !DILocation(line: 10, column: 11, scope: !27)
!27 = distinct !DILexicalBlock(scope: !16, file: !1, line: 10, column: 11)
!28 = !{!29, !29, i64 0}
!29 = !{!"bool", !22, i64 0}
!30 = !{i8 0, i8 2}
!31 = !{}
!32 = !DILocation(line: 10, column: 11, scope: !16)
!33 = !DILocation(line: 11, column: 9, scope: !27)
!34 = !DILocation(line: 13, column: 12, scope: !9)
!35 = !DILocation(line: 14, column: 1, scope: !9)
!36 = !DILocation(line: 13, column: 5, scope: !9)
!37 = !DISubprogram(name: "f1", linkageName: "_Z2f1Pi", scope: !1, file: !1, line: 1, type: !38, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !31)
!38 = !DISubroutineType(types: !39)
!39 = !{null, !40}
!40 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !12, size: 64)
Loading
Loading