Merge pull request swiftlang#26252 from gottesmm/pr-26f2be9085e39bcfada4ac0e174959d83d1bf8bd

[closure-lifetime-fixup] Delete dead trivial phi nodes.

Author: gottesmm

include/swift/SILOptimizer/Utils/CFG.h

@@ -45,6 +45,18 @@ TermInst *addNewEdgeValueToBranch(TermInst *Branch, SILBasicBlock *Dest,
 TermInst *changeEdgeValue(TermInst *Branch, SILBasicBlock *Dest, size_t Idx,
                           SILValue Val);
 
+/// Deletes the edge value between a branch and a destination basic block at the
+/// specified index. Asserts internally that the argument along the edge does
+/// not have uses.
+TermInst *deleteEdgeValue(TermInst *branch, SILBasicBlock *destBlock,
+                          size_t argIndex);
+
+/// Erase the \p argIndex phi argument from \p block. Asserts that the argument
+/// is a /real/ phi argument. Removes all incoming values for the argument from
+/// predecessor terminators. Asserts internally that it only ever is given
+/// "true" phi argument.
+void erasePhiArgument(SILBasicBlock *block, unsigned argIndex);
+
 /// Replace a branch target.
 ///
 /// \param T The terminating instruction to modify.

lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp

@@ -118,6 +118,89 @@ static void findReachableExitBlocks(SILInstruction *i,
   }
 }
 
+/// We use this to ensure that we properly handle recursive cases by revisiting
+/// phi nodes that we are tracking. This just makes it easier to reproduce in a
+/// test case.
+static llvm::cl::opt<bool> ReverseInitialWorklist(
+    "sil-closure-lifetime-fixup-reverse-phi-order", llvm::cl::init(false),
+    llvm::cl::desc(
+        "Reverse the order in which we visit phis for testing purposes"),
+    llvm::cl::Hidden);
+
+// Finally, we need to prune phis inserted by the SSA updater that
+// only take the .none from the entry block. This means that they are
+// not actually reachable from the .some() so we know that we do not
+// need to lifetime extend there at all. As an additional benefit, we
+// eliminate the need to balance these arguments to satisfy the
+// ownership verifier. This occurs since arguments are a place in SIL
+// where the trivialness of an enums case is erased.
+static void
+cleanupDeadTrivialPhiArgs(SILValue initialValue,
+                          SmallVectorImpl<SILPhiArgument *> &insertedPhis) {
+  // Just for testing purposes.
+  if (ReverseInitialWorklist) {
+    std::reverse(insertedPhis.begin(), insertedPhis.end());
+  }
+  SmallVector<SILPhiArgument *, 8> worklist(insertedPhis.begin(),
+                                            insertedPhis.end());
+  sortUnique(insertedPhis);
+  SmallVector<SILValue, 8> incomingValues;
+
+  while (!worklist.empty()) {
+    // Clear the incoming values array after each iteration.
+    SWIFT_DEFER { incomingValues.clear(); };
+
+    auto *phi = worklist.pop_back_val();
+    {
+      auto it = lower_bound(insertedPhis, phi);
+      if (it == insertedPhis.end() || *it != phi)
+        continue;
+    }
+
+    // TODO: When we split true phi arguments from transformational terminators,
+    // this will always succeed and the assert can go away.
+    bool foundPhiValues = phi->getIncomingPhiValues(incomingValues);
+    (void)foundPhiValues;
+    assert(foundPhiValues && "Should always have 'true' phi arguments since "
+                             "these were inserted by the SSA updater.");
+    if (llvm::any_of(incomingValues,
+                     [&](SILValue v) { return v != initialValue; }))
+      continue;
+
+    // Remove it from our insertedPhis list to prevent us from re-visiting this.
+    {
+      auto it = lower_bound(insertedPhis, phi);
+      assert((it != insertedPhis.end() && *it == phi) &&
+             "Should have found the phi");
+      insertedPhis.erase(it);
+    }
+
+    // See if any of our users are branch or cond_br. If so, we may have
+    // exposed additional unneeded phis. Add it back to the worklist in such a
+    // case.
+    for (auto *op : phi->getUses()) {
+      auto *user = op->getUser();
+
+      if (!isa<BranchInst>(user) && !isa<CondBranchInst>(user))
+        continue;
+
+      auto *termInst = cast<TermInst>(user);
+      for (auto succBlockArgList : termInst->getSuccessorBlockArguments()) {
+        copy_if(succBlockArgList, std::back_inserter(worklist),
+                [&](SILPhiArgument *succArg) -> bool {
+                  auto it = lower_bound(insertedPhis, succArg);
+                  return it != insertedPhis.end() && *it == succArg;
+                });
+      }
+    }
+
+    // Then RAUW the phi with the entryBlockOptionalNone and erase the
+    // argument.
+    phi->replaceAllUsesWith(initialValue);
+    erasePhiArgument(phi->getParent(), phi->getIndex());
+  }
+}
+
 /// Extend the lifetime of the convert_escape_to_noescape's operand to the end
 /// of the function.
 ///
