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

Apochens · 2024-03-22T11:09:38Z

This pr fixes #86262.

…entry branch

llvmbot · 2024-03-22T11:10:07Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

…y branch

Full diff: https://github.com/llvm/llvm-project/pull/86269.diff

3 Files Affected:

(modified) llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp (+5-1)
(modified) llvm/test/Transforms/TailCallElim/debugloc.ll (+2-2)
(added) llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll (+60)

diff --git a/llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp b/llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
index 519ff3221a3bc3..e4373fbf431ebd 100644
--- a/llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
@@ -510,7 +510,11 @@ void TailRecursionEliminator::createTailRecurseLoopHeader(CallInst *CI) {
   NewEntry->takeName(HeaderBB);
   HeaderBB->setName("tailrecurse");
   BranchInst *BI = BranchInst::Create(HeaderBB, NewEntry);
-  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, and use dropLocation()
+  // to explicitly present this dicision.
+  BI->dropLocation();
 
   // Move all fixed sized allocas from HeaderBB to NewEntry.
   for (BasicBlock::iterator OEBI = HeaderBB->begin(), E = HeaderBB->end(),
diff --git a/llvm/test/Transforms/TailCallElim/debugloc.ll b/llvm/test/Transforms/TailCallElim/debugloc.ll
index 3abbd6552efce3..49957695a421b9 100644
--- a/llvm/test/Transforms/TailCallElim/debugloc.ll
+++ b/llvm/test/Transforms/TailCallElim/debugloc.ll
@@ -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
diff --git a/llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll b/llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll
new file mode 100644
index 00000000000000..5e105537908df1
--- /dev/null
+++ b/llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll
@@ -0,0 +1,60 @@
+; RUN: opt -S -passes=tailcallelim < %s | FileCheck %s
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local i32 @func(i32 noundef %a) #0 !dbg !10 {
+; 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
+}
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+!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)

Apochens · 2024-03-22T11:13:34Z

@jmorse @SLTozer @dwblaikie Could you please review this?

SLTozer · 2024-03-22T11:48:00Z

For requesting reviews, you should add the prospective reviewers to the "Reviewers" list in the PR; @ mentions are better for one-off questions where you need a specific person's reply, and won't add the issue to their "Review requested" list.

Apochens · 2024-03-22T11:53:23Z

Yeah, I agree that. But I don't have the permission to add reviewers in the PR.😞

SLTozer

Largely LGTM, one inline comment.

llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp

Simplify the fix Co-authored-by: Stephen Tozer <[email protected]>

SLTozer

A couple more inline comments about the test - if it turns out the test isn't needed it can be deleted, but if it's adding unique coverage then LGTM.

SLTozer · 2024-03-22T15:16:08Z

llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll

