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

Apochens · 2024-07-14T02:46:49Z

Fix #98787 .

llvmbot · 2024-07-14T02:47:07Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

Fix #98787 .

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp (+11-5)
(added) llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll (+98)

diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index fdb3211b4a438..a7e63fd8f7d8a 100644
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -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);
@@ -667,9 +668,11 @@ static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT,
   if (FullUnswitch) {
     if (MSSAU) {
       // Remove the cloned branch instruction.
-      ParentBB->getTerminator()->eraseFromParent();
+      Instruction *Term = ParentBB->getTerminator();
+      Term->eraseFromParent();
       // Create unconditional branch now.
-      BranchInst::Create(ContinueBB, ParentBB);
+      Instruction *NewBI = BranchInst::Create(ContinueBB, ParentBB);
+      NewBI->setDebugLoc(Term->getDebugLoc());
       MSSAU->removeEdge(ParentBB, LoopExitBB);
     }
     DT.deleteEdge(ParentBB, LoopExitBB);
@@ -859,10 +862,12 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
   // the switch.
   BasicBlock *OldPH = L.getLoopPreheader();
   BasicBlock *NewPH = SplitEdge(OldPH, L.getHeader(), &DT, &LI, MSSAU);
-  OldPH->getTerminator()->eraseFromParent();
+  Instruction *Term = OldPH->getTerminator();
 
   // Now add the unswitched switch.
   auto *NewSI = SwitchInst::Create(LoopCond, NewPH, ExitCases.size(), OldPH);
+  NewSI->setDebugLoc(Term->getDebugLoc());
+  Term->eraseFromParent();
   SwitchInstProfUpdateWrapper NewSIW(*NewSI);
 
   // Rewrite the IR for the unswitched basic blocks. This requires two steps.
@@ -973,7 +978,8 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
     }
     // Now nuke the switch and replace it with a direct branch.
     SIW.eraseFromParent();
-    BranchInst::Create(CommonSuccBB, BB);
+    Instruction *NewBI = BranchInst::Create(CommonSuccBB, BB);
+    NewBI->setDebugLoc(SIW->getDebugLoc());
   } else if (DefaultExitBB) {
     assert(SI.getNumCases() > 0 &&
            "If we had no cases we'd have a common successor!");
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll b/llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll
new file mode 100644
index 0000000000000..3691d0f2eaafb
--- /dev/null
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll
@@ -0,0 +1,98 @@
+; 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]+]]
+; CHECK:       loop_begin:
+; CHECK-NEXT:    br label %[[LATCH:.*]], !dbg [[DBG17:![0-9]+]]
+;
+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: 8,
+; CHECK: [[DBG17]] = !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)
+
+

OCHyams

This is another one where the desirable behaviour isn't obvious to me - see my inline comment in the test.

The change tested in test1 seems unambiguously correct to me though.

OCHyams · 2024-07-15T08:55:01Z

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

@@ -973,7 +978,8 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
    }
    // Now nuke the switch and replace it with a direct branch.
    SIW.eraseFromParent();
-    BranchInst::Create(CommonSuccBB, BB);
+    Instruction *NewBI = BranchInst::Create(CommonSuccBB, BB);
+    NewBI->setDebugLoc(SIW->getDebugLoc());


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

OCHyams · 2024-07-15T09:58:25Z

llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll

+; 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]+]]


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.

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?

Yeah, I agree with @SLTozer .

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

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.

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!

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

SLTozer

DebugLoc updates LGTM, just needs a couple fixes noted inline.

SLTozer · 2024-07-15T09:57:41Z

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

@@ -667,9 +668,11 @@ static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT,
  if (FullUnswitch) {
    if (MSSAU) {
      // Remove the cloned branch instruction.
-      ParentBB->getTerminator()->eraseFromParent();
+      Instruction *Term = ParentBB->getTerminator();
+      Term->eraseFromParent();


I believe eraseFromParent frees the instruction, so we should copy the DebugLoc (either to the new instruction or as a local variable) before erasing it.

SLTozer · 2024-07-15T09:58:56Z

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

@@ -973,7 +978,8 @@ static bool unswitchTrivialSwitch(Loop &L, SwitchInst &SI, DominatorTree &DT,
    }
    // Now nuke the switch and replace it with a direct branch.
    SIW.eraseFromParent();
-    BranchInst::Create(CommonSuccBB, BB);
+    Instruction *NewBI = BranchInst::Create(CommonSuccBB, BB);
+    NewBI->setDebugLoc(SIW->getDebugLoc());


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

SLTozer · 2024-07-15T10:02:10Z

llvm/test/Transforms/SimpleLoopUnswitch/preserving-debugloc-trivial-terminators.ll

+!21 = !DILocation(line: 13, column: 1, scope: !15)
+!22 = !DILocation(line: 14, column: 1, scope: !15)
+!23 = !DILocation(line: 15, column: 1, scope: !15)
+


Suggested change

SLTozer · 2024-07-15T13:57:56Z

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

+      // Remove the cloned branch instruction and
+      // create unconditional branch now.


Suggested change

// Remove the cloned branch instruction and

// create unconditional branch now.

// Remove the cloned branch instruction and create an unconditional branch

// now.

Tiny nit.

…r new terminators (#98789) Summary: Fix #98787 . Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251388

Fix missing debug location updates

b66b49c

Apochens added loopoptim debuginfo llvm:transforms labels Jul 14, 2024

Apochens requested review from jryans, SLTozer and OCHyams July 14, 2024 02:46

OCHyams reviewed Jul 15, 2024

View reviewed changes

SLTozer reviewed Jul 15, 2024

View reviewed changes

nit

0e67bfa

SLTozer reviewed Jul 15, 2024

View reviewed changes

Apochens added 2 commits July 15, 2024 14:01

nit

e1f7eb1

Refine the update

028254a

SLTozer approved these changes Jul 15, 2024

View reviewed changes

Apochens merged commit 2e5b451 into llvm:main Jul 19, 2024
7 checks passed

Apochens deleted the #98787_simpleloopunswitch_preserving_debugloc_trivial_terminators branch July 19, 2024 05:17

		// Remove the cloned branch instruction and
		// create unconditional branch now.

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

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

Uh oh!

Conversation

Apochens commented Jul 14, 2024

Uh oh!

llvmbot commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SLTozer Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SLTozer Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SLTozer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 14, 2024 •

edited

Loading

SLTozer Jul 15, 2024 •

edited

Loading

SLTozer Jul 15, 2024 •

edited

Loading