[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

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

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.

llvm/trunk/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;

llvm/trunk/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
+
+...