[IfConversion] Fix bug related to !HasFallThrough (#145471)

We can not trust that !HasFallThrough implies that there is not
fallthrough exit in cases when analyzeBranch failed.

Adding a new blockNeverFallThrough helper to make the tests on
!HasFallThrough safe by also checking IsBrAnalyzable. We also
try to prove no-fallthrough by inspecting the successor list. If
the textual successor isn't in the successor list we know that
there is no fallthrough.

The bug has probably been around for years. Found it when
working on an out-of-tree target.

Author: bjope

llvm/lib/CodeGen/IfConversion.cpp

@@ -117,15 +117,22 @@ namespace {
     /// IsAnalyzed      - True if BB has been analyzed (info is still valid).
     /// IsEnqueued      - True if BB has been enqueued to be ifcvt'ed.
     /// IsBrAnalyzable  - True if analyzeBranch() returns false.
-    /// HasFallThrough  - True if BB may fallthrough to the following BB.
+    /// HasFallThrough  - True if BB has fallthrough to the following BB.
+    ///                   Note that BB may have a fallthrough if both
+    ///                   !HasFallThrough and !IsBrAnalyzable is true. Also note
+    ///                   that blockNeverFallThrough() can be used to prove that
+    ///                   there is no fall through.
     /// IsUnpredicable  - True if BB is known to be unpredicable.
     /// ClobbersPred    - True if BB could modify predicates (e.g. has
     ///                   cmp, call, etc.)
     /// NonPredSize     - Number of non-predicated instructions.
     /// ExtraCost       - Extra cost for multi-cycle instructions.
     /// ExtraCost2      - Some instructions are slower when predicated
     /// BB              - Corresponding MachineBasicBlock.
-    /// TrueBB / FalseBB- See analyzeBranch().
+    /// TrueBB / FalseBB- See analyzeBranch(), but note that FalseBB can be set
+    ///                   by AnalyzeBranches even if there is a fallthrough. So
+    ///                   it doesn't correspond exactly to the result from
+    ///                   TTI::analyzeBranch.
     /// BrCond          - Conditions for end of block conditional branches.
     /// Predicate       - Predicate used in the BB.
     struct BBInfo {
@@ -397,6 +404,21 @@ namespace {
       return BBI.IsBrAnalyzable && BBI.TrueBB == nullptr;
     }
 
+    /// Returns true if Block is known not to fallthrough to the following BB.
+    bool blockNeverFallThrough(BBInfo &BBI) const {
+      // Trust "HasFallThrough" if we could analyze branches.
+      if (BBI.IsBrAnalyzable)
+        return !BBI.HasFallThrough;
+      // If this is the last MBB in the function, or if the textual successor
+      // isn't in the successor list, then there is no fallthrough.
+      MachineFunction::iterator PI = BBI.BB->getIterator();
+      MachineFunction::iterator I = std::next(PI);
+      if (I == BBI.BB->getParent()->end() || !PI->isSuccessor(&*I))
+        return true;
+      // Could not prove that there is no fallthrough.
+      return false;
+    }
+
     /// Used to sort if-conversion candidates.
     static bool IfcvtTokenCmp(const std::unique_ptr<IfcvtToken> &C1,
                               const std::unique_ptr<IfcvtToken> &C2) {
@@ -1715,9 +1737,8 @@ bool IfConverter::IfConvertTriangle(BBInfo &BBI, IfcvtKind Kind) {
     // Only merge them if the true block does not fallthrough to the false
     // block. By not merging them, we make it possible to iteratively
     // ifcvt the blocks.
-    if (!HasEarlyExit &&
-        NextMBB.pred_size() == 1 && !NextBBI->HasFallThrough &&
-        !NextMBB.hasAddressTaken()) {
+    if (!HasEarlyExit && NextMBB.pred_size() == 1 &&
+        blockNeverFallThrough(*NextBBI) && !NextMBB.hasAddressTaken()) {
       MergeBlocks(BBI, *NextBBI);
       FalseBBDead = true;
     } else {
@@ -2052,8 +2073,8 @@ bool IfConverter::IfConvertDiamond(BBInfo &BBI, IfcvtKind Kind,
     BBI.BB->removeSuccessor(FalseBBI.BB, true);
 
     BBInfo &TailBBI = BBAnalysis[TailBB->getNumber()];
-    bool CanMergeTail = !TailBBI.HasFallThrough &&
-      !TailBBI.BB->hasAddressTaken();
+    bool CanMergeTail =
+        blockNeverFallThrough(TailBBI) && !TailBBI.BB->hasAddressTaken();
     // The if-converted block can still have a predicated terminator
     // (e.g. a predicated return). If that is the case, we cannot merge
     // it with the tail block.

llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir

@@ -21,30 +21,30 @@
 # As a consequence we could end up merging blocks at the end of a converted
 # diamond/triangle and while doing that we messed up when fixing up the CFG
 # related to fallthrough edges. For the test cases below we incorrectly ended
-# up ended up with a fallthrough from the MBBs with two Bcc instructions to
-# the MBB with the STRH after if conversion.
+# up with a fallthrough from the MBBs with two Bcc instructions to the MBB
+# with the STRH after if conversion.
 #
 ---
 name:            avoidMergeBlockDiamond
 body:             |
   ; CHECK-LABEL: name: avoidMergeBlockDiamond
   ; CHECK: bb.0:
-  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   $sp = tADDspi $sp, 2, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT:   $sp = tADDspi $sp, 1, 0 /* CC::eq */, $cpsr, implicit $sp
   ; CHECK-NEXT:   $sp = tADDspi $sp, 3, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1:
-  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
-  ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tB %bb.1, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2:
-  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tB %bb.2, 14 /* CC::al */, $noreg
   bb.0:
     tBcc %bb.2, 1, $cpsr
 
@@ -76,21 +76,21 @@ name:            avoidMergeBlockTriangle
 body:             |
   ; CHECK-LABEL: name: avoidMergeBlockTriangle
   ; CHECK: bb.0:
-  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   $sp = tADDspi $sp, 1, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT:   $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1:
-  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
-  ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tB %bb.1, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2:
-  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tB %bb.2, 14 /* CC::al */, $noreg
   bb.0:
     tBcc %bb.1, 1, $cpsr
     tB %bb.3, 14, $noreg