Skip to content

[DebugInfo][SimpleLoopUnswitch] Fix missing debug location updates for new terminators #98789

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
Jul 19, 2024
Merged

[DebugInfo][SimpleLoopUnswitch] Fix missing debug location updates for new terminators #98789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b66b49c
Fix missing debug location updates
Apochens Jul 14, 2024
0e67bfa
nit
Apochens Jul 15, 2024
e1f7eb1
nit
Apochens Jul 15, 2024
028254a
Refine the update
Apochens Jul 15, 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
21 changes: 14 additions & 7 deletions llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,8 @@ static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT,
} else {
// Create a new unconditional branch that will continue the loop as a new
// terminator.
BranchInst::Create(ContinueBB, ParentBB);
Instruction *NewBI = BranchInst::Create(ContinueBB, ParentBB);
NewBI->setDebugLoc(BI.getDebugLoc());
}
BI.setSuccessor(LoopExitSuccIdx, UnswitchedBB);
BI.setSuccessor(1 - LoopExitSuccIdx, NewPH);
Expand Down Expand Up @@ -666,10 +667,12 @@ static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT,
// Finish updating dominator tree and memory ssa for full unswitch.
if (FullUnswitch) {
if (MSSAU) {
// Remove the cloned branch instruction.
ParentBB->getTerminator()->eraseFromParent();
// Create unconditional branch now.
BranchInst::Create(ContinueBB, ParentBB);
Instruction *Term = ParentBB->getTerminator();
// Remove the cloned branch instruction and create unconditional branch
// now.
Instruction *NewBI = BranchInst::Create(ContinueBB, ParentBB);
NewBI->setDebugLoc(Term->getDebugLoc());
Term->eraseFromParent();
MSSAU->removeEdge(ParentBB, LoopExitBB);
}
DT.deleteEdge(ParentBB, LoopExitBB);
Expand Down Expand Up @@ -861,8 +864,11 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
BasicBlock *NewPH = SplitEdge(OldPH, L.getHeader(), &DT, &LI, MSSAU);
OldPH->getTerminator()->eraseFromParent();

// Now add the unswitched switch.
// Now add the unswitched switch. This new switch instruction inherits the
// debug location of the old switch, because it semantically replace the old
// one.
auto *NewSI = SwitchInst::Create(LoopCond, NewPH, ExitCases.size(), OldPH);
NewSI->setDebugLoc(SIW->getDebugLoc());
SwitchInstProfUpdateWrapper NewSIW(*NewSI);

// Rewrite the IR for the unswitched basic blocks. This requires two steps.
Expand Down Expand Up @@ -972,8 +978,9 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
/*KeepOneInputPHIs*/ true);
}
// Now nuke the switch and replace it with a direct branch.
Instruction *NewBI = BranchInst::Create(CommonSuccBB, BB);
NewBI->setDebugLoc(SIW->getDebugLoc());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to move SIW.eraseFromParent() after this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as above, SIW is deleted by eraseFromParent so we should copy its DebugLoc before erasing it.

