[SimplifyCFG] Handle tail-sinking of more than 2 incoming branches

[jmolloy]

Summary

This was a real restriction in the original version of SinkIfThenCodeToEnd. Now it's been rewritten, the restriction can be lifted.

As part of this, we handle a very common and useful case where one of the incoming branches is actually conditional. Consider:

if (a)
  x(1);
else if (b)
  x(2);

This produces the following CFG:

   [if]
  /    \
[x(1)] [if]
  |     | \
  |     |  \
  |  [x(2)] |
   \    |  /
    [ end ]

[end] has two unconditional predecessor arcs and one conditional. The conditional refers to the implicit empty 'else' arc. This same pattern can also be caused by an empty default block in a switch.

We can't sink the call to x() down to end because no call to x() happens on the third incoming arc (assume that x() has sideeffects for the sake of argument; if something is safe to speculate we could indeed sink nevertheless but this cannot happen in the general case and causes many extra selects).

We are now able to detect this case and split off the unconditional arcs to a common successor:

    [if]
   /    \
 [x(1)] [if]
   |     | \
   |     |  \
   |  [x(2)] |
    \   /    |
[sink.split] |
      \     /
      [ end ]

Now we can sink the call to x() into %sink.split. This can cause significant code simplification in many testcases.

[hans]

lib/Transforms/Utils/SimplifyCFG.cpp

1463

(Ultra nit: I think 'f' is a better name for a random function than 'x' which is more commonly used for variables.)

1483

Would this scale to larger if-else chains where some subset of them have similar instructions? I assume not because the order of the if-statements makes this hard even if it doesn't really matter for semantics.

1496

Maybe call these Predecessors and UnconditionalPredecessors?

[jmolloy]

lib/Transforms/Utils/SimplifyCFG.cpp

1483

In general this is a very hard problem as you say.

I've been thinking that it should be possible to use a suffix tree or levenshtein distance to find the longest common substring in all incoming blocks. It should be possible to support the case where some blocks have a common subsequence and others don't...

Bit of a longer term blue sky thing though.

[jmolloy]

Hi Hans,

Thanks for the comments! New version attached.

James

[hans]

lib/Transforms/Utils/SimplifyCFG.cpp

1497

Actually, is Preds even used after it's been populated?

If not, maybe this is all we need:

SmallVector<BasicBlock *, 4> UnconditionalPreds;
Instruction *ConditionalPred = nullptr;

and the logic in the for loop could be simplified a little:

if (T is unconditional)
  push to UnconditionalPreds
else if (T is conditional && !ConditionalPred)
  set ConditionalPred
else
  bail out

[jmolloy]

Thanks Hans. I should have addressed all of your comments now.

[hans]

lgtm

[Eugene.Zelenko]

Committed in rL280364. Please specify "Differential revision: <URL>" in commit message.

