Merge pull request #19027 from atrick/fix-phi-incoming-value

Fix the SIL incoming phi value API, verify critical edges, fix SIL passes that break verification.

Author: atrick

include/swift/SIL/SILArgument.h

@@ -77,41 +77,51 @@ class SILArgument : public ValueBase {
     llvm_unreachable("SILArgument not argument of its parent BB");
   }
 
-  /// Returns the incoming SILValue from the \p BBIndex predecessor of this
-  /// argument's parent BB. If the routine fails, it returns an empty SILValue.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  SILValue getIncomingValue(unsigned BBIndex);
-
-  /// Returns the incoming SILValue for this argument from BB. If the routine
-  /// fails, it returns an empty SILValue.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  SILValue getIncomingValue(SILBasicBlock *BB);
-
-  /// Returns true if we were able to find incoming values for each predecessor
-  /// of this arguments basic block. The found values are stored in OutArray.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  bool getIncomingValues(llvm::SmallVectorImpl<SILValue> &OutArray);
-
-  /// Returns true if we were able to find incoming values for each predecessor
-  /// of this arguments basic block. The found values are stored in OutArray.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  bool getIncomingValues(
+  /// Return true if this block argument is actually a phi argument as
+  /// opposed to a cast or projection.
+  bool isPhiArgument();
+
+  /// If this argument is a phi, return the incoming phi value for the given
+  /// predecessor BB. If this argument is not a phi, return an invalid SILValue.
+  SILValue getIncomingPhiValue(SILBasicBlock *predBB);
+
+  /// If this argument is a phi, populate `OutArray` with the incoming phi
+  /// values for each predecessor BB. If this argument is not a phi, return
+  /// false.
+  bool getIncomingPhiValues(llvm::SmallVectorImpl<SILValue> &ReturnedPhiValues);
+
+  /// If this argument is a phi, populate `OutArray` with each predecessor block
+  /// and its incoming phi value. If this argument is not a phi, return false.
+  bool getIncomingPhiValues(
+      llvm::SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>>
+          &ReturnedPredAndPhiValuePairs);
+
+  /// Returns true if we were able to find a single terminator operand value for
+  /// each predecessor of this arguments basic block. The found values are
+  /// stored in OutArray.
+  ///
+  /// Note: this peeks through any projections or cast implied by the
+  /// terminator. e.g. the incoming value for a switch_enum payload argument is
+  /// the enum itself (the operand of the switch_enum).
+  bool getSingleTerminatorOperands(llvm::SmallVectorImpl<SILValue> &OutArray);
+
+  /// Returns true if we were able to find single terminator operand values for
+  /// each predecessor of this arguments basic block. The found values are
+  /// stored in OutArray alongside their predecessor block.
+  ///
+  /// Note: this peeks through any projections or cast implied by the
+  /// terminator. e.g. the incoming value for a switch_enum payload argument is
+  /// the enum itself (the operand of the switch_enum).
+  bool getSingleTerminatorOperands(
       llvm::SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>> &OutArray);
 
