Fix an assert in canonicalizeFunctionArgument.

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -912,10 +912,7 @@ SILPassPipelinePlan::getOnonePassPipeline(const SILOptions &Options) {
   P.startPipeline("non-Diagnostic Enabling Mandatory Optimizations");
   P.addForEachLoopUnroll();
   P.addMandatoryCombine();
-  if (P.getOptions().CopyPropagation == CopyPropagationOption::On) {
-    // MandatoryCopyPropagation should only be run at -Onone, not -O.
-    P.addMandatoryCopyPropagation();
-  }
+
   // TODO: MandatoryARCOpts should be subsumed by CopyPropagation. There should
   // be no need to run another analysis of copies at -Onone.
   P.addMandatoryARCOpts();

lib/SILOptimizer/Transforms/CopyPropagation.cpp

@@ -89,20 +89,23 @@ struct CanonicalDefWorklist {
 
     while (true) {
       def = CanonicalizeOSSALifetime::getCanonicalCopiedDef(def);
-      if (!canonicalizeBorrows) {
-        ownedValues.insert(def);
-        return;
-      }
+
       // If the copy's source is guaranteed, find the root of a borrowed
       // extended lifetime.
       if (auto *copy = dyn_cast<CopyValueInst>(def)) {
         if (SILValue borrowDef =
                 CanonicalizeBorrowScope::getCanonicalBorrowedDef(
                     copy->getOperand())) {
-          borrowedValues.insert(borrowDef);
-          return;
+          if (canonicalizeBorrows || isa<SILFunctionArgument>(borrowDef)) {
+            borrowedValues.insert(borrowDef);
+            return;
+          }
         }
       }
+      if (!canonicalizeBorrows) {
+        ownedValues.insert(def);
+        return;
+      }
       // Look through hoistable owned forwarding instructions on the
       // use-def chain.
       if (SILInstruction *defInst = def->getDefiningInstruction()) {
@@ -491,60 +494,63 @@ void CopyPropagation::run() {
     // of the copiedDefs. If the are converted, they are removed from copiedDefs
     // and the source of the new destructure is added.
     changed |= convertExtractsToDestructures(defWorklist, deleter);
-
-    // borrowCanonicalizer performs all modifications through deleter's
-    // callbacks, so we don't need to explicitly check for changes.
-    CanonicalizeBorrowScope borrowCanonicalizer(deleter);
-    // The utilities in this loop cannot delete borrows before they are popped
-    // from the worklist.
-    while (true) {
-      while (!defWorklist.ownedForwards.empty()) {
-        SILInstruction *ownedForward = defWorklist.ownedForwards.pop_back_val();
-        // Delete a dead forwarded value before sinking to avoid this pattern:
-        //   %outerVal = destructure_struct %def
-        //   destroy %outerVal           <= delete this destroy now
-        //   destroy %def                <= so we don't delete this one later
-        if (deleter.deleteIfDead(ownedForward)) {
-          LLVM_DEBUG(llvm::dbgs() << "  Deleted " << *ownedForward);
-          continue;
-        }
-        // Canonicalize a forwarded owned value before sinking the forwarding
-        // instruction, and sink the instruction before canonicalizing the owned
-        // value being forwarded. Process 'ownedForwards' in reverse since
-        // they may be chained, and CanonicalizeBorrowScopes pushes them
-        // top-down.
-        for (auto result : ownedForward->getResults()) {
-          canonicalizer.canonicalizeValueLifetime(result);
-        }
-        if (sinkOwnedForward(ownedForward, postOrderAnalysis, domTree)) {
-          changed = true;
-          // Sinking 'ownedForward' may create an opportunity to sink its
-          // operand. This handles chained forwarding instructions that were
-          // pushed onto the list out-of-order.
-          if (SILInstruction *forwardDef =
-                  CanonicalizeOSSALifetime::getCanonicalCopiedDef(
-                      ownedForward->getOperand(0))
-                      ->getDefiningInstruction()) {
-            if (CanonicalizeBorrowScope::isRewritableOSSAForward(forwardDef)) {
-              defWorklist.ownedForwards.insert(forwardDef);
-            }
+  }
+  // borrowCanonicalizer performs all modifications through deleter's
+  // callbacks, so we don't need to explicitly check for changes.
+  CanonicalizeBorrowScope borrowCanonicalizer(deleter);
+  // The utilities in this loop cannot delete borrows before they are popped
+  // from the worklist.
+  while (true) {
+    while (!defWorklist.ownedForwards.empty()) {
+      assert(canonicalizeBorrows);
+
+      SILInstruction *ownedForward = defWorklist.ownedForwards.pop_back_val();
+      // Delete a dead forwarded value before sinking to avoid this pattern:
+      //   %outerVal = destructure_struct %def
+      //   destroy %outerVal           <= delete this destroy now
+      //   destroy %def                <= so we don't delete this one later
+      if (deleter.deleteIfDead(ownedForward)) {
+        LLVM_DEBUG(llvm::dbgs() << "  Deleted " << *ownedForward);
+        continue;
+      }
+      // Canonicalize a forwarded owned value before sinking the forwarding
+      // instruction, and sink the instruction before canonicalizing the owned
+      // value being forwarded. Process 'ownedForwards' in reverse since
+      // they may be chained, and CanonicalizeBorrowScopes pushes them
+      // top-down.
+      for (auto result : ownedForward->getResults()) {
+        canonicalizer.canonicalizeValueLifetime(result);
+      }
+      if (sinkOwnedForward(ownedForward, postOrderAnalysis, domTree)) {
+        changed = true;
+        // Sinking 'ownedForward' may create an opportunity to sink its
+        // operand. This handles chained forwarding instructions that were
+        // pushed onto the list out-of-order.
+        if (SILInstruction *forwardDef =
+                CanonicalizeOSSALifetime::getCanonicalCopiedDef(
+                    ownedForward->getOperand(0))
+                    ->getDefiningInstruction()) {
+          if (CanonicalizeBorrowScope::isRewritableOSSAForward(forwardDef)) {
+            defWorklist.ownedForwards.insert(forwardDef);
           }
         }
       }
-      if (defWorklist.borrowedValues.empty())
-        break;
+    }
+    if (defWorklist.borrowedValues.empty())
+      break;
 
-      BorrowedValue borrow(defWorklist.borrowedValues.pop_back_val());
-      borrowCanonicalizer.canonicalizeBorrowScope(borrow);
-      for (CopyValueInst *copy : borrowCanonicalizer.getUpdatedCopies()) {
-        defWorklist.updateForCopy(copy);
-      }
-      // Dead borrow scopes must be removed as uses before canonicalizing the
-      // outer copy.
-      if (auto *beginBorrow = dyn_cast<BeginBorrowInst>(borrow.value)) {
-        if (hasOnlyEndOfScopeOrEndOfLifetimeUses(beginBorrow)) {
-          deleter.recursivelyDeleteUsersIfDead(beginBorrow);
-        }
+    BorrowedValue borrow(defWorklist.borrowedValues.pop_back_val());
+    assert(canonicalizeBorrows || !borrow.isLocalScope());
+
+    borrowCanonicalizer.canonicalizeBorrowScope(borrow);
+    for (CopyValueInst *copy : borrowCanonicalizer.getUpdatedCopies()) {
+      defWorklist.updateForCopy(copy);
+    }
+    // Dead borrow scopes must be removed as uses before canonicalizing the
+    // outer copy.
+    if (auto *beginBorrow = dyn_cast<BeginBorrowInst>(borrow.value)) {
+      if (hasOnlyEndOfScopeOrEndOfLifetimeUses(beginBorrow)) {
+        deleter.recursivelyDeleteUsersIfDead(beginBorrow);
       }
     }
     deleter.cleanupDeadInstructions();
@@ -570,6 +576,8 @@ void CopyPropagation::run() {
   }
 }
 
+// MandatoryCopyPropagation is not currently enabled in the -Onone pipeline
+// because it may negatively affect the debugging experience.
 SILTransform *swift::createMandatoryCopyPropagation() {
   return new CopyPropagation(/*pruneDebug*/ true, /*canonicalizeAll*/ true,
                              /*canonicalizeBorrows*/ false,
@@ -581,4 +589,3 @@ SILTransform *swift::createCopyPropagation() {
                              /*canonicalizeBorrows*/ EnableRewriteBorrows,
                              /*poisonRefs*/ false);
 }
-

lib/SILOptimizer/Utils/CanonicalizeBorrowScope.cpp

@@ -209,6 +209,8 @@ SILValue CanonicalizeBorrowScope::findDefInBorrowScope(SILValue value) {
 ///
 /// \p innerValue is either the initial begin_borrow, or a forwarding operation
 /// within the borrow scope.
+///
+/// Note: This must always return true when innerValue is a function argument.
 template <typename Visitor>
 bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
                                                    Visitor &visitor) {
@@ -259,10 +261,16 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
       case OperandOwnership::ForwardingUnowned:
       case OperandOwnership::PointerEscape:
         // Pointer escapes are only allowed if they use the guaranteed value,
-        // which means that the escaped value must be confied to the current
-        // borrow scope.
-        if (use->get().getOwnershipKind() != OwnershipKind::Guaranteed)
+        // which means that the escaped value must be confined to the current
+        // borrow scope. visitBorrowScopeUses must never return false when
+        // borrowedValue is a SILFunctionArgument.
+        if (use->get().getOwnershipKind() != OwnershipKind::Guaranteed
+            && !isa<SILFunctionArgument>(borrowedValue.value)) {
+          return false;
+        }
+        if (!visitor.visitUse(use)) {
           return false;
+        }
         break;
 
       case OperandOwnership::ForwardingBorrow:
@@ -403,6 +411,8 @@ void CanonicalizeBorrowScope::filterOuterBorrowUseInsts(
 namespace {
 
 /// Remove redundant copies/destroys within a borrow scope.
+///
+/// The visitor callbacks must always return true since this rewrites in-place.
 class RewriteInnerBorrowUses {
   CanonicalizeBorrowScope &scope;
 
@@ -472,6 +482,8 @@ class RewriteInnerBorrowUses {
 /// visitor. They are rewritten separately.
 ///
 /// Implements visitBorrowScopeUses<Visitor>
+///
+/// The visitor callbacks must always return true since this rewrites in-place.
 class RewriteOuterBorrowUses {
   CanonicalizeBorrowScope &scope;
 
@@ -794,8 +806,9 @@ bool CanonicalizeBorrowScope::canonicalizeFunctionArgument(
 
   RewriteInnerBorrowUses innerRewriter(*this);
   beginVisitBorrowScopeUses(); // reset the def/use worklist
+
   bool succeed = visitBorrowScopeUses(borrowedValue.value, innerRewriter);
-  assert(succeed && "should be filtered by FindBorrowScopeUses");
+  assert(succeed && "must always succeed for function arguments");
   return true;
 }
 

test/SILOptimizer/assemblyvision_remark/basic.swift

@@ -142,13 +142,12 @@ public class SubKlass : Klass {
 }
 
 func lookThroughCast(x: SubKlass) -> Klass {
-    return x as Klass // expected-remark {{retain of type 'SubKlass'}}
-                      // expected-note @-2:22 {{of 'x'}}
+    return x as Klass // expected-remark {{retain of type 'Klass'}}
+                      // expected-note @-2:22 {{of 'x.upcast<$Klass>'}}
 }
 
 func lookThroughRefCast(x: Klass) -> SubKlass {
-    return x as! SubKlass // expected-remark {{retain of type 'Klass'}}
-                          // expected-note @-2:25 {{of 'x'}}
+    return x as! SubKlass // expected-remark {{retain of type 'SubKlass'}}
 }
 
 func lookThroughEnum(x: Klass?) -> Klass {

test/SILOptimizer/copy_propagation_borrow.sil

@@ -1,4 +1,5 @@
-// RUN: %target-sil-opt -copy-propagation -canonical-ossa-rewrite-borrows -enable-sil-verify-all %s | %FileCheck %s
+// RUN: %target-sil-opt -copy-propagation -canonical-ossa-rewrite-borrows -enable-sil-verify-all -module-name Swift %s | %FileCheck %s
+// RUN: %target-sil-opt -copy-propagation -enable-sil-verify-all -module-name Swift %s | %FileCheck %s --check-prefixes=CHECK-NOSCOPES
 //
 // Most CopyPropagation tests are still in copy_propagation_opaque.sil.
 //
@@ -12,8 +13,21 @@ import Builtin
 
 typealias AnyObject = Builtin.AnyObject
 
+protocol Error {}
+
 class B { }
 
+
+struct UInt64 {
+  @_hasStorage public var _value: Builtin.Int64 { get set }
+  init(_value: Builtin.Int64)
+}
+
+struct Int64 {
+  @_hasStorage public var _value: Builtin.Int64 { get set }
+  init(_value: Builtin.Int64)
+}
+
 class C {
   var a: Int64
 }
@@ -59,16 +73,6 @@ struct HasObjects {
   var object1: C
 }
 
-struct UInt64 {
-  @_hasStorage public var _value: Builtin.Int64 { get set }
-  init(_value: Builtin.Int64)
-}
-
-struct Int64 {
-  @_hasStorage public var _value: Builtin.Int64 { get set }
-  init(_value: Builtin.Int64)
-}
-
 internal struct _StringObject {
   @usableFromInline
   @_hasStorage internal var _countAndFlagsBits: UInt64 { get set }
@@ -1178,3 +1182,52 @@ bb0(%0 : @guaranteed $String):
   %5 = struct $String.UTF16View (%3 : $_StringGuts)
   return %5 : $String.UTF16View
 }
+
+// visitBorrowScopeUses first looks through this copy then sees a
+// unowned forwarding operation (ref_to_unmanaged). It generally
+// cannot prove that all uses of an unowned forward are within the
+// borrow scope. This is still safe, however, because the borrowed
+// value is a function argument. In fact, borrow scope rewriting
+// *must* succeed because it begins rewriting guaranteed function
+// arguments without checking the uses first.
+//
+// CHECK-NOSCOPES-LABEL: sil [ossa] @testEscapingGuaranteedToOwned : $@convention(method) (@guaranteed C) -> () {
+// CHECK-NOSCOPES: bb0(%0 : @guaranteed $C):
+// CHECK-NOSCOPES-NEXT:  %1 = ref_to_unmanaged %0 : $C to $@sil_unmanaged C
+// CHECK-NOSCOPES-NEXT:  tuple ()
+// CHECK-NOSCOPES-NEXT:  return
+// CHECK-NOSCOPES-LABEL: } // end sil function 'testEscapingGuaranteedToOwned'
+sil [ossa] @testEscapingGuaranteedToOwned : $@convention(method) (@guaranteed C) -> () {
+bb0(%0 : @guaranteed $C):
+  %1 = copy_value %0 : $C
+  %2 = ref_to_unmanaged %1 : $C to $@sil_unmanaged C
+  destroy_value %1 : $C
+  %7 = tuple ()
+  return %7 : $()
+}
+
+// visitBorrowScopeUses first looks through this copy, then a normal
+// forwarding operation (convert_function), then an unowned forwarding
+// operation (convert_escape_to_noescape). It must successfully
+// analyze and replace all uses because borrow scope rewriting begins
+// rewriting guaranteed function arguments without checking the uses
+// first.
+//
+// CHECK-NOSCOPES-LABEL: sil [ossa] @testEscapingEscapingCast : $@convention(method) (@guaranteed @callee_guaranteed (Int64) -> Int64) -> () {
+// CHECK-NOSCOPES:      bb0(%0 : @guaranteed $@callee_guaranteed (Int64) -> Int64):
+// CHECK-NOSCOPES-NEXT: %1 = convert_function %0 : $@callee_guaranteed (Int64) -> Int64 to $@callee_guaranteed (Int64) -> (Int64, @error Error) // user: %2
+// CHECK-NOSCOPES-NEXT: %2 = convert_escape_to_noescape %1 : $@callee_guaranteed (Int64) -> (Int64, @error Error) to $@noescape @callee_guaranteed (Int64) -> (Int64, @error Error)
+// CHECK-NOSCOPES-NEXT: = tuple ()
+// CHECK-NOSCOPES-NEXT: return
+// CHECK-NOSCOPES-LABEL: } // end sil function 'testEscapingEscapingCast'
+sil [ossa] @testEscapingEscapingCast : $@convention(method) (@guaranteed @callee_guaranteed (Int64) -> Int64) -> () {
+bb0(%0 : @guaranteed $@callee_guaranteed (Int64) -> Int64):
+  %1 = copy_value %0 : $@callee_guaranteed (Int64) -> Int64
+  %2 = convert_function %1 : $@callee_guaranteed (Int64) -> Int64 to $@callee_guaranteed (Int64) -> (Int64, @error Error)
+  %3 = copy_value %2 : $@callee_guaranteed (Int64) -> (Int64, @error Error)
+  %4 = convert_escape_to_noescape %3 : $@callee_guaranteed (Int64) -> (Int64, @error Error) to $@noescape @callee_guaranteed (Int64) -> (Int64, @error Error) // user: %29
+  destroy_value %3 : $@callee_guaranteed (Int64) -> (Int64, @error Error)
+  destroy_value %2 : $@callee_guaranteed (Int64) -> (Int64, @error Error)
+  %7 = tuple ()
+  return %7 : $()
+}