[IfConversion] Fix diamond conversion with unanalyzable branches.

[efriedma]

Summary

The code was incorrectly counting the number of identical instructions, and therefore tried to predicate an instruction which should not have been predicated. This could have various effects: a compiler crash, an assembler failure, a miscompile, or just generating an extra, unnecessary instruction.

Instead of depending on TargetInstrInfo::removeBranch, which only works on analyzable branches, just remove all branch instructions.

Fixes https://bugs.llvm.org/show_bug.cgi?id=43121 and https://bugs.llvm.org/show_bug.cgi?id=41121 .

[dmgreen]

LGTM

[hans]

llvm/trunk/lib/CodeGen/IfConversion.cpp

1764 ↗

(On Diff #218968)

The previous code also decremented BBI1->NonPredSize. Do we not need to do that anymore?

(I don't know this code, just driving by.)

[efriedma]

llvm/trunk/lib/CodeGen/IfConversion.cpp

1764 ↗

(On Diff #218968)

In theory, we probably should decrement NonPredSize, for both this loop and the following loop, to account for the instructions we're erasing.

We only use the NonPredSize as input to the if-conversion heuristics, though. It's unlikely we're going to predicate a basic block that's already predicated. So whatever we do probably has very little, if any, effect.

lib/CodeGen/IfConversion.cpp

bool IfConverter::IfConvertDiamondCommon(
   // be marked as "branch". If it's not, then it could be included in
   // "dups1", leaving the blocks potentially empty after moving the common
   // duplicates.
 #ifndef NDEBUG
   // Unanalyzable branches must match exactly. Check that now.
   if (!BBI1->IsBrAnalyzable)
     verifySameBranchInstructions(&MBB1, &MBB2);
 #endif
-  BBI1->NonPredSize -= TII->removeBranch(*BBI1->BB);
-  // Remove duplicated instructions.
+  // Remove duplicated instructions from the tail of MBB1: any branch
+  // instructions, and the common instructions counted by NumDups2.
   DI1 = MBB1.end();
+  while (DI1 != MBB1.begin()) {
+    MachineBasicBlock::iterator Prev = std::prev(DI1);
+    if (!Prev->isBranch() && !Prev->isDebugInstr())
+      break;
+    DI1 = Prev;
+  }
   for (unsigned i = 0; i != NumDups2; ) {
     // NumDups2 only counted non-dbg_value instructions, so this won't
     // run off the head of the list.
     assert(DI1 != MBB1.begin());
     --DI1;
     // skip dbg_value instructions
     if (!DI1->isDebugInstr())
       ++i;

test/CodeGen/ARM/ifcvt-diamond-unanalyzable-common.mir

+# RUN: llc %s -o - -run-pass=if-converter | FileCheck %s
+# Make sure we correctly if-convert blocks containing an INLINEASM_BR.
+# CHECK: t2CMPri killed renamable $r2, 34
+# CHECK-NEXT: $r0 = t2MOVi 2, 1, $cpsr, $noreg
+# CHECK-NEXT: $r0 = t2MOVi 3, 0, killed $cpsr, $noreg, implicit killed $r0
+# CHECK-NEXT: tBL 14, $noreg, @fn2
+# CHECK-NEXT: INLINEASM_BR &"", 9, 13, 0, 13, blockaddress(@fn1, %ir-block.l_yes)
+# CHECK-NEXT: t2B %bb.1, 14, $noreg
+--- |
+  target triple = "thumbv7-unknown-linux-gnueabi"
+  
+  define dso_local void @fn1() {
+  l_yes:
+    ret void
+  }
+  
+  declare dso_local i32 @fn2(...)
+...
+---
+name:            fn1
+alignment:       1
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1(0x40000000), %bb.2(0x40000000)
+    liveins: $r0, $r1, $r2, $r4, $lr
+  
+    $sp = frame-setup t2STMDB_UPD $sp, 14, $noreg, killed $r4, killed $lr
+    t2CMPri killed renamable $r2, 34, 14, $noreg, implicit-def $cpsr
+    t2Bcc %bb.2, 1, killed $cpsr
+  
+  bb.1:
+    successors: %bb.3(0x40000000), %bb.4(0x40000000)
+    liveins: $r1
+  
+    $r0 = t2MOVi 3, 14, $noreg, $noreg
+    tBL 14, $noreg, @fn2, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit $r0, implicit $r1, implicit-def $sp, implicit-def dead $r0
+    INLINEASM_BR &"", 9, 13, 0, 13, blockaddress(@fn1, %ir-block.l_yes)
+    t2B %bb.3, 14, $noreg
+  
+  bb.2:
+    successors: %bb.3(0x40000000), %bb.4(0x40000000)
+    liveins: $r1
+  
+    $r0 = t2MOVi 2, 14, $noreg, $noreg
+    tBL 14, $noreg, @fn2, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit $r0, implicit $r1, implicit-def $sp, implicit-def dead $r0
+    INLINEASM_BR &"", 9, 13, 0, 13, blockaddress(@fn1, %ir-block.l_yes)
+    t2B %bb.3, 14, $noreg
+  
+  bb.3:
+    successors: %bb.4(0x80000000)
+  
+    INLINEASM &"", 1
+  
+  bb.4.l_yes (address-taken):
+    $sp = t2LDMIA_RET $sp, 14, $noreg, def $r4, def $pc
+
+...