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

Given certain CFGs, the SSA updater will insert unnecessary phi arguments that
all take the same "initial" value (the .none from the entry block). As an
example, consider the following CFG:

```
+-------+     +------+
|   2   | <-- |  0   |
+-------+     +------+
  |             |
  |             |
  |             v
  |           +------+     +---+
  |           |  1   | --> | 3 |
  |           +------+     +---+
  |             |            |
  |             |            |
  |             v            |
  |           +------+       |
  |           |  4   |       |
  |           +------+       |
  |             |            |
  |             |            |
  |             v            |
  |           +------+       |
  +---------> |  5   |       |
              +------+       |
                |            |
                |            |
                v            |
+-------+     +------+       |
| Throw | <-- |  6   |       |
+-------+     +------+       |
                |            |
                |            |
                v            |
              +------+       |
              | Exit | <-----+
              +------+
```

In this case, if our some value is actually in BB3, we will actually insert a
phi node in BB5 that has all .none incoming values. This is causing us from the
perspective of the ownership verifier to be leaking that phi node in
BBThrow. The reason why is that arguments are a location in SIL where the
specific case of the enum is erased, forcing us to use @owned if the enum has
any non-trivial cases. These of course will not be balanced by any
destroy_values reachable from the copy_value (since otherwise, we would have a
.some definition along that path), so it makes sense. Rather than try to insert
destroy_values, we just realize that in this case we know that all the incoming
values are really the .none from the entry block and delete the argument,
eliminating the problem.

Author: gottesmm

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;
 }