-  /// If this SILArgument's parent block has one predecessor, return the
-  /// incoming value from that predecessor. Returns SILValue() otherwise.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  SILValue getSingleIncomingValue() const;
+  /// If this SILArgument's parent block has a single predecessor whose
+  /// terminator has a single operand, return the incoming operand of the
+  /// predecessor's terminator. Returns SILValue() otherwise.  Note that for
+  /// some predecessor terminators the incoming value is not exactly the
+  /// argument value. E.g. the incoming value for a switch_enum payload argument
+  /// is the enum itself (the operand of the switch_enum).
+  SILValue getSingleTerminatorOperand() const;
 
 protected:
   SILArgument(ValueKind SubClassKind, SILBasicBlock *ParentBB, SILType Ty,
@@ -138,41 +148,59 @@ class SILArgument : public ValueBase {
 
 class SILPHIArgument : public SILArgument {
 public:
-  /// Returns the incoming SILValue from the \p BBIndex predecessor of this
-  /// argument's parent BB. If the routine fails, it returns an empty SILValue.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  SILValue getIncomingValue(unsigned BBIndex);
-
-  /// Returns the incoming SILValue for this argument from BB. If the routine
-  /// fails, it returns an empty SILValue.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  SILValue getIncomingValue(SILBasicBlock *BB);
-
-  /// Returns true if we were able to find incoming values for each predecessor
-  /// of this arguments basic block. The found values are stored in OutArray.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  bool getIncomingValues(llvm::SmallVectorImpl<SILValue> &OutArray);
-
-  /// Returns true if we were able to find incoming values for each predecessor
-  /// of this arguments basic block. The found values are stored in OutArray.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  bool getIncomingValues(
+  /// Return true if this is block argument is actually a phi argument as
+  /// opposed to a cast or projection.
+  bool isPhiArgument();
+
+  /// If this argument is a phi, return the incoming phi value for the given
+  /// predecessor BB. If this argument is not a phi, return an invalid SILValue.
+  ///
+  /// FIXME: Once SILPHIArgument actually implies that it is a phi argument,
+  /// this will be guaranteed to return a valid SILValue.
+  SILValue getIncomingPhiValue(SILBasicBlock *BB);
+
+  /// If this argument is a phi, populate `OutArray` with the incoming phi
+  /// values for each predecessor BB. If this argument is not a phi, return
+  /// false.
+  ///
+  /// FIXME: Once SILPHIArgument actually implies that it is a phi argument,
+  /// this will always succeed.
+  bool getIncomingPhiValues(llvm::SmallVectorImpl<SILValue> &OutArray);
+
+  /// If this argument is a phi, populate `OutArray` with each predecessor block
+  /// and its incoming phi value. If this argument is not a phi, return false.
+  ///
+  /// FIXME: Once SILPHIArgument actually implies that it is a phi argument,
+  /// this will always succeed.
+  bool getIncomingPhiValues(
       llvm::SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>> &OutArray);
 
-  /// If this SILArgument's parent block has one predecessor, return the
-  /// incoming value from that predecessor. Returns SILValue() otherwise.
-  /// Note that for some predecessor terminators the incoming value is not
-  /// exactly the argument value. E.g. the incoming value for a switch_enum
-  /// payload argument is the enum itself (the operand of the switch_enum).
-  SILValue getSingleIncomingValue() const;
+  /// Returns true if we were able to find a single terminator operand value for
+  /// each predecessor of this arguments basic block. The found values are
+  /// stored in OutArray.
+  ///
+  /// Note: this peeks through any projections or cast implied by the
+  /// terminator. e.g. the incoming value for a switch_enum payload argument is
+  /// the enum itself (the operand of the switch_enum).
+  bool getSingleTerminatorOperands(llvm::SmallVectorImpl<SILValue> &OutArray);
+
+  /// Returns true if we were able to find single terminator operand values for
+  /// each predecessor of this arguments basic block. The found values are
+  /// stored in OutArray alongside their predecessor block.
+  ///
+  /// Note: this peeks through any projections or cast implied by the
+  /// terminator. e.g. the incoming value for a switch_enum payload argument is
+  /// the enum itself (the operand of the switch_enum).
+  bool getSingleTerminatorOperands(
+      llvm::SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>> &OutArray);
+
+  /// If this SILArgument's parent block has a single predecessor whose
+  /// terminator has a single operand, return the incoming operand of the
+  /// predecessor's terminator. Returns SILValue() otherwise.  Note that for
+  /// some predecessor terminators the incoming value is not exactly the
+  /// argument value. E.g. the incoming value for a switch_enum payload argument
+  /// is the enum itself (the operand of the switch_enum).
+  SILValue getSingleTerminatorOperand() const;
 
   static bool classof(const SILInstruction *) = delete;
   static bool classof(const SILUndef *) = delete;
@@ -254,36 +282,45 @@ class SILFunctionArgument : public SILArgument {
 // Out of line Definitions for SILArgument to avoid Forward Decl issues
 //===----------------------------------------------------------------------===//
 
-inline SILValue SILArgument::getIncomingValue(unsigned BBIndex) {
-  if (isa<SILFunctionArgument>(this))
-    return SILValue();
-  return cast<SILPHIArgument>(this)->getIncomingValue(BBIndex);
+inline bool SILArgument::isPhiArgument() {
+  if (auto *phiArg = dyn_cast<SILPHIArgument>(this))
+    return phiArg->isPhiArgument();
+
+  return false;
 }
 
-inline SILValue SILArgument::getIncomingValue(SILBasicBlock *BB) {
+inline SILValue SILArgument::getIncomingPhiValue(SILBasicBlock *BB) {
   if (isa<SILFunctionArgument>(this))
     return SILValue();
-  return cast<SILPHIArgument>(this)->getIncomingValue(BB);
+  return cast<SILPHIArgument>(this)->getIncomingPhiValue(BB);
 }
 
 inline bool
-SILArgument::getIncomingValues(llvm::SmallVectorImpl<SILValue> &OutArray) {
+SILArgument::getIncomingPhiValues(llvm::SmallVectorImpl<SILValue> &OutArray) {
   if (isa<SILFunctionArgument>(this))
     return false;
-  return cast<SILPHIArgument>(this)->getIncomingValues(OutArray);
+  return cast<SILPHIArgument>(this)->getIncomingPhiValues(OutArray);
 }
 
-inline bool SILArgument::getIncomingValues(
+inline bool SILArgument::getIncomingPhiValues(
     llvm::SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>> &OutArray) {
   if (isa<SILFunctionArgument>(this))
     return false;
-  return cast<SILPHIArgument>(this)->getIncomingValues(OutArray);
+  return cast<SILPHIArgument>(this)->getIncomingPhiValues(OutArray);
 }
 
-inline SILValue SILArgument::getSingleIncomingValue() const {
+inline bool SILArgument::getSingleTerminatorOperands(
+    llvm::SmallVectorImpl<SILValue> &OutArray) {
   if (isa<SILFunctionArgument>(this))
-    return SILValue();
-  return cast<SILPHIArgument>(this)->getSingleIncomingValue();
+    return false;
+  return cast<SILPHIArgument>(this)->getSingleTerminatorOperands(OutArray);
+}
+
+inline bool SILArgument::getSingleTerminatorOperands(
+    llvm::SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>> &OutArray) {
+  if (isa<SILFunctionArgument>(this))
+    return false;
+  return cast<SILPHIArgument>(this)->getSingleTerminatorOperands(OutArray);
 }
 
 } // end swift namespace

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/InstructionUtils.cpp

@@ -465,7 +465,7 @@ void swift::findClosuresForFunctionValue(
     // This should be done before calling findClosureStoredIntoBlock.
     if (auto *arg = dyn_cast<SILPHIArgument>(V)) {
       SmallVector<std::pair<SILBasicBlock *, SILValue>, 2> blockArgs;
-      arg->getIncomingValues(blockArgs);
+      arg->getIncomingPhiValues(blockArgs);
       for (auto &blockAndArg : blockArgs)
         worklistInsert(blockAndArg.second);