Add critical edge verification and fix SIL passes.

SIL passes were violating the existing invariant on non-cond-br
critical edges in several places. I fixed the places that I could
find.  Wherever there was a post-pass to "clean up" critical edges, I
replaced it with a a call to verification that the critical edges
aren't broken in the first place.

We still need to eliminate critical edges entirely before enabling
ownership SIL.

Author: atrick

include/swift/SIL/SILFunction.h

@@ -883,6 +883,11 @@ class SILFunction
   /// invariants.
   void verify(bool SingleFunction = true) const;
 
+  /// Verify that all non-cond-br critical edges have been split.
+  ///
+  /// This is a fast subset of the checks performed in the SILVerifier.
+  void verifyCriticalEdges() const;
+
   /// Pretty-print the SILFunction.
   void dump(bool Verbose) const;
   void dump() const;

include/swift/SILOptimizer/Utils/CFG.h

@@ -118,12 +118,11 @@ SILBasicBlock *splitBasicBlockAndBranch(SILBuilder &B,
 /// \brief Return true if the function has a critical edge, false otherwise.
 bool hasCriticalEdges(SILFunction &F, bool OnlyNonCondBr);
 
-/// \brief Split all critical edges in the function updating the dominator tree
-/// and loop information (if they are not set to null). If \p OnlyNonCondBr is
-/// true this will not split cond_br edges (Only edges which can't carry
-/// arguments will be split).
-bool splitAllCriticalEdges(SILFunction &F, bool OnlyNonCondBr,
-                           DominanceInfo *DT, SILLoopInfo *LI);
+/// \brief Split all critical edges in the given function, updating the
+/// dominator tree and loop information if they are provided.
+///
+/// FIXME: This should never be called! Fix passes that create critical edges.
+bool splitAllCriticalEdges(SILFunction &F, DominanceInfo *DT, SILLoopInfo *LI);
 
 /// \brief Split all cond_br critical edges with non-trivial arguments in the
 /// function updating the dominator tree and loop information (if they are not

include/swift/SILOptimizer/Utils/Local.h

@@ -343,37 +343,41 @@ class EdgeThreadingCloner : public BaseThreadingCloner {
   EdgeThreadingCloner(BranchInst *BI)
       : BaseThreadingCloner(*BI->getFunction(),
                             BI->getDestBB(), nullptr) {
-    DestBB = createEdgeBlockAndRedirectBranch(BI);
+    createEdgeBlockAndRedirectBranch(BI);
   }
 
-  SILBasicBlock *createEdgeBlockAndRedirectBranch(BranchInst *BI) {
+  void createEdgeBlockAndRedirectBranch(BranchInst *BI) {
     auto *Fn = BI->getFunction();
     auto *SrcBB = BI->getParent();
-    auto *DestBB = BI->getDestBB();
-    auto *EdgeBB = Fn->createBasicBlockAfter(SrcBB);
+    auto *EdgeBB = BI->getDestBB();
+
+    this->DestBB = Fn->createBasicBlockAfter(SrcBB);
 
     // Create block arguments.
-    for (unsigned ArgIdx : range(DestBB->getNumArguments())) {
-      auto *DestPHIArg = cast<SILPHIArgument>(DestBB->getArgument(ArgIdx));
+    for (unsigned ArgIdx : range(EdgeBB->getNumArguments())) {
+      auto *DestPHIArg = cast<SILPHIArgument>(EdgeBB->getArgument(ArgIdx));
       assert(BI->getArg(ArgIdx)->getType() == DestPHIArg->getType() &&
              "Types must match");
-      auto *BlockArg = EdgeBB->createPHIArgument(
+      auto *BlockArg = DestBB->createPHIArgument(
           DestPHIArg->getType(), DestPHIArg->getOwnershipKind());
       ValueMap[DestPHIArg] = SILValue(BlockArg);
       AvailVals.push_back(std::make_pair(DestPHIArg, BlockArg));
     }
 
     // Redirect the branch.
-    SILBuilderWithScope(BI).createBranch(BI->getLoc(), EdgeBB, BI->getArgs());
+    SILBuilderWithScope(BI).createBranch(BI->getLoc(), DestBB, BI->getArgs());
     BI->eraseFromParent();
-    return EdgeBB;
   }
 
   SILBasicBlock *getEdgeBB() {
     // DestBB really is the edge basic block we created to clone instructions
     // to.
     return DestBB;
   }
+
+  /// Call this after processing all instructions to fix the control flow
+  /// graph. The branch cloner may have left critical edges.
+  bool splitCriticalEdges(DominanceInfo *DT, SILLoopInfo *LI);
 };
 
 /// Helper class for cloning of basic blocks.
@@ -414,8 +418,7 @@ class BasicBlockCloner : public BaseThreadingCloner {
 /// 'NeedToSplitCriticalEdges' to false if all critical edges are split,
 /// otherwise this call will try to split all critical edges.
 void updateSSAAfterCloning(BaseThreadingCloner &Cloner, SILBasicBlock *SrcBB,
-                           SILBasicBlock *DestBB,
-                           bool NeedToSplitCriticalEdges = true);
+                           SILBasicBlock *DestBB);
 
 // Helper class that provides a callback that can be used in
 // inliners/cloners for collecting new call sites.

lib/SIL/SILVerifier.cpp

@@ -378,6 +378,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   SILOpenedArchetypesTracker OpenedArchetypes;
   SmallVector<StringRef, 16> DebugVars;
   const SILInstruction *CurInstruction = nullptr;
+  const SILArgument *CurArgument = nullptr;
   DominanceInfo *Dominance = nullptr;
 
   // Used for dominance checking within a basic block.
@@ -404,12 +405,12 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     if (CurInstruction) {
       llvm::dbgs() << "Verifying instruction:\n";
       CurInstruction->printInContext(llvm::dbgs());
-      llvm::dbgs() << "In function:\n";
-      F.print(llvm::dbgs());
-    } else {
-      llvm::dbgs() << "In function:\n";
-      F.print(llvm::dbgs());
+    } else if (CurArgument) {
+      llvm::dbgs() << "Verifying argument:\n";
+      CurArgument->printInContext(llvm::dbgs());
     }
+    llvm::dbgs() << "In function:\n";
+    F.print(llvm::dbgs());
 
     // We abort by default because we want to always crash in
     // the debugger.
@@ -637,29 +638,49 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     return M.getStage() == SILStage::Raw;
   }
 
-  void visitSILArgument(SILArgument *arg) {
-    checkLegalType(arg->getFunction(), arg, nullptr);
-    checkValueBaseOwnership(arg);
-
-    if (isa<SILPHIArgument>(arg) && prohibitAddressBlockArgs()) {
-      // As a structural SIL property, we disallow address-type block
+  void visitSILPHIArgument(SILPHIArgument *arg) {
+    // Verify that the `isPhiArgument` property is sound:
+    // - Phi arguments come from branches.
+    // - Non-phi arguments have a single predecessor.
+    if (arg->isPhiArgument()) {
+      for (SILBasicBlock *predBB : arg->getParent()->getPredecessorBlocks()) {
+        auto *TI = predBB->getTerminator();
+        // FIXME: when critical edges are removed, only allow BranchInst.
+        require(isa <BranchInst>(TI) || isa<CondBranchInst>(TI),
+                "All phi argument inputs must be from branches.");
+      }
+    } else {
+    }
+    if (arg->isPhiArgument() && prohibitAddressBlockArgs()) {
+      // As a property of well-formed SIL, we disallow address-type block
       // arguments. Supporting them would prevent reliably reasoning about the
       // underlying storage of memory access. This reasoning is important for
       // diagnosing violations of memory access rules and supporting future
       // optimizations such as bitfield packing. Address-type block arguments
       // also create unnecessary complexity for SIL optimization passes that
       // need to reason about memory aliasing.
-      //
-      // Note: We could allow non-phi block arguments to be addresses, because
-      // the address source would still be uniquely recoverable. But then
-      // we would need to separately ensure that something like begin_access is
-      // never passed as a block argument before being used by end_access. For
-      // now, it simpler to have a strict prohibition.
       require(!arg->getType().isAddress(),
               "Block arguments cannot be addresses");
     }
   }
 
+  void visitSILArgument(SILArgument *arg) {
+    CurArgument = arg;
+    checkLegalType(arg->getFunction(), arg, nullptr);
+    checkValueBaseOwnership(arg);
+    if (auto *phiArg = dyn_cast<SILPHIArgument>(arg)) {
+      if (phiArg->isPhiArgument())
+        visitSILPHIArgument(phiArg);
+      else {
+        // A non-phi BlockArgument must have a single predecessor unless it is
+        // unreachable.
+        require(arg->getParent()->pred_empty()
+                    || arg->getParent()->getSinglePredecessorBlock(),
+                "Non-branch terminator must have a unique successor.");
+      }
+    }
+  }
+
   void visitSILInstruction(SILInstruction *I) {
     CurInstruction = I;
     OpenedArchetypes.registerOpenedArchetypes(I);
@@ -4574,7 +4595,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     }
   }
 
-  void verifyBranches(SILFunction *F) {
+  void verifyBranches(const SILFunction *F) {
     // Verify that there is no non_condbr critical edge.
     auto isCriticalEdgePred = [](const TermInst *T, unsigned EdgeIdx) {
       assert(T->getSuccessors().size() > EdgeIdx && "Not enough successors");
@@ -4595,7 +4616,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     };
 
     for (auto &BB : *F) {
-      TermInst *TI = BB.getTerminator();
+      const TermInst *TI = BB.getTerminator();
       CurInstruction = TI;
 
       // Check for non-cond_br critical edges.
@@ -4742,6 +4763,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   }
 
   void visitBasicBlockArguments(SILBasicBlock *BB) {
+    CurInstruction = nullptr;
     for (auto argI = BB->args_begin(), argEnd = BB->args_end(); argI != argEnd;
          ++argI)
       visitSILArgument(*argI);
@@ -4838,6 +4860,10 @@ void SILFunction::verify(bool SingleFunction) const {
   SILVerifier(*this, SingleFunction).verify();
 }
 
+void SILFunction::verifyCriticalEdges() const {
+  SILVerifier(*this, /*SingleFunction=*/true).verifyBranches(this);
+}
+
 /// Verify that a property descriptor follows invariants.
 void SILProperty::verify(const SILModule &M) const {
 #ifdef NDEBUG

lib/SILOptimizer/LoopTransforms/COWArrayOpt.cpp

@@ -2376,9 +2376,8 @@ class SwiftArrayOptPass : public SILFunctionTransform {
       for (auto &HoistableLoopNest : HoistableLoopNests)
         ArrayPropertiesSpecializer(DT, LA, HoistableLoopNest).run();
 
-      // We might have cloned there might be critical edges that need splitting.
-      splitAllCriticalEdges(*getFunction(), true /* only cond_br terminators*/,
-                            DT, nullptr);
+      // Verify that no illegal critical edges were created.
+      getFunction()->verifyCriticalEdges();
 
       LLVM_DEBUG(getFunction()->viewCFG());
 

lib/SILOptimizer/Transforms/ARCCodeMotion.cpp

@@ -1161,7 +1161,7 @@ class ARCCodeMotion : public SILFunctionTransform {
     //
     // TODO: maybe we can do this lazily or maybe we should disallow SIL passes
     // to create critical edges.
-    bool EdgeChanged = splitAllCriticalEdges(*F, false, nullptr, nullptr);
+    bool EdgeChanged = splitAllCriticalEdges(*F, nullptr, nullptr);
     if (EdgeChanged)
       POA->invalidateFunction(F);
 

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -889,7 +889,7 @@ bool MemoryToRegisters::promoteSingleAllocation(AllocStackInst *alloc,
 bool MemoryToRegisters::run() {
   bool Changed = false;
 
-  Changed = splitAllCriticalEdges(F, true, DT, nullptr);
+  F.verifyCriticalEdges();
 
   // Compute dominator tree node levels for the function.
   DomTreeLevelMap DomTreeLevels;

lib/SILOptimizer/Transforms/SimplifyCFG.cpp

@@ -206,14 +206,7 @@ static bool isUsedOutsideOfBlock(SILValue V, SILBasicBlock *BB) {
 
 /// Helper function to perform SSA updates in case of jump threading.
 void swift::updateSSAAfterCloning(BaseThreadingCloner &Cloner,
-                                  SILBasicBlock *SrcBB, SILBasicBlock *DestBB,
-                                  bool NeedToSplitCriticalEdges) {
-  // We are updating SSA form. This means we need to be able to insert phi
-  // nodes. To make sure we can do this split all critical edges from
-  // instructions that don't support block arguments.
-  if (NeedToSplitCriticalEdges)
-    splitAllCriticalEdges(*DestBB->getParent(), true, nullptr, nullptr);
-
+                                  SILBasicBlock *SrcBB, SILBasicBlock *DestBB) {
   SILSSAUpdater SSAUp;
   for (auto AvailValPair : Cloner.AvailVals) {
     ValueBase *Inst = AvailValPair.first;
@@ -333,6 +326,7 @@ class ThreadInfo {
     // We have copied the threaded block into the edge.
     Src = Cloner.getEdgeBB();
 
+    // Rewrite the cloned branch to eliminate the non-taken path.
     if (auto *CondTerm = dyn_cast<CondBranchInst>(Src->getTerminator())) {
       // We know the direction this conditional branch is going to take thread
       // it.
@@ -371,14 +365,11 @@ class ThreadInfo {
         Builder.createBranch(SEI->getLoc(), ThreadedSuccessorBlock,
                              ArrayRef<SILValue>());
       SEI->eraseFromParent();
-
-      // Split the edge from 'Dest' to 'ThreadedSuccessorBlock' it is now
-      // critical. Doing this here safes us from doing it over the whole
-      // function in updateSSAAfterCloning because we have split all other
-      // critical edges earlier.
-      splitEdgesFromTo(Dest, ThreadedSuccessorBlock, nullptr, nullptr);
     }
-    updateSSAAfterCloning(Cloner, Src, Dest, false);
+    // After rewriting the cloned branch, split the critical edge.
+    // This does not currently update DominanceInfo.
+    Cloner.splitCriticalEdges(nullptr, nullptr);
+    updateSSAAfterCloning(Cloner, Src, Dest);
   }
 };
 
@@ -661,7 +652,7 @@ bool SimplifyCFG::dominatorBasedSimplify(DominanceAnalysis *DA) {
   // also required for SSA construction in dominatorBasedSimplifications' jump
   // threading. It only splits new critical edges it creates by jump threading.
   bool Changed =
-      EnableJumpThread ? splitAllCriticalEdges(Fn, false, DT, nullptr) : false;
+      EnableJumpThread ? splitAllCriticalEdges(Fn, DT, nullptr) : false;
 
   unsigned MaxIter = MaxIterationsOfDominatorBasedSimplify;
   SmallVector<SILBasicBlock *, 16> BlocksForWorklist;
@@ -1062,10 +1053,12 @@ bool SimplifyCFG::tryJumpThreading(BranchInst *BI) {
   // destination block into this one, rewriting uses of the BBArgs to use the
   // branch arguments as we go.
   EdgeThreadingCloner Cloner(BI);
-
   for (auto &I : *DestBB)
     Cloner.process(&I);
 
+  // Does not currently update DominanceInfo.
+  Cloner.splitCriticalEdges(nullptr, nullptr);
+
   // Once all the instructions are copied, we can nuke BI itself.  We also add
   // the threaded and edge block to the worklist now that they (likely) can be
   // simplified.
@@ -1291,6 +1284,7 @@ bool SimplifyCFG::simplifyBranchBlock(BranchInst *BI) {
     BI->eraseFromParent();
     removeIfDead(DestBB);
     addToWorklist(BB);
+
     return true;
   }
 
@@ -2444,6 +2438,9 @@ bool SimplifyCFG::tailDuplicateObjCMethodCallSuccessorBlocks() {
     for (auto &I : *DestBB)
       Cloner.process(&I);
 
+    // Does not currently update DominanceInfo.
+    Cloner.splitCriticalEdges(nullptr, nullptr);
+
     updateSSAAfterCloning(Cloner, Cloner.getEdgeBB(), DestBB);
     addToWorklist(Cloner.getEdgeBB());
   }
@@ -2863,9 +2860,7 @@ bool SimplifyCFG::run() {
     if (simplifyBlocks())
       removeUnreachableBlocks(Fn);
   }
-
-  // Split all critical edges from non cond_br terminators.
-  Changed |= splitAllCriticalEdges(Fn, true, nullptr, nullptr);
+  Fn.verifyCriticalEdges();
 
   // Canonicalize switch_enum instructions.
   Changed |= canonicalizeSwitchEnums();
@@ -3615,9 +3610,11 @@ class SplitCriticalEdges : public SILFunctionTransform {
   void run() override {
     auto &Fn = *getFunction();
 
+    if (OnlyNonCondBrEdges)
+      Fn.verifyCriticalEdges();
+
     // Split all critical edges from all or non only cond_br terminators.
-    bool Changed =
-        splitAllCriticalEdges(Fn, OnlyNonCondBrEdges, nullptr, nullptr);
+    bool Changed = splitAllCriticalEdges(Fn, nullptr, nullptr);
 
     if (Changed) {
       invalidateAnalysis(SILAnalysis::InvalidationKind::BranchesAndInstructions);

lib/SILOptimizer/Utils/CFG.cpp

@@ -704,22 +704,21 @@ bool swift::hasCriticalEdges(SILFunction &F, bool OnlyNonCondBr) {
 
 /// Split all critical edges in the function updating the dominator tree and
 /// loop information (if they are not set to null).
-bool swift::splitAllCriticalEdges(SILFunction &F, bool OnlyNonCondBr,
-                                  DominanceInfo *DT, SILLoopInfo *LI) {
+bool swift::splitAllCriticalEdges(SILFunction &F, DominanceInfo *DT,
+                                  SILLoopInfo *LI) {
   bool Changed = false;
 
   for (SILBasicBlock &BB : F) {
-    // Only split critical edges for terminators that don't support block
-    // arguments.
-    if (OnlyNonCondBr && isa<CondBranchInst>(BB.getTerminator()))
-      continue;
-
     if (isa<BranchInst>(BB.getTerminator()))
       continue;
 
-    for (unsigned Idx = 0, e = BB.getSuccessors().size(); Idx != e; ++Idx)
-      Changed |=
-        (splitCriticalEdge(BB.getTerminator(), Idx, DT, LI) != nullptr);
+    for (unsigned Idx = 0, e = BB.getSuccessors().size(); Idx != e; ++Idx) {
+      auto *NewBB = splitCriticalEdge(BB.getTerminator(), Idx, DT, LI);
+      assert(!NewBB
+             || isa<CondBranchInst>(BB.getTerminator())
+                    && "Only cond_br may have a critical edge.");
+      Changed |= (NewBB != nullptr);
+    }
   }
   return Changed;
 }