Merge pull request #19886 from atrick/silgen-critedge

SILGen: simplify cleanups and avoid critical edges.

Author: atrick

include/swift/SIL/BasicBlockUtils.h

@@ -20,6 +20,11 @@ namespace swift {
 class SILFunction;
 class SILBasicBlock;
 
+/// \brief Merge a basic block ending in a branch with its successor
+/// if possible.
+void mergeBasicBlockWithSingleSuccessor(SILBasicBlock *BB,
+                                        SILBasicBlock *succBB);
+
 /// A utility for finding dead-end blocks.
 ///
 /// Dead-end blocks are blocks from which there is no path to the function exit

include/swift/SIL/SILBuilder.h

@@ -173,11 +173,20 @@ class SILBuilder {
   /// location.
   ///
   /// Clients should prefer this constructor.
-  SILBuilder(SILInstruction *I, SILBuilderContext &C)
+  SILBuilder(SILInstruction *I, const SILDebugScope *DS, SILBuilderContext &C)
       : TempContext(C.getModule()), C(C), F(I->getFunction()) {
+    assert(DS && "instruction has no debug scope");
+    setCurrentDebugScope(DS);
     setInsertionPoint(I);
   }
 
+  SILBuilder(SILBasicBlock *BB, const SILDebugScope *DS, SILBuilderContext &C)
+      : TempContext(C.getModule()), C(C), F(BB->getParent()) {
+    assert(DS && "block has no debug scope");
+    setCurrentDebugScope(DS);
+    setInsertionPoint(BB);
+  }
+
   // Allow a pass to override the current SIL module conventions. This should
   // only be done by a pass responsible for lowering SIL to a new stage
   // (e.g. AddressLowering).
@@ -2123,10 +2132,8 @@ class SILBuilderWithScope : public SILBuilder {
   ///
   /// Clients should prefer this constructor.
   SILBuilderWithScope(SILInstruction *I, SILBuilderContext &C)
-    : SILBuilder(I, C) {
-    assert(I->getDebugScope() && "instruction has no debug scope");
-    setCurrentDebugScope(I->getDebugScope());
-  }
+    : SILBuilder(I, I->getDebugScope(), C)
+  {}
 
   explicit SILBuilderWithScope(
       SILInstruction *I,

include/swift/SIL/SILCloner.h

@@ -614,6 +614,10 @@ void SILCloner<ImplClass>::visitBlocksDepthFirst(
   assert(preorderBlocks.empty());
 
   // First clone the CFG region.
+  //
+  // FIXME: Add reverse iteration to SILSuccessor, then convert this to an RPOT
+  // traversal. We would prefer to keep CFG regions in RPO order, and this would
+  // be more scalable for functions with many large switches.
   SmallVector<SILBasicBlock *, 8> dfsWorklist(1, startBB);
   while (!dfsWorklist.empty()) {
     auto *BB = dfsWorklist.pop_back_val();

lib/SIL/BasicBlockUtils.cpp

@@ -11,11 +11,31 @@
 //===----------------------------------------------------------------------===//
 
 #include "swift/SIL/BasicBlockUtils.h"
-#include "swift/SIL/SILFunction.h"
+#include "swift/SIL/SILArgument.h"
 #include "swift/SIL/SILBasicBlock.h"
+#include "swift/SIL/SILFunction.h"
 
 using namespace swift;
 
+/// Merge the basic block with its successor if possible.
+void swift::mergeBasicBlockWithSingleSuccessor(SILBasicBlock *BB,
+                                               SILBasicBlock *succBB) {
+  auto *BI = cast<BranchInst>(BB->getTerminator());
+  assert(succBB->getSinglePredecessorBlock());
+
+  // If there are any BB arguments in the destination, replace them with the
+  // branch operands, since they must dominate the dest block.
+  for (unsigned i = 0, e = BI->getArgs().size(); i != e; ++i)
+    succBB->getArgument(i)->replaceAllUsesWith(BI->getArg(i));
+
+  BI->eraseFromParent();
+
+  // Move the instruction from the successor block to the current block.
+  BB->spliceAtEnd(succBB);
+
+  succBB->eraseFromParent();
+}
+
 //===----------------------------------------------------------------------===//
 //                              DeadEndBlocks
 //===----------------------------------------------------------------------===//

lib/SILGen/Cleanup.cpp

@@ -154,16 +154,13 @@ void CleanupManager::emitCleanupsForReturn(CleanupLocation loc,
 }
 
 /// Emit a new block that jumps to the specified location and runs necessary
-/// cleanups based on its level.  If there are no cleanups to run, this just
-/// returns the dest block.
+/// cleanups based on its level. Emit a block even if there are no cleanups;
+/// this is usually the destination of a conditional branch, so jumping
+/// straight to `dest` creates a critical edge.
 SILBasicBlock *CleanupManager::emitBlockForCleanups(JumpDest dest,
                                                     SILLocation branchLoc,
                                                     ArrayRef<SILValue> args,
                                                     ForUnwind_t forUnwind) {
-  // If there are no cleanups to run, just return the Dest block directly.
-  if (!hasAnyActiveCleanups(dest.getDepth()))
-    return dest.getBlock();
-
   // Otherwise, create and emit a new block.
   auto *newBlock = SGF.createBasicBlock();
   SILGenSavedInsertionPoint IPRAII(SGF, newBlock);

lib/SILGen/Condition.cpp

@@ -19,14 +19,9 @@
 using namespace swift;
 using namespace Lowering;
 
-void Condition::enterTrue(SILGenFunction &SGF) {
-  assert(TrueBB && "Cannot call enterTrue without a True block!");
-  
-  // TrueBB has already been inserted somewhere unless there's a
-  // continuation block.
-  if (!ContBB) return;
-  
-  SGF.B.emitBlock(TrueBB);
+void Condition::enter(SILGenFunction &SGF, SILBasicBlock *destBB) {
+  assert(destBB && "Cannot reenter a finished block.");
+  SGF.B.emitBlock(destBB);
 }
 
 /// Extract the last SILLocation used in BB.
@@ -36,110 +31,24 @@ static SILLocation getContinuationLoc(SILBasicBlock &BB, SILLocation Fallback) {
       return L;
   return Fallback;
 }
-void Condition::exitTrue(SILGenFunction &SGF, ArrayRef<SILValue> Args) {
-  // If there's no continuation block, it's because the condition was
-  // folded to true.  In that case, we just continue emitting code as
-  // if we were still in the true case, and we're unreachable iff the
-  // end of the true case is unreachable.  In other words, there's
-  // nothing to do.
-  if (!ContBB) {
-    assert(!FalseBB && "no continuation");
-    return;
-  }
-  
-  // If there is a continuation block, we should branch to it if the
-  // current point is reachable.
-  if (!SGF.B.hasValidInsertionPoint()) {
-    // If there is no false code, the continuation block has a use
-    // because the main condition jumps directly to it.
-    assert(ContBB->pred_empty() || !FalseBB);
+void Condition::exit(SILGenFunction &SGF, SILBasicBlock *destBB,
+                     ArrayRef<SILValue> Args) {
+  // If the current point it reachable, branch to the continuation block.
+  if (!SGF.B.hasValidInsertionPoint())
     return;
-  }
 
-  // Otherwise, resume into the continuation block.  This branch might
-  // be folded by exitFalse if it turns out that that point is
-  // unreachable.
   SGF.B.createBranch(getContinuationLoc(*SGF.B.getInsertionBB(), Loc),
                      ContBB, Args);
-  
-  // Coming out of exitTrue, we can be in one of three states:
-  //   - a valid non-terminal IP, but only if there is no continuation
-  //     block, which is only possible if there is no false block;
-  //   - a valid terminal IP, if the end of the true block was reachable; or
-  //   - a cleared IP, if the end of the true block was not reachable.
-}
-
-void Condition::enterFalse(SILGenFunction &SGF) {
-  assert(FalseBB && "entering the false branch when it was not valid");
-  
-  // FalseBB has already been inserted somewhere unless there's a
-  // continuation block.
-  if (!ContBB) return;
-  
-  // It's possible to have no insertion point here if the end of the
-  // true case was unreachable.
-  SGF.B.emitBlock(FalseBB);
-}
-
-void Condition::exitFalse(SILGenFunction &SGF, ArrayRef<SILValue> Args) {
-  // If there's no continuation block, it's because the condition was
-  // folded to false.  In that case, we just continue emitting code as
-  // if we were still in the false case, and we're unreachable iff the
-  // end of the false case is unreachable.  In other words, there's
-  // nothing to do.
-  if (!ContBB) return;
-  
-  if (ContBB->pred_empty()) {
-    // If the true case didn't need the continuation block, then
-    // we don't either, regardless of whether the current location
-    // is reachable.  Just keep inserting / being unreachable
-    // right where we are.
-  } else if (!SGF.B.hasValidInsertionPoint()) {
-    // If the true case did need the continuation block, but the false
-    // case doesn't, just merge the continuation block back into its
-    // single predecessor and move the IP there.
-    //
-    // Note that doing this tends to strand the false code after
-    // everything else in the function, so maybe it's not a great idea.
-    auto PI = ContBB->pred_begin();
-    SILBasicBlock *ContPred = *PI;
-
-    // Verify there was only a single predecessor to ContBB.
-    ++PI;
-    assert(PI == ContBB->pred_end() && "Only expect one branch to the ContBB");
-    
-    // Insert before the uncond branch and zap it.
-    auto *Br = cast<BranchInst>(ContPred->getTerminator());
-    SGF.B.setInsertionPoint(Br->getParent());
-
-    Br->eraseFromParent();
-    assert(ContBB->pred_empty() &&
-           "Zapping the branch should make ContBB dead");
-  } else {
-    // Otherwise, branch to the continuation block and start inserting there.
-    SGF.B.createBranch(getContinuationLoc(*SGF.B.getInsertionBB(), Loc),
-                       ContBB, Args);
-  }
 }
 
 SILBasicBlock *Condition::complete(SILGenFunction &SGF) {
-  // If there is no continuation block, it's because we
-  // constant-folded the branch.  The case-exit will have left us in a
-  // normal insertion state (i.e. not a post-terminator IP) with
-  // nothing to clean up after.
-  if (!ContBB) {
-    return SGF.B.getInsertionBB();
+  assert(!TrueBB && "enterTrue is always called.");
+  if (FalseBB) {
+    assert(ContBB->getNumArguments() == 0 &&
+           "block arguments require a non-empty false path.");
+    SILGenBuilder(SGF.B, FalseBB).createBranch(Loc, ContBB);
+    FalseBB = nullptr;
   }
-  
-  // Kill the continuation block if it's not being used.  Case-exits
-  // only leave themselves post-terminator if they use the
-  // continuation block, so we're in an acceptable insertion state.
-  if (ContBB->pred_empty() && ContBB->args_empty()) {
-    SGF.eraseBasicBlock(ContBB);
-    return SGF.B.hasValidInsertionPoint() ? SGF.B.getInsertionBB() : nullptr;
-  }
-  
-  // Okay, we need to insert the continuation block.
   SGF.B.emitBlock(ContBB);
   return ContBB;
 }

lib/SILGen/Condition.h

@@ -32,10 +32,12 @@ namespace Lowering {
 
 /// A condition is the result of evaluating a boolean expression as
 /// control flow.
+///
+/// For each Condition instance, `enterTrue` must be called before `complete`.
+/// If `enterFalse` is skipped, then an empty fall-through block is created.
 class LLVM_LIBRARY_VISIBILITY Condition {
-  /// The blocks responsible for executing the true and false conditions.  A
-  /// block is non-null if that branch is possible, but it's only an independent
-  /// block if both branches are possible.
+  /// The blocks responsible for executing the true and false conditions. These
+  /// are initialized non-null and set to null after being emitted.
   SILBasicBlock *TrueBB;
   SILBasicBlock *FalseBB;
 
@@ -50,30 +52,39 @@ class LLVM_LIBRARY_VISIBILITY Condition {
             SILBasicBlock *ContBB,
             SILLocation L)
     : TrueBB(TrueBB), FalseBB(FalseBB), ContBB(ContBB), Loc(L)
-  {}
-  
-  bool hasTrue() const { return TrueBB; }
-  bool hasFalse() const { return FalseBB; }
-  
-  /// enterTrue - Begin the emission of the true block.  This should only be
-  /// called if hasTrue() returns true.
-  void enterTrue(SILGenFunction &SGF);
-  
-  /// exitTrue - End the emission of the true block.  This must be called after
-  /// enterTrue but before anything else on this Condition.
-  void exitTrue(SILGenFunction &SGF, ArrayRef<SILValue> Args = {});
-  
-  /// enterFalse - Begin the emission of the false block.  This should only be
-  /// called if hasFalse() returns true.
-  void enterFalse(SILGenFunction &SGF);
-  
-  /// exitFalse - End the emission of the true block.  This must be called after
-  /// enterFalse but before anything else on this Condition.
-  void exitFalse(SILGenFunction &SGF, ArrayRef<SILValue> Args = {});
-  
+  {
+    assert((TrueBB != nullptr && FalseBB != nullptr) &&
+           "Requires non-null block pointers.");
+  }
+
+  /// enterTrue - Begin the emission of the true block.
+  void enterTrue(SILGenFunction &SGF) { enter(SGF, TrueBB); }
+
+  /// exitTrue - End the emission of the true block.
+  void exitTrue(SILGenFunction &SGF, ArrayRef<SILValue> Args = {}) {
+    exit(SGF, TrueBB, Args);
+    TrueBB = nullptr;
+  }
+
+  /// enterFalse - Begin the emission of the false block.
+  void enterFalse(SILGenFunction &SGF) { enter(SGF, FalseBB); }
+
+  /// exitFalse - End the emission of the true block.
+  void exitFalse(SILGenFunction &SGF, ArrayRef<SILValue> Args = {}) {
+    exit(SGF, FalseBB, Args);
+    FalseBB = nullptr;
+  }
+
   /// complete - Complete this conditional execution.  This should be called
   /// only after all other calls on this Condition have been made.
+  /// This leaves SGF's SILGenBuilder at the continuation block.
   SILBasicBlock *complete(SILGenFunction &SGF);
+
+protected:
+  void enter(SILGenFunction &SGF, SILBasicBlock *destBB);
+
+  void exit(SILGenFunction &SGF, SILBasicBlock *destBB,
+            ArrayRef<SILValue> Args = {});
 };
 
 /// A conditional value is one that depends on conditional execution.

lib/SILGen/SILGenBuilder.h

@@ -63,6 +63,14 @@ class SILGenBuilder : public SILBuilder {
                 SILBasicBlock::iterator insertInst)
       : SILGenBuilder(SGF, &*insertBB, insertInst) {}
 
+  // Create a new builder, inheriting the given builder's context and debug
+  // scope.
+  SILGenBuilder(SILGenBuilder &builder, SILBasicBlock *insertBB)
+    : SILBuilder(insertBB, builder.getCurrentDebugScope(),
+                 builder.getBuilderContext()),
+      SGF(builder.SGF)
+  {}
+
   SILGenModule &getSILGenModule() const;
   SILGenFunction &getSILGenFunction() const { return SGF; }
 

lib/SILGen/SILGenDecl.cpp

@@ -1264,7 +1264,7 @@ SILValue SILGenFunction::emitOSVersionRangeCheck(SILLocation loc,
 /// specified JumpDest.  The insertion point is left in the block where the
 /// condition has matched and any bound variables are in scope.
 ///
-void SILGenFunction::emitStmtCondition(StmtCondition Cond, JumpDest FailDest,
+void SILGenFunction::emitStmtCondition(StmtCondition Cond, JumpDest FalseDest,
                                        SILLocation loc,
                                        ProfileCounter NumTrueTaken,
                                        ProfileCounter NumFalseTaken) {
@@ -1279,7 +1279,7 @@ void SILGenFunction::emitStmtCondition(StmtCondition Cond, JumpDest FailDest,
     switch (elt.getKind()) {
     case StmtConditionElement::CK_PatternBinding: {
       InitializationPtr initialization =
-      InitializationForPattern(*this, FailDest).visit(elt.getPattern());
+      InitializationForPattern(*this, FalseDest).visit(elt.getPattern());
 
       // Emit the initial value into the initialization.
       FullExpr Scope(Cleanups, CleanupLocation(elt.getInitializer()));
@@ -1320,8 +1320,8 @@ void SILGenFunction::emitStmtCondition(StmtCondition Cond, JumpDest FailDest,
 
     // Just branch on the condition.  On failure, we unwind any active cleanups,
     // on success we fall through to a new block.
+    auto FailBB = Cleanups.emitBlockForCleanups(FalseDest, loc);
     SILBasicBlock *ContBB = createBasicBlock();
-    auto FailBB = Cleanups.emitBlockForCleanups(FailDest, loc);
     B.createCondBranch(booleanTestLoc, booleanTestValue, ContBB, FailBB,
                        NumTrueTaken, NumFalseTaken);