[IfConversion] Add missing check in IfConversion/canFallThroughTo

[uabelho]

Summary

When trying to figure out if MBB could fallthrough to ToMBB (possibly by
falling through a bunch of other MBBs) we didn't actually check if there
was fallthrough between the last two blocks in the chain.

[uabelho]

Not sure who is code owner of IfConversion.cpp so I added the last three persons who changed
anything there as reviwers.

[kparzysz]

LGTM with a simplified testcase.

test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir

4

You can remove this entire section (with the LLVM IR).

31

You can remove lines 30-50.

[MatzeB]

test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir

4

See also http://llvm.org/docs/MIRLangRef.html#simplifying-mir-files

[iteratee]

Looks good to me with the changes mentioned.

[uabelho]

Simplified the testcase

[uabelho]

Thanks for the comments! I didn't know you could omit this much from the mir file so I learned something today :) I suppose I can submit this now then?

[kparzysz]

In D32996#750710, @uabelho wrote:

Thanks for the comments! I didn't know you could omit this much from the mir file so I learned something today :) I suppose I can submit this now then?

Yes, go ahead.

lib/CodeGen/IfConversion.cpp

static bool canFallThroughTo(MachineBasicBlock &MBB, MachineBasicBlock &ToMBB) {
   MachineFunction::iterator E = MBB.getParent()->end();
   while (I != TI) {
     // Check isSuccessor to avoid case where the next block is empty, but
     // it's not a successor.
     if (I == E || !I->empty() || !PI->isSuccessor(&*I))
       return false;
     PI = I++;
   }
-  return true;
+  // Finally see if the last I is indeed a successor to PI.
+  return PI->isSuccessor(&*I);
 }
 
 /// Invalidate predecessor BB info so it would be re-analyzed to determine if it
 /// can be if-converted. If predecessor is already enqueued, dequeue it!
 void IfConverter::InvalidatePreds(MachineBasicBlock &MBB) {
   for (const MachineBasicBlock *Predecessor : MBB.predecessors()) {
     BBInfo &PBBI = BBAnalysis[Predecessor->getNumber()];
     if (PBBI.IsDone || PBBI.BB == &MBB)

test/CodeGen/MIR/ARM/ifcvt_canFallThroughTo.mir

+# RUN: llc -mtriple=arm-apple-ios -o - %s -run-pass if-converter | FileCheck %s
+---
+name:            f1
+body:             |
+  bb.0:
+    successors: %bb.1
+
+    B %bb.1
+
+  bb.1:
+    successors: %bb.2, %bb.4
+
+    Bcc %bb.4, 1, %cpsr
+
+  bb.2:
+    successors: %bb.3, %bb.5
+
+    Bcc %bb.5, 1, %cpsr
+
+  bb.3:
+    successors: %bb.5
+
+    B %bb.5
+
+  bb.4:
+    successors:
+
+  bb.5:
+    successors: %bb.1, %bb.6
+
+    Bcc %bb.1, 1, %cpsr
+
+  bb.6:
+    BX_RET 14, _
+
+...
+
+# IfConversion.cpp/canFallThroughTo thought there was a fallthrough from
+# bb.4 to bb5 even if the successor list was empty.
+# bb.4 is empty, so it surely looks like it can fallthrough, but this is what
+# happens for a bb just containing an "unreachable".
+
+#CHECK: body:             |
+#CHECK:   bb.0:
+#CHECK:     successors: %bb.1
+
+#CHECK:   bb.1:
+#CHECK:     successors: %bb.3({{.*}}), %bb.2
+
+# The original brr_cond from bb.1, jumping to the empty bb
+#CHECK:     Bcc %bb.2
+#CHECK:     B %bb.3
+
+# Empty bb.2, originally containing "unreachable" and thus has no successors
+#CHECK:   bb.2:
+#CHECK-NOT: successors
+
+#CHECK:   bb.3:
+#CHECK:     successors: %bb.1
+
+# Conditional BX_RET and then loop back to bb.1
+#CHECK:     BX_RET 0
+#CHECK:     B %bb.1
+