Merge pull request swiftlang#39566 from atrick/comment-ossa

Minor source and doc comments related to OSSA

Author: atrick

docs/SIL.rst

@@ -3842,7 +3842,7 @@ mark_function_escape
 
   sil-instruction ::= 'mark_function_escape' sil-operand (',' sil-operand)
 
-  %2 = mark_function_escape %1 : $*T
+  mark_function_escape %1 : $*T
 
 Indicates that a function definition closes over a symbolic memory location.
 This instruction is variadic, and all of its operands must be addresses.

include/swift/Basic/DAGNodeWorklist.h

@@ -25,6 +25,9 @@
 ///
 /// The primary API has two methods: intialize() and pop(). Others are provided
 /// for flexibility.
+///
+/// TODO: This also works well for cyclic graph traversal. Particularly CFG
+/// traversal. So we should probably just call it GraphNodeWorklist.
 template <typename T, unsigned SmallSize> struct DAGNodeWorklist {
   llvm::SmallPtrSet<T, SmallSize> nodeVisited;
   llvm::SmallVector<T, SmallSize> nodeVector;

include/swift/SIL/MemAccessUtils.h

@@ -77,7 +77,7 @@
 /// Each object's property or its tail storage is separately accessed.
 ///
 /// In addition to the free-standing functions, the AccessBase and
-/// AccessStorage classes encapsulte the identity of an access. They can be
+/// AccessStorage classes encapsulate the identity of an access. They can be
 /// used to:
 /// - directly compare and hash access identities
 /// - exhaustively switch over the kinds of accesses

include/swift/SIL/OwnershipUtils.h

@@ -712,6 +712,9 @@ class InteriorPointerOperandKind {
 /// object with guaranteed ownership. All transitive address uses of the
 /// interior pointer must be within the lifetime of the guaranteed lifetime. As
 /// such, these must be treated as implicit uses of the parent guaranteed value.
+///
+/// FIXME: This should probably also handle project_box once we support
+/// borrowing a box.
 struct InteriorPointerOperand {
   Operand *operand;
   InteriorPointerOperandKind kind;

include/swift/SIL/SILInstruction.h

@@ -4038,6 +4038,10 @@ class BeginBorrowInst
         lexical(isLexical) {}
 
 public:
+  // FIXME: this does not return all instructions that end a local borrow
+  // scope. Branches can also end it via a reborrow, so APIs using this are
+  // incorrect. Instead, either iterate over all uses and return those with
+  // OperandOwnership::EndBorrow or Reborrow.
   using EndBorrowRange =
       decltype(std::declval<ValueBase>().getUsersOfType<EndBorrowInst>());
 

include/swift/SILOptimizer/Utils/OwnershipOptUtils.h

@@ -34,9 +34,6 @@ inline bool requiresOSSACleanup(SILValue v) {
     && v.getOwnershipKind() != OwnershipKind::Unowned;
 }
 
-// Defined in BasicBlockUtils.h
-struct JointPostDominanceSetComputer;
-
 /// Given a new phi that may use a guaranteed value, create nested borrow scopes
 /// for its incoming operands and end_borrows that cover the phi's extended
 /// borrow scope, which transitively includes any phis that use this phi.

lib/SIL/Verifier/LinearLifetimeChecker.cpp

@@ -503,7 +503,7 @@ void State::checkDataflowEndState(DeadEndBlocks &deBlocks) {
       }
       llvm::errs() << "Post Dominating Failure Blocks:\n";
       for (auto *succBlock : successorBlocksThatMustBeVisited) {
-        llvm::errs() << "bb" << succBlock->getDebugID();
+        llvm::errs() << " bb" << succBlock->getDebugID();
       }
       llvm::errs() << '\n';
     });

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -950,6 +950,10 @@ static void replaceAllUsesAndFixLifetimes(SILValue foldedVal,
   // destroy foldedVal at the end of the borrow scope.
   assert(originalVal.getOwnershipKind() == OwnershipKind::Guaranteed);
 
+  // FIXME: getUniqueBorrowScopeIntroducingValue may look though various storage
+  // casts. There's no reason to think that it's valid to replace uses of
+  // originalVal with a new borrow of the the "introducing value". All casts
+  // potentially need to be cloned.
   Optional<BorrowedValue> originalScopeBegin =
       getUniqueBorrowScopeIntroducingValue(originalVal);
   assert(originalScopeBegin &&

lib/SILOptimizer/Utils/CanonicalOSSALifetime.cpp

@@ -514,6 +514,9 @@ void CanonicalizeOSSALifetime::findOrInsertDestroyInBlock(SILBasicBlock *bb) {
 ///   consuming uses, including destroys on all return paths.
 /// - The postdominating consumes cannot be within nested loops.
 /// - Any blocks in nested loops are now marked LiveOut.
+///
+/// TODO: replace this with PrunedLivenessAnalysis::computeBoundary. Separate
+/// out destroy insertion, debug info, diagnostics, etc. as post-passes.
 void CanonicalizeOSSALifetime::findOrInsertDestroys() {
   this->accessBlocks = accessBlockAnalysis->get(getCurrentDef()->getFunction());
 
@@ -525,7 +528,7 @@ void CanonicalizeOSSALifetime::findOrInsertDestroys() {
     switch (liveness.getBlockLiveness(bb)) {
     case PrunedLiveBlocks::LiveOut:
       // A lifetimeEndBlock may be determined to be LiveOut after analyzing the
-      // extended  It is irrelevent for finding the boundary.
+      // liveness. It is irrelevent for finding the boundary.
       break;
     case PrunedLiveBlocks::LiveWithin: {
       // The liveness boundary is inside this block. Insert a final destroy