Revert "[BranchFolding] Kill common hoisted debug instructions"

[OCHyams]

Reverts #140091 due to crash (see comments for reproducer)

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Reverts llvm/llvm-project#140091 due to crash (see comments for reproducer)

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/BranchFolding.cpp (+6-39)
• (modified) llvm/test/DebugInfo/X86/branch-folder-dbg.mir (+14-20)

diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index c1e9a20328527..3b3e7a418feb5 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2083,54 +2083,22 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
   if (TBB == FBB) {
     MBB->splice(Loc, TBB, TBB->begin(), TIB);
   } else {
-    // Merge the debug locations, and hoist and kill the debug instructions from
-    // both branches. FIXME: We could probably try harder to preserve some debug
-    // instructions (but at least this isn't producing wrong locations).
-    MachineInstrBuilder MIRBuilder(*MBB->getParent(), Loc);
-    auto HoistAndKillDbgInstr = [MBB, Loc](MachineBasicBlock::iterator DI) {
-      assert(DI->isDebugInstr() && "Expected a debug instruction");
-      if (DI->isDebugRef()) {
-        const TargetInstrInfo *TII =
-            MBB->getParent()->getSubtarget().getInstrInfo();
-        const MCInstrDesc &DBGV = TII->get(TargetOpcode::DBG_VALUE);
-        DI = BuildMI(*MBB->getParent(), DI->getDebugLoc(), DBGV, false, 0,
-                     DI->getDebugVariable(), DI->getDebugExpression());
-        MBB->insert(Loc, &*DI);
-        return;
-      }
-
-      DI->setDebugValueUndef();
-      DI->moveBefore(&*Loc);
-    };
-
     // TIB and FIB point to the end of the regions to hoist/merge in TBB and
     // FBB.
     MachineBasicBlock::iterator FE = FIB;
     MachineBasicBlock::iterator FI = FBB->begin();
     for (MachineBasicBlock::iterator TI :
          make_early_inc_range(make_range(TBB->begin(), TIB))) {
-      // Hoist and kill debug instructions from FBB. After this loop FI points
-      // to the next non-debug instruction to hoist (checked in assert after the
-      // TBB debug instruction handling code).
-      while (FI->isDebugInstr()) {
-        assert(FI != FE && "Unexpected end of FBB range");
-        MachineBasicBlock::iterator FINext = std::next(FI);
-        HoistAndKillDbgInstr(FI);
-        FI = FINext;
-      }
-
-      // Kill debug instructions before moving.
-      if (TI->isDebugInstr()) {
-        HoistAndKillDbgInstr(TI);
+      // Move debug instructions and pseudo probes without modifying them.
+      // FIXME: This is the wrong thing to do for debug locations, which
+      // should at least be killed (and hoisted from BOTH blocks).
+      if (TI->isDebugOrPseudoInstr()) {
+        TI->moveBefore(&*Loc);
         continue;
       }
 
-      // If FI is a debug instruction, skip forward to the next non-debug
-      // instruction.
+      // Get the next non-meta instruction in FBB.
       FI = skipDebugInstructionsForward(FI, FE, false);
-      // Pseudo probes are excluded from the range when identifying foldable
-      // instructions, so we don't expect to see one now.
-      assert(!TI->isPseudoProbe() && "Unexpected pseudo probe in range");
       // NOTE: The loop above checks CheckKillDead but we can't do that here as
       // it modifies some kill markers after the check.
       assert(TI->isIdenticalTo(*FI, MachineInstr::CheckDefs) &&
@@ -2143,7 +2111,6 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
       ++FI;
     }
   }
-
   FBB->erase(FBB->begin(), FIB);
 
   if (UpdateLiveIns)
diff --git a/llvm/test/DebugInfo/X86/branch-folder-dbg.mir b/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
index 831b263fdccd4..5c38fd2a43829 100644
--- a/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
+++ b/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
@@ -1,21 +1,16 @@
 # RUN: llc %s --start-before=branch-folder --stop-after=branch-folder -o - | FileCheck %s
 
 ## Check that common instructions hoisted from `if.then` and `if.else` into
-## common pred `entry` get merged debug locations. The debug instructions from
-## both branches should get hoisted and killed.
-##
-## The MIR debug instructions have been modified by hand in order to check they
-## can be killed.
+## common pred `entry` get merged debug locations.
+
+## FIXME: The debug instructions handling here is wrong.
 
 # CHECK: bb.0
 # CHECK:      CALL64pcrel32 @f, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
-## --- Start splice from bb.2.if.else (and debug instructions from bb.1.if.then) ---
-# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(),  debug-location ![[#]]
-# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(),  debug-location ![[#]]
-# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 2, debug-location !DILocation(line: 0, scope: ![[#]])
-# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_arg, 0),  debug-location ![[#]]
-# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_arg, 0),  debug-location ![[#]]
-## --- End splice ------------------------------------------------------------------
+## --- Start splice from bb.2.if.else ---
+# CHECK-NEXT: DBG_VALUE 2, $noreg, ![[#]], !DIExpression(),  debug-location ![[#]]
+# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags,  debug-location !DILocation(line: 0, scope: ![[#]])
+## --- End splice --------------
 # CHECK-NEXT: TEST64rr killed renamable $rax, renamable $rax, implicit-def $eflags
 # CHECK-NEXT: JCC_1 %bb.2, 9, implicit killed $eflags
 # CHECK: bb.1
@@ -78,8 +73,6 @@
 ...
 ---
 name:            g
-tracksRegLiveness: true
-isSSA: false
 body:             |
   bb.0 (%ir-block.0):
     successors: %bb.1(0x40000000), %bb.2(0x40000000)
@@ -93,21 +86,22 @@ body:             |
 
   bb.1.if.then:
     successors: %bb.3(0x80000000)
-    DBG_VALUE $esi, $noreg, !11, !DIExpression(),  debug-location !13
-    $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 1, debug-location !14
-    DBG_INSTR_REF !11, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0),  debug-location !13
+
+    DBG_VALUE 0, $noreg, !11, !DIExpression(),  debug-location !13
+    $edi = MOV32r0 implicit-def dead $eflags,  debug-location !14
     $esi = MOV32r0 implicit-def dead $eflags,  debug-location !14
     CALL64pcrel32 target-flags(x86-plt) @_Z3fooii, csr_64, implicit $rsp, implicit $ssp, implicit killed $edi, implicit killed $esi, implicit-def $rsp, implicit-def $ssp,  debug-location !14
+    DBG_VALUE 1, $noreg, !11, !DIExpression(),  debug-location !13
     JMP_1 %bb.3,  debug-location !15
 
   bb.2.if.else:
     successors: %bb.3(0x80000000)
 
-    DBG_VALUE $esp, $noreg, !11, !DIExpression(), debug-location !13
-    $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 2, debug-location !16
-    DBG_INSTR_REF !11, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(2, 0),  debug-location !13
+    DBG_VALUE 2, $noreg, !11, !DIExpression(),  debug-location !13
+    $edi = MOV32r0 implicit-def dead $eflags,  debug-location !16
     $esi = MOV32ri 1,  debug-location !16
     CALL64pcrel32 target-flags(x86-plt) @_Z3barii, csr_64, implicit $rsp, implicit $ssp, implicit killed $edi, implicit killed $esi, implicit-def $rsp, implicit-def $ssp,  debug-location !16
+    DBG_VALUE 3, $noreg, !11, !DIExpression(),  debug-location !13
 
   bb.3.if.end:
     $eax = MOV32ri 2

[dwblaikie]

(please apply the skip-precommit-approval label when creating PRs without the intent for them to be reviewed before commit)

[OCHyams]

(please apply the skip-precommit-approval label when creating PRs without the intent for them to be reviewed before commit)

Ah, I didn't know about the label. Thanks!