lib/Transforms/Utils/SimplifyCFG.cpp

 /// Given an unconditional branch that goes to BBEnd,
 /// check whether BBEnd has only two predecessors and the other predecessor
 /// ends with an unconditional branch. If it is true, sink any common code
 /// in the two predecessors to BBEnd.
 static bool SinkThenElseCodeToEnd(BranchInst *BI1) {
   assert(BI1->isUnconditional());
   BasicBlock *BBEnd = BI1->getSuccessor(0);
 
-  // We currently only support branch targets with two predecessors.
-  // FIXME: this is an arbitrary restriction and should be lifted.
-  SmallVector<BasicBlock*,4> Blocks;
-  for (auto *BB : predecessors(BBEnd))
-    Blocks.push_back(BB);
-  if (Blocks.size() != 2 ||
-      !all_of(Blocks, [](const BasicBlock *BB) {
-        auto *BI = dyn_cast<BranchInst>(BB->getTerminator());
-        return BI && BI->isUnconditional();
-      }))
+  // We support two situations:
+  //   (1) all incoming arcs are unconditional
+  //   (2) one incoming arc is conditional
+  //
+  // (2) is very common in switch defaults and
+  // else-if patterns;
+  //
+  //   if (a) x(1);
+  //   else if (b) x(2);
+  //
+  // produces:
+  //
+  //       [if]
+  //      /    \
+  //    [x(1)] [if]
+  //      |     | \
+  //      |     |  \
+  //      |  [x(2)]|
+  //       \    | /
+  //        [ end ]
+  //
+  // [end] has two unconditional predecessor arcs and one conditional. The
+  // conditional refers to the implicit empty 'else' arc. This conditional
+  // arc can also be caused by an empty default block in a switch.
+  //
+  // In this case, we attempt to sink code from all *unconditional* arcs.
+  // If we can sink instructions from these arcs (determined during the scan
+  // phase below) we insert a common successor for all unconditional arcs and
+  // connect that to [end], to enable sinking:
+  //
+  //       [if]
+  //      /    \
+  //    [x(1)] [if]
+  //      |     | \
+  //      |     |  \
+  //      |  [x(2)] |
+  //       \   /    |
+  //   [sink.split] |
+  //         \     /
+  //         [ end ]
+  //
+  SmallVector<BasicBlock*,4> Blocks, AllBlocks;
+  Instruction *Cond = nullptr;
+  for (auto *B : predecessors(BBEnd)) {
+    AllBlocks.push_back(B);
+    auto *T = B->getTerminator();
+    if (isa<BranchInst>(T) && cast<BranchInst>(T)->isUnconditional()) {
+      Blocks.push_back(B);
+      continue;
+    }
+    if (isa<BranchInst>(T) || isa<SwitchInst>(T)) {
+      if (Cond)
+        return false;
+      Cond = T;
+      continue;
+    }
     return false;
-
+  }
+  if (Blocks.size() < 2)
+    return false;
+  
   bool Changed = false;
   SmallVector<Instruction*,4> Insts;
 
   // Helper to populate the Insts list with the Idx'th instruction from
   // the end of each block (excluding terminator). So Idx=0 would populate
   // Insts with the last non-terminator instruction from each block.
   //
   // Return false if this isn't possible (one block isn't large enough)

   // they're not identical in each instruction) we add these to
   // ValuesNeedingPHIs.
   unsigned ScanIdx = 0;
   SmallPtrSet<Value*,4> ValuesToSink;
   SmallVector<SmallVector<Value*,4>,4> ValuesNeedingPHIs;
   SmallVector<Value*,4> ThisValuesNeedingPHIs;
   while (PopulateInsts(ScanIdx) &&
          canSinkInstruction(Insts, ThisValuesNeedingPHIs)) {
+    DEBUG(dbgs() << "SINK: instruction can be sunk: " << *Insts[0] << " - "
+                 << ThisValuesNeedingPHIs.size() << " phis needed\n");
     ValuesNeedingPHIs.push_back(ThisValuesNeedingPHIs);
     ValuesToSink.insert(Insts.begin(), Insts.end());
     ++ScanIdx;
   }
 
+  if (ScanIdx > 0 && Cond) {
+    DEBUG(dbgs() << "SINK: Splitting edge\n");
+    // We have a conditional edge and we're going to sink some instructions.
+    // Insert a new block postdominating all blocks we're going to sink from.
+    if (!SplitBlockPredecessors(BI1->getSuccessor(0), Blocks, ".sink.split"))
+      // Edges couldn't be split.
+      return false;
+    Changed = true;
+  }
+  
   // Now that we've analyzed all potential sinking candidates, perform the
   // actual sink. We iteratively sink the last non-terminator of the source
   // blocks into their common successor unless doing so would require too
   // many PHI instructions to be generated (currently only one PHI is allowed
   // per sunk instruction).
   //
   // We can use ValuesToSink to discount values needing PHI-merging that will
   // actually be sunk in a later iteration. This allows us to be more
   // aggressive in what we sink. This does allow a false positive where we
   // sink presuming a later value will also be sunk, but stop half way through
   // and never actually sink it which means we produce more PHIs than intended.
   // This is unlikely in practice though.
   for (unsigned SinkIdx = 0; SinkIdx != ScanIdx; ++SinkIdx) {
+    DEBUG(dbgs() << "SINK: Sink: " << *Blocks[0]->getTerminator()->getPrevNode()
+                 << "\n");
     unsigned NumPHIdValues = 0;
     for (auto *V : ValuesNeedingPHIs[SinkIdx])
       if (ValuesToSink.count(V) == 0)
         ++NumPHIdValues;
+    DEBUG(dbgs() << "SINK: #phid values: " << NumPHIdValues << "\n");
     assert((NumPHIdValues % Blocks.size() == 0) &&
            "Every operand must either be PHId or not PHId!");
     
-    if (NumPHIdValues / Blocks.size() > 1)
+    if (NumPHIdValues / Blocks.size() > 1) {
       // Too many PHIs would be created.
+      DEBUG(dbgs() << "SINK: stopping here, too many PHIs would be created!\n");
       break;
+    }
     
     sinkLastInstruction(Blocks);
     NumSinkCommons++;
     Changed = true;
   }
-  
   return Changed;
 }
 
 /// \brief Determine if we can hoist sink a sole store instruction out of a
 /// conditional block.
 ///
 /// We are looking for code like the following:
 ///   BrBB:

