[DebugInfo][SimpleLoopUnswitch] Fix missing debug location updates #97662
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Shan Huang (Apochens) ChangesFix #97559 . For the change at line 1253, I propagate the debug location of the terminator (i.e., the insertion point) to the new phi. because For the change at line 2348, I switch the order of moving and cloning Full diff: https://github.com/llvm/llvm-project/pull/97662.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index fdb3211b4a438..caa1f7a156a3f 100644
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -1245,9 +1245,12 @@ static BasicBlock *buildClonedLoopBlocks(
if (SE && isa<PHINode>(I))
SE->forgetValue(&I);
+ Instruction *InsertPt = &*MergeBB->getFirstInsertionPt();
+
auto *MergePN =
PHINode::Create(I.getType(), /*NumReservedValues*/ 2, ".us-phi");
- MergePN->insertBefore(MergeBB->getFirstInsertionPt());
+ MergePN->insertBefore(InsertPt);
+ MergePN->setDebugLoc(InsertPt->getDebugLoc());
I.replaceAllUsesWith(MergePN);
MergePN->addIncoming(&I, ExitBB);
MergePN->addIncoming(&ClonedI, ClonedExitBB);
@@ -1306,8 +1309,9 @@ static BasicBlock *buildClonedLoopBlocks(
else if (auto *SI = dyn_cast<SwitchInst>(ClonedTerminator))
ClonedConditionToErase = SI->getCondition();
+ Instruction *BI = BranchInst::Create(ClonedSuccBB, ClonedParentBB);
+ BI->setDebugLoc(ClonedTerminator->getDebugLoc());
ClonedTerminator->eraseFromParent();
- BranchInst::Create(ClonedSuccBB, ClonedParentBB);
if (ClonedConditionToErase)
RecursivelyDeleteTriviallyDeadInstructions(ClonedConditionToErase, nullptr,
@@ -2334,14 +2338,15 @@ static void unswitchNontrivialInvariants(
// nuke the initial terminator placed in the split block.
SplitBB->getTerminator()->eraseFromParent();
if (FullUnswitch) {
- // Splice the terminator from the original loop and rewrite its
- // successors.
- TI.moveBefore(*SplitBB, SplitBB->end());
-
// Keep a clone of the terminator for MSSA updates.
Instruction *NewTI = TI.clone();
NewTI->insertInto(ParentBB, ParentBB->end());
+ // Splice the terminator from the original loop and rewrite its
+ // successors.
+ TI.moveBefore(*SplitBB, SplitBB->end());
+ TI.dropLocation();
+
// First wire up the moved terminator to the preheaders.
if (BI) {
BasicBlock *ClonedPH = ClonedPHs.begin()->second;
@@ -2349,6 +2354,9 @@ static void unswitchNontrivialInvariants(
BI->setSuccessor(1 - ClonedSucc, LoopPH);
Value *Cond = skipTrivialSelect(BI->getCondition());
if (InsertFreeze)
+ // We don't give any debug location to the new freeze, because the
+ // BI (`dyn_cast<BranchInst>(TI)`) is an in-loop instruction hoisted
+ // out of the loop.
Cond = new FreezeInst(Cond, Cond->getName() + ".fr", BI->getIterator());
BI->setCondition(Cond);
DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});
@@ -2432,12 +2440,13 @@ static void unswitchNontrivialInvariants(
DTUpdates.push_back({DominatorTree::Delete, ParentBB, SuccBB});
}
- // After MSSAU update, remove the cloned terminator instruction NewTI.
- ParentBB->getTerminator()->eraseFromParent();
-
// Create a new unconditional branch to the continuing block (as opposed to
// the one cloned).
- BranchInst::Create(RetainedSuccBB, ParentBB);
+ Instruction *NewBI = BranchInst::Create(RetainedSuccBB, ParentBB);
+ NewBI->setDebugLoc(NewTI->getDebugLoc());
+
+ // After MSSAU update, remove the cloned terminator instruction NewTI.
+ NewTI->eraseFromParent();
} else {
assert(BI && "Only branches have partial unswitching.");
assert(UnswitchedSuccBBs.size() == 1 &&
@@ -2710,6 +2719,7 @@ static BranchInst *turnSelectIntoBranch(SelectInst *SI, DominatorTree &DT,
PHINode::Create(SI->getType(), 2, "unswitched.select", SI->getIterator());
Phi->addIncoming(SI->getTrueValue(), ThenBB);
Phi->addIncoming(SI->getFalseValue(), HeadBB);
+ Phi->setDebugLoc(SI->getDebugLoc());
SI->replaceAllUsesWith(Phi);
SI->eraseFromParent();
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll b/llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll
new file mode 100644
index 0000000000000..bdb97ff220608
--- /dev/null
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll
@@ -0,0 +1,75 @@
+; RUN: opt -passes='simple-loop-unswitch<nontrivial>' -S < %s | FileCheck %s
+
+define i32 @basic(i32 %N, i1 %cond, i32 %select_input) !dbg !5 {
+; CHECK-LABEL: define i32 @basic(
+
+; Check that SimpleLoopUnswitch's unswitchNontrivialInvariants() drops the
+; debug location of the hoisted terminator and doesn't give any debug location
+; to the new freeze, since it's inserted in a hoist block.
+; Also check that in unswitchNontrivialInvariants(), the new br instruction
+; inherits the debug location of the old terminator in the same block.
+
+; CHECK: entry:
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]{{$}}
+; CHECK-NEXT: br i1 [[COND_FR]], label %[[ENTRY_SPLIT_US:.*]], label %[[ENTRY_SPLIT:.*]]{{$}}
+; CHECK: for.body.us:
+; CHECK-NEXT: br label %0, !dbg [[DBG13:![0-9]+]]
+
+; Check that in turnSelectIntoBranch(), the new phi inherits the debug
+; location of the old select instruction replaced.
+
+; CHECK: 1:
+; CHECK-NEXT: [[UNSWITCHED_SELECT_US:%.*]] = phi i32 [ [[SELECT_INPUT:%.*]], %0 ], !dbg [[DBG13]]
+
+; Check that in BuildClonedLoopBlocks(), the new phi inherits the debug
+; location of the instruction at the insertion point and the new br
+; instruction inherits the debug location of the old terminator.
+
+; CHECK: for.body:
+; CHECK-NEXT: br label %2, !dbg [[DBG13]]
+; CHECK: for.cond.cleanup:
+; CHECK: [[DOTUS_PHI:%.*]] = phi i32 [ [[RES_LCSSA:%.*]], %[[FOR_COND_CLEANUP_SPLIT:.*]] ], [ [[RES_LCSSA_US:%.*]], %[[FOR_COND_CLEANUP_SPLIT_US:.*]] ], !dbg [[DBG17:![0-9]+]]
+entry:
+ br label %for.cond, !dbg !8
+
+for.cond: ; preds = %for.body, %entry
+ %res = phi i32 [ 0, %entry ], [ %add, %for.body ], !dbg !9
+ %i = phi i32 [ 0, %entry ], [ %inc, %for.body ], !dbg !10
+ %cmp = icmp slt i32 %i, %N, !dbg !11
+ br i1 %cmp, label %for.body, label %for.cond.cleanup, !dbg !12
+
+for.body: ; preds = %for.cond
+ %cond1 = select i1 %cond, i32 %select_input, i32 42, !dbg !13
+ %add = add nuw nsw i32 %cond1, %res, !dbg !14
+ %inc = add nuw nsw i32 %i, 1, !dbg !15
+ br label %for.cond, !dbg !16
+
+for.cond.cleanup: ; preds = %for.cond
+ ret i32 %res, !dbg !17
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!2, !3}
+!llvm.module.flags = !{!4}
+
+; CHECK: [[DBG13]] = !DILocation(line: 6,
+; CHECK: [[DBG17]] = !DILocation(line: 10,
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "main.ll", directory: "/")
+!2 = !{i32 10}
+!3 = !{i32 0}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "basic", linkageName: "basic", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !DILocation(line: 1, column: 1, scope: !5)
+!9 = !DILocation(line: 2, column: 1, scope: !5)
+!10 = !DILocation(line: 3, column: 1, scope: !5)
+!11 = !DILocation(line: 4, column: 1, scope: !5)
+!12 = !DILocation(line: 5, column: 1, scope: !5)
+!13 = !DILocation(line: 6, column: 1, scope: !5)
+!14 = !DILocation(line: 7, column: 1, scope: !5)
+!15 = !DILocation(line: 8, column: 1, scope: !5)
+!16 = !DILocation(line: 9, column: 1, scope: !5)
+!17 = !DILocation(line: 10, column: 1, scope: !5)
|
|
@llvm/pr-subscribers-llvm-transforms Author: Shan Huang (Apochens) ChangesFix #97559 . For the change at line 1253, I propagate the debug location of the terminator (i.e., the insertion point) to the new phi. because For the change at line 2348, I switch the order of moving and cloning Full diff: https://github.com/llvm/llvm-project/pull/97662.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index fdb3211b4a438..caa1f7a156a3f 100644
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -1245,9 +1245,12 @@ static BasicBlock *buildClonedLoopBlocks(
if (SE && isa<PHINode>(I))
SE->forgetValue(&I);
+ Instruction *InsertPt = &*MergeBB->getFirstInsertionPt();
+
auto *MergePN =
PHINode::Create(I.getType(), /*NumReservedValues*/ 2, ".us-phi");
- MergePN->insertBefore(MergeBB->getFirstInsertionPt());
+ MergePN->insertBefore(InsertPt);
+ MergePN->setDebugLoc(InsertPt->getDebugLoc());
I.replaceAllUsesWith(MergePN);
MergePN->addIncoming(&I, ExitBB);
MergePN->addIncoming(&ClonedI, ClonedExitBB);
@@ -1306,8 +1309,9 @@ static BasicBlock *buildClonedLoopBlocks(
else if (auto *SI = dyn_cast<SwitchInst>(ClonedTerminator))
ClonedConditionToErase = SI->getCondition();
+ Instruction *BI = BranchInst::Create(ClonedSuccBB, ClonedParentBB);
+ BI->setDebugLoc(ClonedTerminator->getDebugLoc());
ClonedTerminator->eraseFromParent();
- BranchInst::Create(ClonedSuccBB, ClonedParentBB);
if (ClonedConditionToErase)
RecursivelyDeleteTriviallyDeadInstructions(ClonedConditionToErase, nullptr,
@@ -2334,14 +2338,15 @@ static void unswitchNontrivialInvariants(
// nuke the initial terminator placed in the split block.
SplitBB->getTerminator()->eraseFromParent();
if (FullUnswitch) {
- // Splice the terminator from the original loop and rewrite its
- // successors.
- TI.moveBefore(*SplitBB, SplitBB->end());
-
// Keep a clone of the terminator for MSSA updates.
Instruction *NewTI = TI.clone();
NewTI->insertInto(ParentBB, ParentBB->end());
+ // Splice the terminator from the original loop and rewrite its
+ // successors.
+ TI.moveBefore(*SplitBB, SplitBB->end());
+ TI.dropLocation();
+
// First wire up the moved terminator to the preheaders.
if (BI) {
BasicBlock *ClonedPH = ClonedPHs.begin()->second;
@@ -2349,6 +2354,9 @@ static void unswitchNontrivialInvariants(
BI->setSuccessor(1 - ClonedSucc, LoopPH);
Value *Cond = skipTrivialSelect(BI->getCondition());
if (InsertFreeze)
+ // We don't give any debug location to the new freeze, because the
+ // BI (`dyn_cast<BranchInst>(TI)`) is an in-loop instruction hoisted
+ // out of the loop.
Cond = new FreezeInst(Cond, Cond->getName() + ".fr", BI->getIterator());
BI->setCondition(Cond);
DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});
@@ -2432,12 +2440,13 @@ static void unswitchNontrivialInvariants(
DTUpdates.push_back({DominatorTree::Delete, ParentBB, SuccBB});
}
- // After MSSAU update, remove the cloned terminator instruction NewTI.
- ParentBB->getTerminator()->eraseFromParent();
-
// Create a new unconditional branch to the continuing block (as opposed to
// the one cloned).
- BranchInst::Create(RetainedSuccBB, ParentBB);
+ Instruction *NewBI = BranchInst::Create(RetainedSuccBB, ParentBB);
+ NewBI->setDebugLoc(NewTI->getDebugLoc());
+
+ // After MSSAU update, remove the cloned terminator instruction NewTI.
+ NewTI->eraseFromParent();
} else {
assert(BI && "Only branches have partial unswitching.");
assert(UnswitchedSuccBBs.size() == 1 &&
@@ -2710,6 +2719,7 @@ static BranchInst *turnSelectIntoBranch(SelectInst *SI, DominatorTree &DT,
PHINode::Create(SI->getType(), 2, "unswitched.select", SI->getIterator());
Phi->addIncoming(SI->getTrueValue(), ThenBB);
Phi->addIncoming(SI->getFalseValue(), HeadBB);
+ Phi->setDebugLoc(SI->getDebugLoc());
SI->replaceAllUsesWith(Phi);
SI->eraseFromParent();
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll b/llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll
new file mode 100644
index 0000000000000..bdb97ff220608
--- /dev/null
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll
@@ -0,0 +1,75 @@
+; RUN: opt -passes='simple-loop-unswitch<nontrivial>' -S < %s | FileCheck %s
+
+define i32 @basic(i32 %N, i1 %cond, i32 %select_input) !dbg !5 {
+; CHECK-LABEL: define i32 @basic(
+
+; Check that SimpleLoopUnswitch's unswitchNontrivialInvariants() drops the
+; debug location of the hoisted terminator and doesn't give any debug location
+; to the new freeze, since it's inserted in a hoist block.
+; Also check that in unswitchNontrivialInvariants(), the new br instruction
+; inherits the debug location of the old terminator in the same block.
+
+; CHECK: entry:
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]{{$}}
+; CHECK-NEXT: br i1 [[COND_FR]], label %[[ENTRY_SPLIT_US:.*]], label %[[ENTRY_SPLIT:.*]]{{$}}
+; CHECK: for.body.us:
+; CHECK-NEXT: br label %0, !dbg [[DBG13:![0-9]+]]
+
+; Check that in turnSelectIntoBranch(), the new phi inherits the debug
+; location of the old select instruction replaced.
+
+; CHECK: 1:
+; CHECK-NEXT: [[UNSWITCHED_SELECT_US:%.*]] = phi i32 [ [[SELECT_INPUT:%.*]], %0 ], !dbg [[DBG13]]
+
+; Check that in BuildClonedLoopBlocks(), the new phi inherits the debug
+; location of the instruction at the insertion point and the new br
+; instruction inherits the debug location of the old terminator.
+
+; CHECK: for.body:
+; CHECK-NEXT: br label %2, !dbg [[DBG13]]
+; CHECK: for.cond.cleanup:
+; CHECK: [[DOTUS_PHI:%.*]] = phi i32 [ [[RES_LCSSA:%.*]], %[[FOR_COND_CLEANUP_SPLIT:.*]] ], [ [[RES_LCSSA_US:%.*]], %[[FOR_COND_CLEANUP_SPLIT_US:.*]] ], !dbg [[DBG17:![0-9]+]]
+entry:
+ br label %for.cond, !dbg !8
+
+for.cond: ; preds = %for.body, %entry
+ %res = phi i32 [ 0, %entry ], [ %add, %for.body ], !dbg !9
+ %i = phi i32 [ 0, %entry ], [ %inc, %for.body ], !dbg !10
+ %cmp = icmp slt i32 %i, %N, !dbg !11
+ br i1 %cmp, label %for.body, label %for.cond.cleanup, !dbg !12
+
+for.body: ; preds = %for.cond
+ %cond1 = select i1 %cond, i32 %select_input, i32 42, !dbg !13
+ %add = add nuw nsw i32 %cond1, %res, !dbg !14
+ %inc = add nuw nsw i32 %i, 1, !dbg !15
+ br label %for.cond, !dbg !16
+
+for.cond.cleanup: ; preds = %for.cond
+ ret i32 %res, !dbg !17
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!2, !3}
+!llvm.module.flags = !{!4}
+
+; CHECK: [[DBG13]] = !DILocation(line: 6,
+; CHECK: [[DBG17]] = !DILocation(line: 10,
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "main.ll", directory: "/")
+!2 = !{i32 10}
+!3 = !{i32 0}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "basic", linkageName: "basic", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !DILocation(line: 1, column: 1, scope: !5)
+!9 = !DILocation(line: 2, column: 1, scope: !5)
+!10 = !DILocation(line: 3, column: 1, scope: !5)
+!11 = !DILocation(line: 4, column: 1, scope: !5)
+!12 = !DILocation(line: 5, column: 1, scope: !5)
+!13 = !DILocation(line: 6, column: 1, scope: !5)
+!14 = !DILocation(line: 7, column: 1, scope: !5)
+!15 = !DILocation(line: 8, column: 1, scope: !5)
+!16 = !DILocation(line: 9, column: 1, scope: !5)
+!17 = !DILocation(line: 10, column: 1, scope: !5)
|
| @@ -1245,9 +1245,12 @@ static BasicBlock *buildClonedLoopBlocks( | |||
| if (SE && isa<PHINode>(I)) | |||
| SE->forgetValue(&I); | |||
|
|
|||
| Instruction *InsertPt = &*MergeBB->getFirstInsertionPt(); | |||
There was a problem hiding this comment.
Can we keep this as an iterator rather than dereference it? It has variable location implications in Instruction::insertBefore (more info here -TL;DR, there's an extra bit in iterators that determines whether the iterator points before this instruction only or additionally the debug records attached to it too).
There was a problem hiding this comment.
Oh, thanks for pointing out this. I'll fix it.
| // Splice the terminator from the original loop and rewrite its | ||
| // successors. | ||
| TI.moveBefore(*SplitBB, SplitBB->end()); | ||
| TI.dropLocation(); |
There was a problem hiding this comment.
Why move this code down from where it was before?
There was a problem hiding this comment.
TI is moved out of the loop and its debug location should be dropped. However, NewTI (the code moved up) which is cloned from TI should preserve TI's debugl location, because NewTI is inserted into the original place where TI is at. In addition, NewTI will be replaced by a new branch instruction, which should inherit the debug location of NewTI.
So, I move the code down after the clone to preserve the debug location so that NewTI can preserve the right debug location while the debug location of TI can be correctly dropped.
| // First wire up the moved terminator to the preheaders. | ||
| if (BI) { | ||
| BasicBlock *ClonedPH = ClonedPHs.begin()->second; | ||
| BI->setSuccessor(ClonedSucc, ClonedPH); | ||
| BI->setSuccessor(1 - ClonedSucc, LoopPH); | ||
| Value *Cond = skipTrivialSelect(BI->getCondition()); | ||
| if (InsertFreeze) | ||
| // We don't give any debug location to the new freeze, because the | ||
| // BI (`dyn_cast<BranchInst>(TI)`) is an in-loop instruction hoisted | ||
| // out of the loop. |
There was a problem hiding this comment.
nit: Please either add braces around this or hoist the comment out the if statement (see second para here).
|
|
||
| ; CHECK: entry: | ||
| ; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]{{$}} | ||
| ; CHECK-NEXT: br i1 [[COND_FR]], label %[[ENTRY_SPLIT_US:.*]], label %[[ENTRY_SPLIT:.*]]{{$}} |
There was a problem hiding this comment.
Glancing at the IR, the old behaviour (new cond branch gets uncond branch location) looks fine, but I think this change does make sense - just to make sure I'm reading this right, is the argument here that the conditional branch above is essentially a select from within the loop that's been hoisted out? And that in this case the new branch would get the select's source location, but because it has been hoisted out the loop we should actually drop it.
There was a problem hiding this comment.
This dropping is corresponding to the newly added update TI.dropLocation(). I traced the origin of TI:
(1) it is the argument of function unswitchNontrivialInvariants();
static void unswitchNontrivialInvariants(
Loop &L, Instruction &TI, ...)(2) The function unswitchNontrivialInvariants() is called in function unswitchBestCondition(), and the TI argument takes *Best.TI. Best is the result of the function call to findBestNonTrivialUnswitchCandidate(), which return the best candidate from UnswitchCandidates. And UnswitchCandidates are collected by calling the function collectUnswitchCandidates();
static bool unswitchBestCondition(...) {
...
collectUnswitchCandidates(UnswitchCandidates, ...);
...
NonTrivialUnswitchCandidate Best =
findBestNonTrivialUnswitchCandidate(UnswitchCandidates, ...);
...
-> unswitchNontrivialInvariants(L, *Best.TI, ... );
}(3) In function collectUnswitchCandidates(), instructions satisfying certain conditions will be added into UnswitchCandidates. According to the code, instructions added into UnswitchCandidates are all in the loop, including instructions in the header of the loop (the code it too long to paste it here, see L2912-2964).
To sum up, the conditional branch is essentially the hoisted instruction in the loop (it could be a select, a branch, or a switch according to the code).
Glancing at the IR, the old behaviour (new cond branch gets uncond branch location) looks fine
Acctually, the new cond branch did not get the location of uncond branch. Before the dropping, there is no location propagation from the uncond branch to the new cond branch. See the following snippets (godbolt). (Hope I understand the old branch right.)
entry:
br label %for.cond, !dbg !8
!8 = !DILocation(line: 1, column: 1, scope: !5)entry:
%cond.fr = freeze i1 %cond
br i1 %cond.fr, label %entry.split.us, label %entry.split, !dbg !8
!8 = !DILocation(line: 6, column: 1, scope: !5)There was a problem hiding this comment.
Thanks for digging in and confirming - this all makes sense and sounds right to me.
Just one thing:
Actually, the new cond branch did not get the location of uncond branch. Before the dropping, there is no location propagation from the uncond branch to the new cond branch. See the following snippets (godbolt). (Hope I understand the old branch right.)
In the two snippets you posted at the end of the last comment it looks like the unconditional branch's location !8 (br label %for.cond, !dbg !8) gets propagated to the conditional branch br i1 %cond.fr, label %entry.split.us, label %entry.split, !dbg !8 which seems to oppose what you're saying - I must've missed something here, are we talking about a different unconditional branch?
There was a problem hiding this comment.
Oh, I think we are talking about the same unconditional branch and I did not state it clearly. The conditional branch in the second code snippet just has the same number of !dbg with the unconditional branch in the first code snippet, i.e., !dbg !8. However, !dbg !8 in the first snippet points to line 1 while it points to line 6 in the second snippet. So, the debug location of the unconditional branch did not get propagated to the conditional branch (they just have the same metadata number).
There was a problem hiding this comment.
Oh that makes sense, thanks for the clarification.
| ; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]{{$}} | ||
| ; CHECK-NEXT: br i1 [[COND_FR]], label %[[ENTRY_SPLIT_US:.*]], label %[[ENTRY_SPLIT:.*]]{{$}} | ||
| ; CHECK: for.body.us: | ||
| ; CHECK-NEXT: br label %0, !dbg [[DBG13:![0-9]+]] |
There was a problem hiding this comment.
I'm not 100% what's the correct thing to do for this one - if I'm reading it right this source location is from the select. What's the thinking behind you change for this one?
There was a problem hiding this comment.
This debug location comes from the debug location update:
Instruction *BI = BranchInst::Create(ClonedSuccBB, ClonedParentBB);
BI->setDebugLoc(ClonedTerminator->getDebugLoc());
ClonedTerminator->eraseFromParent();In the update, BI points the new br instruction. So, the debug location of select is propagated from ClonedTerminator to BI. To illustrate the instruction that ClonedTerminator points to, I paste the IR program before the remove of ClonedTerminator in the above code:
define i32 @basic(i32 %N, i1 %cond, i32 %select_input) !dbg !5 {
entry:
br label %entry.split, !dbg !8
entry.split.us: ; No predecessors!
br label %for.cond.us, !dbg !8
for.cond.us: ; preds = %1, %entry.split.us
%res.us = phi i32 [ 0, %entry.split.us ], [ %add.us, %1 ], !dbg !9
%i.us = phi i32 [ 0, %entry.split.us ], [ %inc.us, %1 ], !dbg !10
%cmp.us = icmp slt i32 %i.us, %N, !dbg !11
br i1 %cmp.us, label %for.body.us, label %for.cond.cleanup.split.us, !dbg !12
for.body.us: ; preds = %for.cond.us
->br i1 %cond, label %0, label %1, !dbg !13 (ClonedTerminator)
br label %0, !dbg !13. (BI)
0: ; preds = %for.body.us, %for.body.us
br label %1, !dbg !13
1: ; preds = %0, %for.body.us
%unswitched.select.us = phi i32 [ %select_input, %0 ], !dbg !13
%add.us = add nuw nsw i32 %unswitched.select.us, %res.us, !dbg !14
%inc.us = add nuw nsw i32 %i.us, 1, !dbg !15
br label %for.cond.us, !dbg !16
for.cond.cleanup.split.us: ; preds = %for.cond.us
...
}I just propagate the debug location of the instruction pointed by ClonedTerminator to BI, and the reason why ClonedTerminator has the debug location of the select could be resulted by spliting the cloned for body (as the contents of block %1 is actually the contents of %for.body).
There was a problem hiding this comment.
I see, thanks, yep that sounds ok.
|
|
||
| ; CHECK: entry: | ||
| ; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]{{$}} | ||
| ; CHECK-NEXT: br i1 [[COND_FR]], label %[[ENTRY_SPLIT_US:.*]], label %[[ENTRY_SPLIT:.*]]{{$}} |
There was a problem hiding this comment.
Oh that makes sense, thanks for the clarification.
| ; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]{{$}} | ||
| ; CHECK-NEXT: br i1 [[COND_FR]], label %[[ENTRY_SPLIT_US:.*]], label %[[ENTRY_SPLIT:.*]]{{$}} | ||
| ; CHECK: for.body.us: | ||
| ; CHECK-NEXT: br label %0, !dbg [[DBG13:![0-9]+]] |
There was a problem hiding this comment.
I see, thanks, yep that sounds ok.
Fix #97559 .
For the change at line 1253, I propagate the debug location of the terminator (i.e., the insertion point) to the new phi. because
MergeBBis generated by splittingExitBBseveral lines above, it only has the terminator, which could provide a reasonable debug location.For the change at line 2348, I switch the order of moving and cloning
TI. BecauseNewTIcloned fromTIis inserted into the original place whereTIis,NewTIshould preserve the origianl debug location. At the same time, doing this allows us to propagate the debug location to the new branch instruction replacingNewTI(the change at line 2446).