Skip to content

[Debuginfo][TailCallElim] Fix #86262: drop the debug location of entry branch #86269

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 4 commits into from
Mar 28, 2024
Merged

[Debuginfo][TailCallElim] Fix #86262: drop the debug location of entry branch #86269

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15e8951
[Debuginfo][TailCallElim] Fix #86262: drop the debug location of entr…
Apochens Mar 22, 2024
63b07e6
remove redundant attrs
Apochens Mar 22, 2024
bf3ccfb
Update llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
Apochens Mar 22, 2024
cbc44b6
remove a redundant test
Apochens Mar 23, 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
4 changes: 3 additions & 1 deletion llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,9 @@ void TailRecursionEliminator::createTailRecurseLoopHeader(CallInst *CI) {
NewEntry->takeName(HeaderBB);
HeaderBB->setName("tailrecurse");
BranchInst *BI = BranchInst::Create(HeaderBB, NewEntry);
Copy link
Collaborator

Choose a reason for hiding this comment

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

BI is unused now (but referenced in the comment at line 515), leading to compiler warnings.

../lib/Transforms/Scalar/TailRecursionElimination.cpp:512:15: error: unused variable 'BI' [-Werror,-Wunused-variable]
  BranchInst *BI = BranchInst::Create(HeaderBB, NewEntry);
              ^
1 error generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out this. I fix this in #86927.

BI->setDebugLoc(CI->getDebugLoc());
// If the new branch preserves the debug location of CI, it could result in
// misleading stepping, if CI is located in a conditional branch.
// So, here we don't give any debug location to BI.

// Move all fixed sized allocas from HeaderBB to NewEntry.
for (BasicBlock::iterator OEBI = HeaderBB->begin(), E = HeaderBB->end(),
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/Transforms/TailCallElim/debugloc.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
define void @foo() {
entry:
; CHECK-LABEL: entry:
; CHECK: br label %tailrecurse, !dbg ![[DbgLoc:[0-9]+]]
; CHECK: br label %tailrecurse{{$}}

call void @foo() ;; line 1
ret void

; CHECK-LABEL: tailrecurse:
; CHECK: br label %tailrecurse, !dbg ![[DbgLoc]]
; CHECK: br label %tailrecurse, !dbg ![[DbgLoc:[0-9]+]]
}

;; Make sure tailrecurse has the call instruction's DL
Expand Down
57 changes: 57 additions & 0 deletions llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
; RUN: opt -S -passes=tailcallelim < %s | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing, it'd be good to add a comment describing what this is testing for, particularly since the CHECK lines are fairly bare. Related to that, does this test add any coverage that the previous test (debugloc.ll) doesn't cover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case indeed adds coverage (related to the creation of the recursion accumulator). However, the purpose of the tests is to check that the new entry branch has no debugloc. For this, the previous test is enough. I'll delete the newly added test.


define dso_local i32 @func(i32 noundef %a) !dbg !10 {
; CHECK: entry
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; CHECK: entry
; CHECK: entry:

; CHECK: br label %tailrecurse{{$}}
entry:
tail call void @llvm.dbg.value(metadata i32 %a, metadata !15, metadata !DIExpression()), !dbg !16
%cmp = icmp sgt i32 %a, 1, !dbg !17
br i1 %cmp, label %if.then, label %if.end, !dbg !19

if.then: ; preds = %entry
%sub = sub nsw i32 %a, 1, !dbg !20
%call = call i32 @func(i32 noundef %sub), !dbg !22
%mul = mul nsw i32 %a, %call, !dbg !23
br label %return, !dbg !24

if.end: ; preds = %entry
br label %return, !dbg !25

return: ; preds = %if.end, %if.then
%retval.0 = phi i32 [ %mul, %if.then ], [ 1, %if.end ], !dbg !16
ret i32 %retval.0, !dbg !26
}

declare void @llvm.dbg.declare(metadata, metadata, metadata)

declare void @llvm.dbg.value(metadata, metadata, metadata)

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

!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 19.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "main.c", directory: "/root/llvm-test/TailCallElim")
!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, !"frame-pointer", i32 2}
!10 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !14)
!11 = !DISubroutineType(types: !12)
!12 = !{!13, !13}
!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!14 = !{}
!15 = !DILocalVariable(name: "a", arg: 1, scope: !10, file: !1, line: 1, type: !13)
!16 = !DILocation(line: 0, scope: !10)
!17 = !DILocation(line: 2, column: 11, scope: !18)
!18 = distinct !DILexicalBlock(scope: !10, file: !1, line: 2, column: 9)
!19 = !DILocation(line: 2, column: 9, scope: !10)
!20 = !DILocation(line: 3, column: 27, scope: !21)
!21 = distinct !DILexicalBlock(scope: !18, file: !1, line: 2, column: 16)
!22 = !DILocation(line: 3, column: 20, scope: !21)
!23 = !DILocation(line: 3, column: 18, scope: !21)
!24 = !DILocation(line: 3, column: 9, scope: !21)
!25 = !DILocation(line: 5, column: 5, scope: !10)
!26 = !DILocation(line: 6, column: 1, scope: !10)