SIW.eraseFromParent();
BranchInst::Create(CommonSuccBB, BB);
} else if (DefaultExitBB) {
assert(SI.getNumCases() > 0 &&
"If we had no cases we'd have a common successor!");
Expand Down
95 changes: 95 additions & 0 deletions llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
; RUN: opt -passes='loop(simple-loop-unswitch)' -S < %s | FileCheck %s
; RUN: opt -passes='loop-mssa(simple-loop-unswitch)' -S < %s | FileCheck %s

; Check that SimpleLoopUnswitch's unswitchTrivialBranch() and unswitchTrivialSwitch()
; propagates debug locations to the new terminators replacing the old ones.

define i32 @test1(ptr %var, i1 %cond1, i1 %cond2) !dbg !5 {
; CHECK-LABEL: define i32 @test1(
; CHECK: loop_begin:
; CHECK-NEXT: br label %[[CONTINUE:.*]], !dbg [[DBG8:![0-9]+]]
;
entry:
br label %loop_begin, !dbg !8

loop_begin: ; preds = %do_something, %entry
br i1 %cond1, label %continue, label %loop_exit, !dbg !9

continue: ; preds = %loop_begin
%var_val = load i32, ptr %var, align 4, !dbg !10
br i1 %cond2, label %do_something, label %loop_exit, !dbg !11

do_something: ; preds = %continue
call void @some_func(), !dbg !12
br label %loop_begin, !dbg !13

loop_exit: ; preds = %continue, %loop_begin
ret i32 0, !dbg !14
}

define i32 @test7(i32 %cond1, i32 %x, i32 %y) !dbg !15 {
; CHECK-LABEL: define i32 @test7(
; CHECK-SAME: i32 [[COND1:%.*]], i32 [[X:%.*]], i32 [[Y:%.*]])
; CHECK-NEXT: entry:
; CHECK-NEXT: switch i32 [[COND1]], label %[[ENTRY_SPLIT:.*]] [
; CHECK-NEXT: i32 0, label %[[LOOP_EXIT:.*]]
; CHECK-NEXT: i32 1, label %[[LOOP_EXIT]]
; CHECK-NEXT: ], !dbg [[DBG16:![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 think I'd like a second opinion on this one (I think this is similar to the previous loop unswitch question). AFAICT in this example the pass is rearranging the loop such that it is essentially hoisting this loop-invariant switch out the loop - this patch has the switch take the source location from the branch it replaces but I wonder if instead drop the source location (use dropLocation to create a line zero dbgloc).

cc @SLTozer / @jmorse

In HowToUpdateDebugInfo we have:

LICM. E.g., if an instruction is moved from the loop body to the preheader, the rule for dropping locations applies.

Copy link
Contributor

@SLTozer SLTozer Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, this initially looked fine to me, but you may be partially right. In this specific example, the debug loc does not need to be dropped, because the switch is in the loop header which is unconditionally branched to by the loop preheader, meaning there's no need to drop it. In the cases where the switch is not in the loop header however, dropping it would be necessary. The safest thing is to just drop it, but it would probably be better to detect the case where the switch is the loop condition (which is trivially known by the caller) and not drop it in those cases.

Edit: Actually, I think this may be suitable in all cases that this function is currently called - the caller for this function, unswitchAllTrivialConditions, ensures that there are no conditional branches between either the loop preheader pre-unswitching, or the last unswitched condition otherwise, which means that it should always be safe to preserve the debug loc... does that sound correct to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree with @SLTozer .

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that sounds reasonable to me thanks both.

One last angle to cover in order to ensure this is correct - I feel in that case there could be an argument that perhaps we should preserve the switch's location rather than inherit the replaced-branch's debugloc?

To be clear, I'm not arguing this should be the case - I just want to make sure we're evaluating all options thoroughly in order to avoid misleading debugging/pgo

Copy link
Contributor

@SLTozer SLTozer Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree actually - the transform here is:

// Before
    do {
        switch(cond) {
            case 0:
            case 1:
                goto exit_loop;
            default:
                some_func();
        }
    } while (true);
// After
    switch(cond) {
        case 0:
        case 1:
            goto exit_loop;
        default:
            do {
                some_func();
            } while (true);
    }

I think I agree, the "new" switch instruction should have the same debug loc of the old switch instruction; when we evaluate the condition, it should be on the switch line, not the do line.

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, the new switch actually implictly replaces the old one. It's appropriate to propagate the debug location from the old one to the new one. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM - once that's done I'm happy for @SLTozer to sign off on this one once he's happy, thanks.

; CHECK: loop_begin:
; CHECK-NEXT: br label %[[LATCH:.*]], !dbg [[DBG16]]
;
entry:
br label %loop_begin, !dbg !16

loop_begin: ; preds = %latch, %entry
switch i32 %cond1, label %latch [
i32 0, label %loop_exit
i32 1, label %loop_exit
], !dbg !17

latch: ; preds = %loop_begin
call void @some_func(), !dbg !18
br label %loop_begin, !dbg !19

loop_exit: ; preds = %loop_begin, %loop_begin
%result1 = phi i32 [ %x, %loop_begin ], [ %x, %loop_begin ], !dbg !20
%result2 = phi i32 [ %y, %loop_begin ], [ %y, %loop_begin ], !dbg !21
%result = add i32 %result1, %result2, !dbg !22
ret i32 %result, !dbg !23
}

; Function Attrs: noreturn
declare void @some_func()

!llvm.dbg.cu = !{!0}
!llvm.debugify = !{!2, !3}
!llvm.module.flags = !{!4}

; CHECK: [[DBG8]] = !DILocation(line: 2,

; CHECK: [[DBG16]] = !DILocation(line: 9,

!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
!1 = !DIFile(filename: "test2.ll", directory: "/")
!2 = !{i32 15}
!3 = !{i32 0}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = distinct !DISubprogram(name: "test1", linkageName: "test1", 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 = distinct !DISubprogram(name: "test7", linkageName: "test7", scope: null, file: !1, line: 8, type: !6, scopeLine: 8, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
!16 = !DILocation(line: 8, column: 1, scope: !15)
!17 = !DILocation(line: 9, column: 1, scope: !15)
!18 = !DILocation(line: 10, column: 1, scope: !15)
!19 = !DILocation(line: 11, column: 1, scope: !15)
!20 = !DILocation(line: 12, column: 1, scope: !15)
!21 = !DILocation(line: 13, column: 1, scope: !15)
!22 = !DILocation(line: 14, column: 1, scope: !15)
!23 = !DILocation(line: 15, column: 1, scope: !15)
Loading