[BranchFolder] Fix missing debug info with tail merging #94715
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This breaks I'm still trying to figure out what exactly is going on here - it seems like the existing test is missing debug info, but there's also some weirdness with basic blocks being added... |
alanzhao1
force-pushed
the
bugfix/missing-dbg-loc
branch
from
622bcf9 to
5cd3c88
Compare
alanzhao1
changed the title
Test has been updated. |
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-backend-x86 Author: Alan Zhao (alanzhao1) Changes
Fixes #94050 Full diff: https://github.com/llvm/llvm-project/pull/94715.diff 4 Files Affected:
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index 55aa1d438b2a6..c6c48cfc320c9 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -455,12 +455,14 @@ static unsigned EstimateRuntime(MachineBasicBlock::iterator I,
// with a conditional branch to the next block, optimize by reversing the
// test and conditionally branching to SuccMBB instead.
static void FixTail(MachineBasicBlock *CurMBB, MachineBasicBlock *SuccBB,
- const TargetInstrInfo *TII) {
+ const TargetInstrInfo *TII, const DebugLoc &BranchDL) {
MachineFunction *MF = CurMBB->getParent();
MachineFunction::iterator I = std::next(MachineFunction::iterator(CurMBB));
MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
SmallVector<MachineOperand, 4> Cond;
DebugLoc dl = CurMBB->findBranchDebugLoc();
+ if (!dl)
+ dl = BranchDL;
if (I != MF->end() && !TII->analyzeBranch(*CurMBB, TBB, FBB, Cond, true)) {
MachineBasicBlock *NextBB = &*I;
if (TBB == NextBB && !Cond.empty() && !FBB) {
@@ -686,7 +688,8 @@ unsigned BranchFolder::ComputeSameTails(unsigned CurHash,
void BranchFolder::RemoveBlocksWithHash(unsigned CurHash,
MachineBasicBlock *SuccBB,
- MachineBasicBlock *PredBB) {
+ MachineBasicBlock *PredBB,
+ const DebugLoc &BranchDL) {
MPIterator CurMPIter, B;
for (CurMPIter = std::prev(MergePotentials.end()),
B = MergePotentials.begin();
@@ -694,7 +697,7 @@ void BranchFolder::RemoveBlocksWithHash(unsigned CurHash,
// Put the unconditional branch back, if we need one.
MachineBasicBlock *CurMBB = CurMPIter->getBlock();
if (SuccBB && CurMBB != PredBB)
- FixTail(CurMBB, SuccBB, TII);
+ FixTail(CurMBB, SuccBB, TII, BranchDL);
if (CurMPIter == B)
break;
}
@@ -908,6 +911,7 @@ bool BranchFolder::TryTailMergeBlocks(MachineBasicBlock *SuccBB,
// Walk through equivalence sets looking for actual exact matches.
while (MergePotentials.size() > 1) {
unsigned CurHash = MergePotentials.back().getHash();
+ const DebugLoc &BranchDL = MergePotentials.back().getBranchDebugLoc();
// Build SameTails, identifying the set of blocks with this hash code
// and with the maximum number of instructions in common.
@@ -918,7 +922,7 @@ bool BranchFolder::TryTailMergeBlocks(MachineBasicBlock *SuccBB,
// If we didn't find any pair that has at least MinCommonTailLength
// instructions in common, remove all blocks with this hash code and retry.
if (SameTails.empty()) {
- RemoveBlocksWithHash(CurHash, SuccBB, PredBB);
+ RemoveBlocksWithHash(CurHash, SuccBB, PredBB, BranchDL);
continue;
}
@@ -965,7 +969,7 @@ bool BranchFolder::TryTailMergeBlocks(MachineBasicBlock *SuccBB,
// Split a block so that one does.
if (!CreateCommonTailOnlyBlock(PredBB, SuccBB,
maxCommonTailLength, commonTailIndex)) {
- RemoveBlocksWithHash(CurHash, SuccBB, PredBB);
+ RemoveBlocksWithHash(CurHash, SuccBB, PredBB, BranchDL);
continue;
}
}
@@ -1013,7 +1017,8 @@ bool BranchFolder::TailMergeBlocks(MachineFunction &MF) {
if (MergePotentials.size() == TailMergeThreshold)
break;
if (!TriedMerging.count(&MBB) && MBB.succ_empty())
- MergePotentials.push_back(MergePotentialsElt(HashEndOfMBB(MBB), &MBB));
+ MergePotentials.push_back(MergePotentialsElt(HashEndOfMBB(MBB), &MBB,
+ MBB.findBranchDebugLoc()));
}
// If this is a large problem, avoid visiting the same basic blocks
@@ -1115,8 +1120,8 @@ bool BranchFolder::TailMergeBlocks(MachineFunction &MF) {
}
// Remove the unconditional branch at the end, if any.
+ DebugLoc dl = PBB->findBranchDebugLoc();
if (TBB && (Cond.empty() || FBB)) {
- DebugLoc dl = PBB->findBranchDebugLoc();
TII->removeBranch(*PBB);
if (!Cond.empty())
// reinsert conditional branch only, for now
@@ -1124,7 +1129,8 @@ bool BranchFolder::TailMergeBlocks(MachineFunction &MF) {
NewCond, dl);
}
- MergePotentials.push_back(MergePotentialsElt(HashEndOfMBB(*PBB), PBB));
+ MergePotentials.push_back(
+ MergePotentialsElt(HashEndOfMBB(*PBB), PBB, dl));
}
}
@@ -1142,7 +1148,8 @@ bool BranchFolder::TailMergeBlocks(MachineFunction &MF) {
PredBB = &*std::prev(I); // this may have been changed in TryTailMergeBlocks
if (MergePotentials.size() == 1 &&
MergePotentials.begin()->getBlock() != PredBB)
- FixTail(MergePotentials.begin()->getBlock(), IBB, TII);
+ FixTail(MergePotentials.begin()->getBlock(), IBB, TII,
+ MergePotentials.begin()->getBranchDebugLoc());
}
return MadeChange;
diff --git a/llvm/lib/CodeGen/BranchFolding.h b/llvm/lib/CodeGen/BranchFolding.h
index 63b2ef04b21ba..ff2bbe06c0488 100644
--- a/llvm/lib/CodeGen/BranchFolding.h
+++ b/llvm/lib/CodeGen/BranchFolding.h
@@ -50,10 +50,11 @@ class TargetRegisterInfo;
class MergePotentialsElt {
unsigned Hash;
MachineBasicBlock *Block;
+ DebugLoc BranchDebugLoc;
public:
- MergePotentialsElt(unsigned h, MachineBasicBlock *b)
- : Hash(h), Block(b) {}
+ MergePotentialsElt(unsigned h, MachineBasicBlock *b, DebugLoc bdl)
+ : Hash(h), Block(b), BranchDebugLoc(std::move(bdl)) {}
unsigned getHash() const { return Hash; }
MachineBasicBlock *getBlock() const { return Block; }
@@ -62,6 +63,8 @@ class TargetRegisterInfo;
Block = MBB;
}
+ const DebugLoc &getBranchDebugLoc() { return BranchDebugLoc; }
+
bool operator<(const MergePotentialsElt &) const;
};
@@ -162,8 +165,9 @@ class TargetRegisterInfo;
/// Remove all blocks with hash CurHash from MergePotentials, restoring
/// branches at ends of blocks as appropriate.
- void RemoveBlocksWithHash(unsigned CurHash, MachineBasicBlock* SuccBB,
- MachineBasicBlock* PredBB);
+ void RemoveBlocksWithHash(unsigned CurHash, MachineBasicBlock *SuccBB,
+ MachineBasicBlock *PredBB,
+ const DebugLoc &BranchDL);
/// None of the blocks to be tail-merged consist only of the common tail.
/// Create a block that does by splitting one.
diff --git a/llvm/test/CodeGen/X86/tail-merging-preserve-debugloc.ll b/llvm/test/CodeGen/X86/tail-merging-preserve-debugloc.ll
new file mode 100644
index 0000000000000..d8124d8f5960a
--- /dev/null
+++ b/llvm/test/CodeGen/X86/tail-merging-preserve-debugloc.ll
@@ -0,0 +1,31 @@
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -enable-tail-merge=1 -stop-after=branch-folder | FileCheck %s
+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-grtev4-linux-gnu"
+
+define i32 @main(i1 %0) {
+entry:
+ br i1 %0, label %1, label %2
+
+1: ; preds = %entry
+ store i64 1, ptr null, align 1
+; CHECK: JMP_1 %bb.3, debug-location !3
+ br label %3, !dbg !3
+
+2: ; preds = %entry
+ store i64 0, ptr null, align 1
+ br label %3
+
+3: ; preds = %2, %1
+ ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, nameTableKind: None)
+!1 = !DIFile(filename: "foo.c", directory: "/tmp", checksumkind: CSK_MD5, checksum: "2d07c91bb9d9c2fa4eee31a1aeed20e3")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocation(line: 17, column: 3, scope: !4)
+!4 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 6, type: !5, scopeLine: 6, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!5 = !DISubroutineType(types: !6)
+!6 = !{}
diff --git a/llvm/test/DebugInfo/COFF/local-variables.ll b/llvm/test/DebugInfo/COFF/local-variables.ll
index 56ece6b794b4b..820f6bd8beaa4 100644
--- a/llvm/test/DebugInfo/COFF/local-variables.ll
+++ b/llvm/test/DebugInfo/COFF/local-variables.ll
@@ -45,6 +45,8 @@
; ASM: .cv_loc 1 1 5 3 # t.cpp:5:3
; ASM: callq capture
; ASM: leaq 40(%rsp), %rcx
+; ASM: [[end_inline_1:\.Ltmp.*]]:
+; ASM: .cv_loc 0 1 11 5 # t.cpp:11:5
; ASM: jmp .LBB0_3
; ASM: [[else_start:\.Ltmp.*]]:
; ASM: .LBB0_2: # %if.else
@@ -87,7 +89,7 @@
; ASM: .long 116 # TypeIndex
; ASM: .short 0 # Flags
; ASM: .asciz "v"
-; ASM: .cv_def_range [[inline_site1]] [[else_start]], frame_ptr_rel, 44
+; ASM: .cv_def_range [[inline_site1]] [[end_inline_1]], frame_ptr_rel, 44
; ASM: .short 4430 # Record kind: S_INLINESITE_END
; ASM: .short 4429 # Record kind: S_INLINESITE
; ASM: .short 4414 # Record kind: S_LOCAL
@@ -154,7 +156,7 @@
; OBJ: ChangeLineOffset: 1
; OBJ: ChangeCodeOffset: 0x14
; OBJ: ChangeCodeOffsetAndLineOffset: {CodeOffset: 0xD, LineOffset: 1}
-; OBJ: ChangeCodeLength: 0xC
+; OBJ: ChangeCodeLength: 0xA
; OBJ: ]
; OBJ: }
; OBJ: LocalSym {
@@ -168,7 +170,7 @@
; OBJ: LocalVariableAddrRange {
; OBJ: OffsetStart: .text+0x14
; OBJ: ISectStart: 0x0
-; OBJ: Range: 0x19
+; OBJ: Range: 0x17
; OBJ: }
; OBJ: }
; OBJ: InlineSiteEnd {
|
aeubanks
changed the title
|
seems reasonable to me, although hopefully @dwblaikie can comment on if the COFF test change looks reasonable |
jmorse
reviewed
Jun 14, 2024
There was a problem hiding this comment.
It looks like the changes to the COFF test file are because a jmp without a source location previously inherited an earlier source-location, but now gets it's own source location. That causes:
- A new label to appear in the assembly to help describe ranges,
- A variable's scope range to reduce slightly,
- A lexical scope (I think?) and another variable to reduce range slightly.
Which I think is consistent with what you're doing here, the jmp previously inherited being "inside" an inlined function, but now the source location is preserved it really belongs to the outer function.
aeubanks
reviewed
Jun 18, 2024
Done |
jmorse
reviewed
Jun 19, 2024
There was a problem hiding this comment.
Am I right in thinking that this is preserving the source-location on branch instructions that aren't merged but were manipulated during analysis? If so, LGTM.
It would be undesirable if we gave a source location to a branch that's been merged with other branches without using DILocation::getMergedLocation as it could cause an untrue line number to appear on a path that it shouldn't. I think (80% confidence) this patch shouldn't make that happen though.
`BranchFolder::TryTailMergeBlocks(...)` removes unconditional branch instructions and then recreates them. However, this process loses debug source location information from the previous branch instruction, even if tail merging doesn't change IR. This patch preserves the debug information from the removed instruction and inserts them into the recreated instruction. Fixes llvm#94050
alanzhao1
force-pushed
the
bugfix/missing-dbg-loc
branch
from
fdcc976 to
b141516
Compare
Yes, this should be the case. |
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
`BranchFolder::TryTailMergeBlocks(...)` removes unconditional branch instructions and then recreates them. However, this process loses debug source location information from the previous branch instruction, even if tail merging doesn't change IR. This patch preserves the debug information from the removed instruction and inserts them into the recreated instruction. Fixes llvm#94050
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BranchFolder::TryTailMergeBlocks(...)removes unconditional branch instructions and then recreates them. However, this process loses debug source location information from the previous branch instruction, even if tail merging doesn't change IR. This patch preserves the debug information from the removed instruction and inserts them into the recreated instruction.Fixes #94050