Skip to content
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

Jump to bottom

[DebugInfo][SimpleLoopUnswitch] Fix missing debug location updates #97662

Merged
merged 2 commits into from
Jul 15, 2024
Merged

[DebugInfo][SimpleLoopUnswitch] Fix missing debug location updates #97662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74af7f4
fix missing debug location updates
Apochens Jul 4, 2024
b0f6c23
nit
Apochens Jul 11, 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
33 changes: 22 additions & 11 deletions llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1245,9 +1245,12 @@ static BasicBlock *buildClonedLoopBlocks(
if (SE && isa<PHINode>(I))
SE->forgetValue(&I);

BasicBlock::iterator 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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -2334,22 +2338,27 @@ 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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move this code down from where it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please either add braces around this or hoist the comment out the if statement (see second para here).

Cond = new FreezeInst(Cond, Cond->getName() + ".fr", BI->getIterator());
}
BI->setCondition(Cond);
DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});
} else {
Expand Down Expand Up @@ -2432,12 +2441,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 &&
Expand Down Expand Up @@ -2710,6 +2720,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();

Expand Down
75 changes: 75 additions & 0 deletions llvm/test/Transforms/SimpleLoopUnswitch/preserving-dropping-debugloc-nontrivial.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:.*]]{{$}}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Apochens Apochens Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that makes sense, thanks for the clarification.

; CHECK: for.body.us:
; CHECK-NEXT: br label %0, !dbg [[DBG13:![0-9]+]]
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks, yep that sounds ok.


; 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)
Loading