@@ -157,7 +240,8 @@ static void extendLifetimeToEndOfFunction(SILFunction &fn,
   // Ok. At this point we know that Cvt is not in the entry block... so we can
   // use SILSSAUpdater::GetValueInMiddleOfBlock() to extend the object's
   // lifetime respecting loops.
-  SILSSAUpdater updater;
+  SmallVector<SILPhiArgument *, 8> insertedPhis;
+  SILSSAUpdater updater(&insertedPhis);
   updater.Initialize(optionalEscapingClosureTy);
 
   // Create an Optional<() -> ()>.none in the entry block of the function and
@@ -166,10 +250,11 @@ static void extendLifetimeToEndOfFunction(SILFunction &fn,
   // Since we know that Cvt is not in the entry block and this must be, we know
   // that it is safe to use the SSAUpdater's getValueInMiddleOfBlock with this
   // value.
-  updater.AddAvailableValue(fn.getEntryBlock(), [&]() -> SILValue {
+  SILValue entryBlockOptionalNone = [&]() -> SILValue {
     SILBuilderWithScope b(fn.getEntryBlock()->begin());
     return b.createOptionalNone(loc, optionalEscapingClosureTy);
-  }());
+  }();
+  updater.AddAvailableValue(fn.getEntryBlock(), entryBlockOptionalNone);
 
   // Create a copy of the convert_escape_to_no_escape and add it as an available
   // value to the SSA updater.
@@ -206,6 +291,14 @@ static void extendLifetimeToEndOfFunction(SILFunction &fn,
     SILValue v = updater.GetValueAtEndOfBlock(block);
     SILBuilderWithScope(safeDestructionPt).createDestroyValue(loc, v);
   }
+
+  // Finally, we need to prune phis inserted by the SSA updater that only take
+  // the .none from the entry block.
+  //
+  // TODO: Should we sort inserted phis before or after we initialize
+  // the worklist or maybe backwards? We should investigate how the
+  // SSA updater adds phi nodes to this list to resolve this question.
+  cleanupDeadTrivialPhiArgs(entryBlockOptionalNone, insertedPhis);
 }
 
 static SILInstruction *lookThroughRebastractionUsers(
@@ -724,16 +817,19 @@ static bool fixupCopyBlockWithoutEscaping(CopyBlockWithoutEscapingInst *cb,
   auto optionalEscapingClosureTy =
       SILType::getOptionalType(sentinelClosure->getType());
 
-  SILSSAUpdater updater;
+  SmallVector<SILPhiArgument *, 8> insertedPhis;
+  SILSSAUpdater updater(&insertedPhis);
   updater.Initialize(optionalEscapingClosureTy);
 
   // Create the Optional.none as the beginning available value.
+  SILValue entryBlockOptionalNone;
   {
     SILBuilderWithScope b(fn.getEntryBlock()->begin());
-    updater.AddAvailableValue(
-        fn.getEntryBlock(),
-        b.createOptionalNone(autoGenLoc, optionalEscapingClosureTy));
+    entryBlockOptionalNone =
+        b.createOptionalNone(autoGenLoc, optionalEscapingClosureTy);
+    updater.AddAvailableValue(fn.getEntryBlock(), entryBlockOptionalNone);
   }
+  assert(entryBlockOptionalNone);
 
   // Then create the Optional.some(closure sentinel).
   //
@@ -790,6 +886,14 @@ static bool fixupCopyBlockWithoutEscaping(CopyBlockWithoutEscapingInst *cb,
     }
   }
 
+  // Finally, we need to prune phis inserted by the SSA updater that only take
+  // the .none from the entry block.
+  //
+  // TODO: Should we sort inserted phis before or after we initialize
+  // the worklist or maybe backwards? We should investigate how the
+  // SSA updater adds phi nodes to this list to resolve this question.
+  cleanupDeadTrivialPhiArgs(entryBlockOptionalNone, insertedPhis);
+
   return true;
 }
 

lib/SILOptimizer/Transforms/SimplifyCFG.cpp

@@ -2712,84 +2712,6 @@ bool SimplifyCFG::tailDuplicateObjCMethodCallSuccessorBlocks() {
   return Changed;
 }
 
-static void
-deleteTriviallyDeadOperandsOfDeadArgument(MutableArrayRef<Operand> TermOperands,
-                                          unsigned DeadArgIndex) {
-  Operand &Op = TermOperands[DeadArgIndex];
-  auto *I = Op.get()->getDefiningInstruction();
-  if (!I)
-    return;
-  Op.set(SILUndef::get(Op.get()->getType(), *I->getFunction()));
-  recursivelyDeleteTriviallyDeadInstructions(I);
-}
-
-static void removeArgumentFromTerminator(SILBasicBlock *BB, SILBasicBlock *Dest,
-                                         int idx) {
-  TermInst *Branch = BB->getTerminator();
-  SILBuilderWithScope Builder(Branch);
-  LLVM_DEBUG(llvm::dbgs() << "remove dead argument " << idx << " from "
-                          << *Branch);
-
-  if (auto *CBI = dyn_cast<CondBranchInst>(Branch)) {
-    SmallVector<SILValue, 8> TrueArgs;
-    SmallVector<SILValue, 8> FalseArgs;
-
-    for (auto A : CBI->getTrueArgs())
-      TrueArgs.push_back(A);
-
-    for (auto A : CBI->getFalseArgs())
-      FalseArgs.push_back(A);
-
-    if (Dest == CBI->getTrueBB()) {
-      deleteTriviallyDeadOperandsOfDeadArgument(CBI->getTrueOperands(), idx);
-      TrueArgs.erase(TrueArgs.begin() + idx);
-    }
-
-    if (Dest == CBI->getFalseBB()) {
-      deleteTriviallyDeadOperandsOfDeadArgument(CBI->getFalseOperands(), idx);
-      FalseArgs.erase(FalseArgs.begin() + idx);
-    }
-
-    Builder.createCondBranch(CBI->getLoc(), CBI->getCondition(),
-                             CBI->getTrueBB(), TrueArgs, CBI->getFalseBB(),
-                             FalseArgs, CBI->getTrueBBCount(),
-                             CBI->getFalseBBCount());
-    Branch->eraseFromParent();
-    return;
-  }
-
-  if (auto *BI = dyn_cast<BranchInst>(Branch)) {
-    SmallVector<SILValue, 8> Args;
-
-    for (auto A : BI->getArgs())
-      Args.push_back(A);
-
-    deleteTriviallyDeadOperandsOfDeadArgument(BI->getAllOperands(), idx);
-    Args.erase(Args.begin() + idx);
-    Builder.createBranch(BI->getLoc(), BI->getDestBB(), Args);
-    Branch->eraseFromParent();
-    return;
-  }
-  llvm_unreachable("unsupported terminator");
-}
-
-static void removeArgument(SILBasicBlock *BB, unsigned i) {
-  NumDeadArguments++;
-  BB->eraseArgument(i);
-
-  // Determine the set of predecessors in case any predecessor has
-  // two edges to this block (e.g. a conditional branch where both
-  // sides reach this block).
-  llvm::SetVector<SILBasicBlock *,SmallVector<SILBasicBlock *, 8>,
-                  SmallPtrSet<SILBasicBlock *, 8>> PredBBs;
-
-  for (auto *Pred : BB->getPredecessorBlocks())
-    PredBBs.insert(Pred);
-
-  for (auto *Pred : PredBBs)
-    removeArgumentFromTerminator(Pred, BB, i);
-}
-
 namespace {
 
 class ArgumentSplitter {
@@ -3026,7 +2948,8 @@ bool ArgumentSplitter::split() {
       Worklist.push_back(A);
       continue;
     }
-    removeArgument(ParentBB, i);
+    erasePhiArgument(ParentBB, i);
+    ++NumDeadArguments;
   }
 
   return true;
@@ -3766,7 +3689,8 @@ bool SimplifyCFG::simplifyArgs(SILBasicBlock *BB) {
       continue;
     }
 
-    removeArgument(BB, i);
+    erasePhiArgument(BB, i);
+    ++NumDeadArguments;
     Changed = true;
   }
 

lib/SILOptimizer/Utils/CFG.cpp

@@ -77,6 +77,77 @@ TermInst *swift::addNewEdgeValueToBranch(TermInst *Branch, SILBasicBlock *Dest,
   return NewBr;
 }
 
+static void
+deleteTriviallyDeadOperandsOfDeadArgument(MutableArrayRef<Operand> termOperands,
+                                          unsigned deadArgIndex) {
+  Operand &op = termOperands[deadArgIndex];
+  auto *i = op.get()->getDefiningInstruction();
+  if (!i)
+    return;
+  op.set(SILUndef::get(op.get()->getType(), *i->getFunction()));
+  recursivelyDeleteTriviallyDeadInstructions(i);
+}
+
+// Our implementation assumes that our caller is attempting to remove a dead
+// SILPhiArgument from a SILBasicBlock and has already RAUWed the argument.
+TermInst *swift::deleteEdgeValue(TermInst *branch, SILBasicBlock *destBlock,
+                                 size_t argIndex) {
+  if (auto *cbi = dyn_cast<CondBranchInst>(branch)) {
+    SmallVector<SILValue, 8> trueArgs;
+    SmallVector<SILValue, 8> falseArgs;
+
+    copy(cbi->getTrueArgs(), std::back_inserter(trueArgs));
+    copy(cbi->getFalseArgs(), std::back_inserter(falseArgs));
+
+    if (destBlock == cbi->getTrueBB()) {
+      deleteTriviallyDeadOperandsOfDeadArgument(cbi->getTrueOperands(), argIndex);
+      trueArgs.erase(trueArgs.begin() + argIndex);
+    }
+
+    if (destBlock == cbi->getFalseBB()) {
+      deleteTriviallyDeadOperandsOfDeadArgument(cbi->getFalseOperands(), argIndex);
+      falseArgs.erase(falseArgs.begin() + argIndex);
+    }
+
+    SILBuilderWithScope builder(cbi);
+    auto *result = builder.createCondBranch(cbi->getLoc(), cbi->getCondition(),
+                             cbi->getTrueBB(), trueArgs, cbi->getFalseBB(),
+                             falseArgs, cbi->getTrueBBCount(),
+                             cbi->getFalseBBCount());
+    branch->eraseFromParent();
+    return result;
+  }
+
+  if (auto *bi = dyn_cast<BranchInst>(branch)) {
+    SmallVector<SILValue, 8> args;
+    copy(bi->getArgs(), std::back_inserter(args));
+
+    deleteTriviallyDeadOperandsOfDeadArgument(bi->getAllOperands(), argIndex);
+    args.erase(args.begin() + argIndex);
+    auto *result = SILBuilderWithScope(bi).createBranch(bi->getLoc(), bi->getDestBB(), args);
+    branch->eraseFromParent();
+    return result;
+  }
+
+  llvm_unreachable("unsupported terminator");
+}
+
+void swift::erasePhiArgument(SILBasicBlock *block, unsigned argIndex) {
+  assert(block->getArgument(argIndex)->isPhiArgument() &&
+         "Only should be used on phi arguments");
+  block->eraseArgument(argIndex);
+
+  // Determine the set of predecessors in case any predecessor has
+  // two edges to this block (e.g. a conditional branch where both
+  // sides reach this block).
+  SmallVector<SILBasicBlock *, 8> predBlocks(block->pred_begin(),
+                                             block->pred_end());
+  sortUnique(predBlocks);
+
+  for (auto *pred : predBlocks)
+    deleteEdgeValue(pred->getTerminator(), block, argIndex);
+}
+
 /// Changes the edge value between a branch and destination basic block
 /// at the specified index. Changes all edges from \p Branch to \p Dest to carry
 /// the value.