Merge pull request #34635 from atrick/verify-critedge

Verify non-critical edges in OSSA

Author: atrick

include/swift/SILOptimizer/Utils/CFGOptUtils.h

@@ -83,6 +83,11 @@ void replaceBranchTarget(TermInst *t, SILBasicBlock *oldDest,
 /// Check if the edge from the terminator is critical.
 bool isCriticalEdge(TermInst *t, unsigned edgeIdx);
 
+inline bool isNonCriticalEdge(SILBasicBlock *predBB, SILBasicBlock *succBB) {
+  return predBB->getSingleSuccessorBlock() == succBB
+    || succBB->getSinglePredecessorBlock() == predBB;
+}
+
 /// Splits the edge from terminator if it is critical.
 ///
 /// Updates dominance information and loop information if not null.

lib/SIL/Verifier/SILVerifier.cpp

@@ -5201,37 +5201,30 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
 
   void verifyBranches(const SILFunction *F) {
     // Verify no critical edge.
-    auto isCriticalEdgePred = [](const TermInst *T, unsigned EdgeIdx) {
-      assert(T->getSuccessors().size() > EdgeIdx && "Not enough successors");
-
+    auto requireNonCriticalSucc = [this](const TermInst *termInst,
+                                         const Twine &message) {
       // A critical edge has more than one outgoing edges from the source
       // block.
-      auto SrcSuccs = T->getSuccessors();
-      if (SrcSuccs.size() <= 1)
-        return false;
-
-      // And its destination block has more than one predecessor.
-      SILBasicBlock *DestBB = SrcSuccs[EdgeIdx];
-      assert(!DestBB->pred_empty() && "There should be a predecessor");
-      if (DestBB->getSinglePredecessorBlock())
-        return false;
+      auto succBlocks = termInst->getSuccessorBlocks();
+      if (succBlocks.size() <= 1)
+        return;
 
-      return true;
+      for (const SILBasicBlock *destBB : succBlocks) {
+        // And its destination block has more than one predecessor.
+        _require(destBB->getSinglePredecessorBlock(), message);
+      }
     };
 
-    for (auto &BB : *F) {
-      const TermInst *TI = BB.getTerminator();
-      CurInstruction = TI;
+    for (auto &bb : *F) {
+      const TermInst *termInst = bb.getTerminator();
+      CurInstruction = termInst;
 
-      // FIXME: In OSSA, critical edges will never be allowed.
-      // In Lowered SIL, they are allowed on unconditional branches only.
-      //   if (!(isSILOwnershipEnabled() && F->hasOwnership()))
-      if (AllowCriticalEdges && isa<CondBranchInst>(TI))
-        continue;
-
-      for (unsigned Idx = 0, e = BB.getSuccessors().size(); Idx != e; ++Idx) {
-        require(!isCriticalEdgePred(TI, Idx),
-                "non cond_br critical edges not allowed");
+      if (isSILOwnershipEnabled() && F->hasOwnership()) {
+        requireNonCriticalSucc(termInst, "critical edges not allowed in OSSA");
+      }
+      // In Lowered SIL, they are allowed on conditional branches only.
+      if (!AllowCriticalEdges && !isa<CondBranchInst>(termInst)) {
+        requireNonCriticalSucc(termInst, "only cond_br critical edges allowed");
       }
     }
   }

lib/SILOptimizer/Transforms/CopyPropagation.cpp

@@ -453,12 +453,10 @@ static bool computeLiveness(CopyPropagationState &pass) {
 /// (assuming no critical edges).
 static void insertDestroyOnCFGEdge(SILBasicBlock *predBB, SILBasicBlock *succBB,
                                    CopyPropagationState &pass) {
-  // FIXME: ban critical edges and avoid invalidating CFG analyses.
-  auto *destroyBB = splitIfCriticalEdge(predBB, succBB);
-  if (destroyBB != succBB)
-    pass.markInvalid(SILAnalysis::InvalidationKind::Branches);
+  assert(succBB->getSinglePredecessorBlock() == predBB &&
+         "value is live-out on another predBB successor: critical edge?");
 
-  SILBuilderWithScope builder(destroyBB->begin());
+  SILBuilderWithScope builder(succBB->begin());
   auto *di =
       builder.createDestroyValue(succBB->begin()->getLoc(), pass.currDef);
 
@@ -543,13 +541,8 @@ static void findOrInsertDestroys(CopyPropagationState &pass) {
     auto visitBB = [&](SILBasicBlock *bb, SILBasicBlock *succBB) {
       switch (pass.liveness.isBlockLive(bb)) {
       case LiveOut:
-        // If succBB is null, then the original destroy must be an inner
-        // nested destroy, so just skip it.
-        //
-        // Otherwise, this CFG edge is a liveness boundary, so insert a new
-        // destroy on the edge.
-        if (succBB)
-          insertDestroyOnCFGEdge(bb, succBB, pass);
+        assert(succBB && "value live-out of a block where it is consumed");
+        insertDestroyOnCFGEdge(bb, succBB, pass);
         break;
       case LiveWithin:
         // The liveness boundary is inside this block. Insert a final destroy

lib/SILOptimizer/Utils/CFGOptUtils.cpp

@@ -601,9 +601,10 @@ bool swift::hasCriticalEdges(SILFunction &f, bool onlyNonCondBr) {
     if (isa<BranchInst>(bb.getTerminator()))
       continue;
 
-    for (unsigned idx = 0, e = bb.getSuccessors().size(); idx != e; ++idx)
-      if (isCriticalEdge(bb.getTerminator(), idx))
+    for (SILBasicBlock *succBB : bb.getSuccessorBlocks()) {
+      if (!isNonCriticalEdge(&bb, succBB))
         return true;
+    }
   }
   return false;
 }

test/SILOptimizer/copy_propagation.sil

@@ -166,15 +166,18 @@ bb0(%arg : @owned $T, %addr : $*T):
 // CHECK-LABEL: } // end sil function 'testDestroyEdge'
 sil [ossa] @testDestroyEdge : $@convention(thin) <T> (@in T, @inout T, Builtin.Int1) -> () {
 bb0(%arg : @owned $T, %addr : $*T, %z : $Builtin.Int1):
-  cond_br %z, bb1, bb2
+  cond_br %z, bb2, bb1
 
 bb1:
+  br bb3
+
+bb2:
   debug_value %arg : $T
   %copy = copy_value %arg : $T
   destroy_value %copy : $T
-  br bb2
+  br bb3
 
-bb2:
+bb3:
   destroy_value %arg : $T
   %10 = tuple ()
   return %10 : $()