[OSSACanonicalizeGuaranteed] Don't rewrite consuming uses of move-only values.

lib/SILOptimizer/Utils/CanonicalizeBorrowScope.cpp

@@ -96,29 +96,29 @@ bool CanonicalizeBorrowScope::isRewritableOSSAForward(SILInstruction *inst) {
   if (inst->getNumOperands() != 1)
     return false;
 
-  if (isa<OwnershipForwardingSingleValueInstruction>(inst) ||
-      isa<OwnershipForwardingMultipleValueInstruction>(inst)) {
-    Operand *forwardedOper = &inst->getOperandRef(0);
-    // Trivial conversions do not need to be hoisted out of a borrow scope.
-    auto operOwnership = forwardedOper->getOperandOwnership();
-    if (operOwnership == OperandOwnership::TrivialUse)
-      return false;
-    // Don't mess with unowned conversions. They need to be copied immediately.
-    if (operOwnership != OperandOwnership::GuaranteedForwarding &&
-        operOwnership != OperandOwnership::ForwardingConsume) {
-      return false;
-    }
-    assert(operOwnership == OperandOwnership::GuaranteedForwarding ||
-           operOwnership == OperandOwnership::ForwardingConsume);
-
-    // Filter instructions that belong to a Forwarding*ValueInst mixin but
-    // cannot be converted to forward owned value (struct_extract).
-    if (!canOpcodeForwardOwnedValues(forwardedOper))
-      return false;
+  if (!isa<OwnershipForwardingSingleValueInstruction>(inst) &&
+      !isa<OwnershipForwardingMultipleValueInstruction>(inst))
+    return false;
 
-    return true;
+  Operand *forwardedOper = &inst->getOperandRef(0);
+  // Trivial conversions do not need to be hoisted out of a borrow scope.
+  auto operOwnership = forwardedOper->getOperandOwnership();
+  if (operOwnership == OperandOwnership::TrivialUse)
+    return false;
+  // Don't mess with unowned conversions. They need to be copied immediately.
+  if (operOwnership != OperandOwnership::GuaranteedForwarding &&
+      operOwnership != OperandOwnership::ForwardingConsume) {
+    return false;
   }
-  return false;
+  assert(operOwnership == OperandOwnership::GuaranteedForwarding ||
+         operOwnership == OperandOwnership::ForwardingConsume);
+
+  // Filter instructions that belong to a Forwarding*ValueInst mixin but
+  // cannot be converted to forward owned value (struct_extract).
+  if (!canOpcodeForwardOwnedValues(forwardedOper))
+    return false;
+
+  return true;
 }
 
 /// Return the root of a borrowed extended lifetime for \p def or invalid.
@@ -218,8 +218,6 @@ 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) {
@@ -277,14 +275,12 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
       case OperandOwnership::PointerEscape:
         // Pointer escapes are only allowed if they use the guaranteed value,
         // which means that the escaped value must be confined to the current
-        // borrow scope. visitBorrowScopeUses must never return false when
-        // borrowedValue is a SILFunctionArgument.
+        // borrow scope.
         if (use->get()->getOwnershipKind() != OwnershipKind::Guaranteed &&
             !isa<SILFunctionArgument>(borrowedValue.value)) {
           return false;
         }
         if (!visitor.visitUse(use)) {
-          assert(!isa<SILFunctionArgument>(borrowedValue.value));
           return false;
         }
         break;
@@ -293,7 +289,6 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
       case OperandOwnership::ForwardingConsume:
         if (CanonicalizeBorrowScope::isRewritableOSSAForward(user)) {
           if (!visitor.visitForwardingUse(use)) {
-            assert(!isa<SILFunctionArgument>(borrowedValue.value));
             return false;
           }
           break;
@@ -306,7 +301,6 @@ bool CanonicalizeBorrowScope::visitBorrowScopeUses(SILValue innerValue,
       case OperandOwnership::BitwiseEscape:
       case OperandOwnership::DestroyingConsume:
         if (!visitor.visitUse(use)) {
-          assert(!isa<SILFunctionArgument>(borrowedValue.value));
           return false;
         }
         break;
@@ -394,7 +388,7 @@ class FindBorrowScopeUses {
 
 } // namespace
 
-/// Erase users from \p outerUseInsts that are not within the borrow scope.
+/// Erase users from \p outerUseInsts that are actually within the borrow scope.
 void CanonicalizeBorrowScope::filterOuterBorrowUseInsts(
     OuterUsers &outerUseInsts) {
   auto *beginBorrow = cast<BeginBorrowInst>(borrowedValue.value);
@@ -451,6 +445,10 @@ class RewriteInnerBorrowUses {
     }
     SILValue def = scope.findDefInBorrowScope(value);
     if (use->isConsuming()) {
+      if (use->get()->getType().isMoveOnly()) {
+        // Can't produce a copy of a non-copyable value on demand.  Bail out.
+        return false;
+      }
       // All in-scope consuming uses need a unique copy in the same block.
       auto *copy = dyn_cast<CopyValueInst>(use->get());
       if (copy && copy->hasOneUse() && copy->getParent() == user->getParent()) {
@@ -480,7 +478,9 @@ class RewriteInnerBorrowUses {
       if (!hasValueOwnership(result)) {
         continue;
       }
-      scope.visitBorrowScopeUses(result, *this);
+      if (!scope.visitBorrowScopeUses(result, *this)) {
+        return false;
+      }
     }
     // Update this operand bypassing any copies.
     SILValue value = use->get();
@@ -796,9 +796,7 @@ bool CanonicalizeBorrowScope::consolidateBorrowScope() {
   if (outerUseInsts.empty()) {
     RewriteInnerBorrowUses innerRewriter(*this);
     beginVisitBorrowScopeUses(); // reset the def/use worklist
-    bool succeed = visitBorrowScopeUses(borrowedValue.value, innerRewriter);
-    assert(succeed && "should be filtered by FindBorrowScopeUses");
-    return true;
+    return visitBorrowScopeUses(borrowedValue.value, innerRewriter);
   }
   LLVM_DEBUG(llvm::dbgs() << "  Outer uses:\n";
              for (SILInstruction *inst
@@ -826,11 +824,25 @@ bool CanonicalizeBorrowScope::canonicalizeFunctionArgument(
   RewriteInnerBorrowUses innerRewriter(*this);
   beginVisitBorrowScopeUses(); // reset the def/use worklist
 
-  bool succeed = visitBorrowScopeUses(borrowedValue.value, innerRewriter);
-  assert(succeed && "must always succeed for function arguments");
-  return true;
+  return visitBorrowScopeUses(borrowedValue.value, innerRewriter);
 }
 
+namespace swift::test {
+// Arguments:
+// - SILFunctionArgument: function argument to canonicalize
+// Dumps:
+// - function after argument canonicalization
+static FunctionTest CanonicalizeFunctionArgumentTest(
+    "canonicalize_function_argument",
+    [](auto &function, auto &arguments, auto &test) {
+      auto *argument = cast<SILFunctionArgument>(arguments.takeBlockArgument());
+      InstructionDeleter deleter;
+      CanonicalizeBorrowScope canonicalizer(&function, deleter);
+      canonicalizer.canonicalizeFunctionArgument(argument);
+      function.print(llvm::outs());
+    });
+} // end namespace swift::test
+
 /// Canonicalize a worklist of extended lifetimes. This iterates after rewriting
 /// borrow scopes to handle new outer copies and new owned lifetimes from
 /// forwarding operations.

test/SILOptimizer/canonicalize_function_argument_unit.sil

@@ -0,0 +1,79 @@
+// RUN: %target-sil-opt \
+// RUN:     -test-runner \
+// RUN:     -module-name Swift \
+// RUN:     %s \
+// RUN:     -o /dev/null \
+// RUN: 2>&1 | %FileCheck %s
+
+import Builtin
+
+@_marker protocol Copyable {}
+
+class C {}
+struct NC : ~Copyable {
+  var c: C
+}
+
+// CHECK-LABEL: begin running test {{.*}} on consume_move_only
+// CHECK-LABEL: sil [ossa] @consume_move_only : {{.*}} {
+// CHECK:       bb0([[C:%[^,]+]] :
+//                Necessary copy.
+// CHECK:         [[COPY:%[^,]+]] = copy_value [[C]]
+// CHECK:         [[NC:%[^,]+]] = struct $NC ([[COPY]]
+// CHECK:         apply undef([[NC]])
+// CHECK-LABEL: } // end sil function 'consume_move_only'
+// CHECK-LABEL: end running test {{.*}} on consume_move_only
+sil [ossa] @consume_move_only : $@convention(thin) (@guaranteed C) -> () {
+bb0(%c : @guaranteed $C):
+  specify_test "canonicalize_function_argument @argument"
+  %copy = copy_value %c
+  %nc = struct $NC (%copy)
+  apply undef(%nc) : $@convention(thin) (@owned NC) -> ()
+  %retval = tuple ()
+  return %retval
+}
+
+// CHECK-LABEL: begin running test {{.*}} on borrow_move_only
+// CHECK-LABEL: sil [ossa] @borrow_move_only : {{.*}} {
+// CHECK:       bb0([[C:%[^,]+]] :
+//                No copy!
+// CHECK:         [[NC:%[^,]+]] = struct $NC ([[C]]
+// CHECK:         apply undef([[NC]])
+// CHECK-LABEL: } // end sil function 'borrow_move_only'
+// CHECK-LABEL: end running test {{.*}} on borrow_move_only
+sil [ossa] @borrow_move_only : $@convention(thin) (@guaranteed C) -> () {
+bb0(%c : @guaranteed $C):
+  specify_test "canonicalize_function_argument @argument"
+  %copy = copy_value %c
+  %nc = struct $NC (%copy)
+  apply undef(%nc) : $@convention(thin) (@guaranteed NC) -> ()
+  destroy_value %nc
+  %retval = tuple ()
+  return %retval
+}
+
+// CHECK-LABEL: begin running test {{.*}} on consume_and_borrow_move_only
+// CHECK-LABEL: sil [ossa] @consume_and_borrow_move_only : {{.*}} {
+// CHECK:       bb0([[C:%[^,]+]] :
+//                No copy!
+// CHECK:         [[NC1:%[^,]+]] = struct $NC ([[C]]
+// CHECK:         apply undef([[NC1]])
+//                Necessary copy.
+// CHECK:         [[COPY2:%[^,]+]] = copy_value [[C]]
+// CHECK:         [[NC2:%[^,]+]] = struct $NC ([[COPY2]]
+// CHECK:         apply undef([[NC2]])
+// CHECK-LABEL: } // end sil function 'consume_and_borrow_move_only'
+// CHECK-LABEL: end running test {{.*}} on consume_and_borrow_move_only
+sil [ossa] @consume_and_borrow_move_only : $@convention(thin) (@guaranteed C) -> () {
+bb0(%c : @guaranteed $C):
+  specify_test "canonicalize_function_argument @argument"
+  %copy1 = copy_value %c
+  %nc1 = struct $NC (%copy1)
+  apply undef(%nc1) : $@convention(thin) (@guaranteed NC) -> ()
+  destroy_value %nc1
+  %copy2 = copy_value %c
+  %nc2 = struct $NC (%copy2)
+  apply undef(%nc2) : $@convention(thin) (@owned NC) -> ()
+  %retval = tuple ()
+  return %retval
+}