+; RUN: opt -S -passes=tailcallelim < %s | FileCheck %s
+
+define dso_local i32 @func(i32 noundef %a) !dbg !10 {
+; CHECK: entry


Suggested change

; CHECK: entry

; CHECK: entry:

SLTozer · 2024-03-22T15:16:44Z

llvm/test/Transforms/TailCallElim/entry-branch-drop-debugloc.ll

@@ -0,0 +1,57 @@
+; RUN: opt -S -passes=tailcallelim < %s | FileCheck %s


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?

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.

github-actions · 2024-03-23T01:55:26Z

✅ With the latest revision this PR passed the C/C++ code formatter.

github-actions · 2024-03-23T01:55:26Z

✅ With the latest revision this PR passed the Python code formatter.

Apochens · 2024-03-28T01:50:05Z

Hi! I would like to politely ask if this PR is ready to merge?

SLTozer · 2024-03-28T09:13:41Z

Hi! I would like to politely ask if this PR is ready to merge?

Yep, you have the green tick of approval!

Apochens · 2024-03-28T09:23:33Z

Yep, you have the green tick of approval!

But it seems that this PR has remained unmerged for a while🤔

mikaelholmen · 2024-03-28T09:52:36Z

llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp

@@ -510,7 +510,9 @@ void TailRecursionEliminator::createTailRecurseLoopHeader(CallInst *CI) {
  NewEntry->takeName(HeaderBB);
  HeaderBB->setName("tailrecurse");
  BranchInst *BI = BranchInst::Create(HeaderBB, NewEntry);


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.

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

Remove the unused variable `BI` introduced in #86269.

jryans · 2024-03-28T10:46:02Z

But it seems that this PR has remained unmerged for a while 🤔

LLVM PRs tend to wait a bit (sometimes a few days) even after the first approving review in case others may have feedback to add. There's no firm rule though. With simpler changes, it's typically fine to merge soon after the first approval.

Sometimes reviewers forget that new contributors (such as yourself) can't merge the PR themselves (when the PR author has commit access, the typical process is the PR author merges once there's approval). You may need to comment on PRs after approval to remind reviewers that you don't have commit access yourself, so they should merge for you.

jryans · 2024-03-28T10:47:53Z

Thanks for working on this! 😄

[Debuginfo][TailCallElim] Fix llvm#86262: drop the debug location of …

15e8951

…entry branch

llvmbot added the llvm:transforms label Mar 22, 2024

Apochens changed the title ~~[Debuginfo][TailCallElim] Fix #86262: drop the debug location of entr~~ [Debuginfo][TailCallElim] Fix #86262: drop the debug location of entry branch Mar 22, 2024

SLTozer requested review from jmorse, SLTozer and dwblaikie March 22, 2024 13:02

jryans added the debuginfo label Mar 22, 2024

remove redundant attrs

63b07e6

SLTozer reviewed Mar 22, 2024

View reviewed changes

llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp Outdated Show resolved Hide resolved

Update llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp

bf3ccfb

Simplify the fix Co-authored-by: Stephen Tozer <[email protected]>

SLTozer reviewed Mar 22, 2024

View reviewed changes

remove a redundant test

cbc44b6

SLTozer approved these changes Mar 25, 2024

View reviewed changes

dtcxzyw merged commit 912e2c4 into llvm:main Mar 28, 2024

mikaelholmen reviewed Mar 28, 2024

View reviewed changes

Apochens deleted the #86262_drop_debugloc_of_entry_branch branch March 28, 2024 10:02

Apochens mentioned this pull request Mar 28, 2024

Fix #86269: remove unused variable #86927

Merged

mikaelholmen pushed a commit that referenced this pull request Mar 28, 2024

Fix #86269: remove unused variable (#86927)

8963a47

Remove the unused variable `BI` introduced in #86269.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Apochens commented Mar 22, 2024 •

edited

Loading

llvmbot commented Mar 22, 2024 •

edited

Loading

Apochens commented Mar 22, 2024

SLTozer commented Mar 22, 2024

Apochens commented Mar 22, 2024

SLTozer left a comment

SLTozer left a comment

SLTozer Mar 22, 2024

SLTozer Mar 22, 2024

Apochens Mar 23, 2024

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

Apochens commented Mar 28, 2024

SLTozer commented Mar 28, 2024

Apochens commented Mar 28, 2024

mikaelholmen Mar 28, 2024

Apochens Mar 28, 2024

jryans commented Mar 28, 2024

jryans commented Mar 28, 2024

		@@ -0,0 +1,57 @@
		; RUN: opt -S -passes=tailcallelim < %s \| FileCheck %s

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

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

Conversation

Apochens commented Mar 22, 2024 • edited Loading

llvmbot commented Mar 22, 2024 • edited Loading

Apochens commented Mar 22, 2024

SLTozer commented Mar 22, 2024

Apochens commented Mar 22, 2024

SLTozer left a comment

Choose a reason for hiding this comment

SLTozer left a comment

Choose a reason for hiding this comment

SLTozer Mar 22, 2024

Choose a reason for hiding this comment

SLTozer Mar 22, 2024

Choose a reason for hiding this comment

Apochens Mar 23, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

Apochens commented Mar 28, 2024

SLTozer commented Mar 28, 2024

Apochens commented Mar 28, 2024

mikaelholmen Mar 28, 2024

Choose a reason for hiding this comment

Apochens Mar 28, 2024

Choose a reason for hiding this comment

jryans commented Mar 28, 2024

jryans commented Mar 28, 2024

Apochens commented Mar 22, 2024 •

edited

Loading

llvmbot commented Mar 22, 2024 •

edited

Loading