test/Transforms/SimplifyCFG/sink-common-code.ll

if.end:
   %p = phi i1 [ %cmp1, %if.then ], [ %cmp2, %if.else ]
   ret i32 1
 }
 
 ; CHECK-LABEL: test15
 ; CHECK: getelementptr
 ; CHECK: load
 ; CHECK-NOT: load
+
+define zeroext i1 @test16(i1 zeroext %flag, i1 zeroext %flag2, i32 %blksA, i32 %blksB, i32 %nblks) {
+entry:
+  br i1 %flag, label %if.then, label %if.else
+
+if.then:
+  %cmp = icmp uge i32 %blksA, %nblks
+  %frombool1 = zext i1 %cmp to i8
+  br label %if.end
+
+if.else:
+  br i1 %flag2, label %if.then2, label %if.end
+
+if.then2:
+  %add = add i32 %nblks, %blksB
+  %cmp2 = icmp ule i32 %add, %blksA
+  %frombool3 = zext i1 %cmp2 to i8
+  br label %if.end
+
+if.end:
+  %obeys.0 = phi i8 [ %frombool1, %if.then ], [ %frombool3, %if.then2 ], [ 0, %if.else ]
+  %tobool4 = icmp ne i8 %obeys.0, 0
+  ret i1 %tobool4
+}
+
+; CHECK-LABEL: test16
+; CHECK: zext
+; CHECK-NOT: zext
+
+define zeroext i1 @test17(i32 %flag, i32 %blksA, i32 %blksB, i32 %nblks) {
+entry:
+  switch i32 %flag, label %if.end [
+    i32 0, label %if.then
+    i32 1, label %if.then2
+  ]
+
+if.then:
+  %cmp = icmp uge i32 %blksA, %nblks
+  %frombool1 = zext i1 %cmp to i8
+  br label %if.end
+
+if.then2:
+  %add = add i32 %nblks, %blksB
+  %cmp2 = icmp ule i32 %add, %blksA
+  %frombool3 = zext i1 %cmp2 to i8
+  br label %if.end
+
+if.end:
+  %obeys.0 = phi i8 [ %frombool1, %if.then ], [ %frombool3, %if.then2 ], [ 0, %entry ]
+  %tobool4 = icmp ne i8 %obeys.0, 0
+  ret i1 %tobool4
+}
+
+; CHECK-LABEL: test17
+; CHECK: if.then:
+; CHECK-NEXT: icmp uge
+; CHECK-NEXT: br label %[[x:.*]]
+
+; CHECK: if.then2:
+; CHECK-NEXT: add
+; CHECK-NEXT: icmp ule
+; CHECK-NEXT: br label %[[x]]
+
+; CHECK: [[x]]:
+; CHECK-NEXT: %[[y:.*]] = phi i1 [ %cmp
+; CHECK-NEXT: %[[z:.*]] = zext i1 %[[y]]
+; CHECK-NEXT: br label %if.end
+
+; CHECK: if.end:
+; CHECK-NEXT: phi i8
+; CHECK-DAG: [ %[[z]], %[[x]] ]
+; CHECK-DAG: [ 0, %entry ]
+
+define zeroext i1 @test18(i32 %flag, i32 %blksA, i32 %blksB, i32 %nblks) {
+entry:
+  switch i32 %flag, label %if.then3 [
+    i32 0, label %if.then
+    i32 1, label %if.then2
+  ]
+
+if.then:
+  %cmp = icmp uge i32 %blksA, %nblks
+  %frombool1 = zext i1 %cmp to i8
+  br label %if.end
+
+if.then2:
+  %add = add i32 %nblks, %blksB
+  %cmp2 = icmp ule i32 %add, %blksA
+  %frombool3 = zext i1 %cmp2 to i8
+  br label %if.end
+
+if.then3:
+  %add2 = add i32 %nblks, %blksA
+  %cmp3 = icmp ule i32 %add2, %blksA
+  %frombool4 = zext i1 %cmp3 to i8
+  br label %if.end
+
+if.end:
+  %obeys.0 = phi i8 [ %frombool1, %if.then ], [ %frombool3, %if.then2 ], [ %frombool4, %if.then3 ]
+  %tobool4 = icmp ne i8 %obeys.0, 0
+  ret i1 %tobool4
+}
+
+; CHECK-LABEL: test18
+; CHECK: if.end:
+; CHECK-NEXT: %[[x:.*]] = phi i1
+; CHECK-DAG: [ %cmp, %if.then ]
+; CHECK-DAG: [ %cmp2, %if.then2 ]
+; CHECK-DAG: [ %cmp3, %if.then3 ]
+; CHECK-NEXT: zext i1 %[[x]] to i8