Merge pull request #60047 from gottesmm/plus_zero_fix_noimplicitcopy

[no-implicit-copy] Use default param convention instead of forcing +1

Author: gottesmm

docs/SIL.rst

@@ -2570,6 +2570,198 @@ Consider the following legal SIL where we leak ``%0`` in blocks prefixed with
     return %1 : $Klass
   }
 
+Move Only Wrapped Types
+-----------------------
+
+Semantics
+~~~~~~~~~
+
+Today SIL supports values of move only wrapped type. Given a copyable type
+``$T``, the type ``$@moveOnly T`` is the move only wrapped variant of the
+type. This type is always non-trivial even if ``$T`` itself is trivial (see
+discussion below).  All move only wrapped types obey the invariant that they can
+be copied in Raw SIL but cannot be copied in Canonical SIL and later SIL
+stages. This ensures that once we are in Canonical SIL such values are "move
+only values" that are guaranteed to never be copied. This is enforced by:
+
+* Having SILGen emit copies as it normally does.
+
+* Use OSSA canonicalization to eliminate copies that aren't needed semantically
+  due to consuming uses of the value. This is implemented by the pass guaranteed
+  pass "MoveOnlyChecker". Emit errors on any of the consuming uses that we found
+  in said pass.
+
+Assuming that no errors are emitted, we can then conclude before we reach
+canonical SIL that the value was never copied and thus is a "move only value"
+even though the actual underlying wrapped type is copyable. As an example of
+this, consider the following Swift::
+
+  func doSomething(@_noImplicitCopy _ x: Klass) -> () { // expected-error {{'x' has guaranteed ownership but was consumed}}
+    x.doSomething()
+    let x2 = x // expected-note {{consuming use}}
+    x2.doSomething()
+  }
+
+which codegens to the following SIL::
+
+  sil hidden [ossa] @doSomething : $@convention(thin) (@guaranteed Klass) -> () {
+  bb0(%0 : @noImplicitCopy $Klass):
+    %1 = copyable_to_moveonlywrapper [guaranteed] %0 : $@moveOnly Klass
+    %2 = copy_value %1 : $@moveOnly Klass
+    %3 = mark_must_check [no_copy] %2 : $@moveOnly Klass
+    debug_value %3 : $@moveOnly Klass, let, name "x", argno 1
+    %4 = begin_borrow %3 : $@moveOnly Klass
+    %5 = function_ref @$s4test5KlassC11doSomethingyyF : $@convention(method) (@guaranteed Klass) -> ()
+    %6 = moveonlywrapper_to_copyable [guaranteed] %4 : $@moveOnly Klass
+    %7 = apply %5(%6) : $@convention(method) (@guaranteed Klass) -> ()
+    end_borrow %4 : $@moveOnly Klass
+    %9 = begin_borrow %3 : $@moveOnly Klass
+    %10 = copy_value %9 : $@moveOnly Klass
+    %11 = moveonlywrapper_to_copyable [owned] %10 : $@moveOnly Klass
+    %12 = begin_borrow [lexical] %11 : $Klass
+    debug_value %12 : $Klass, let, name "x2"
+    end_borrow %9 : $@moveOnly Klass
+    %15 = function_ref @$s4test5KlassC11doSomethingyyF : $@convention(method) (@guaranteed Klass) -> ()
+    %16 = apply %15(%12) : $@convention(method) (@guaranteed Klass) -> ()
+    end_borrow %12 : $Klass
+    destroy_value %11 : $Klass
+    destroy_value %3 : $@moveOnly Klass
+    %20 = tuple ()
+    return %20 : $()
+  } // end sil function '$s4test11doSomethingyyAA5KlassCF'
+
+When the move only checker runs upon this SIL, it will see that the
+``moveonlywrapped_to_copyable [owned]`` is a transitive consuming use of ``%2``
+(ignoring copies) and that we have a non-consuming use later. So it will emit an
+error that we have a guaranteed parameter that is being consumed. If we remove
+the assignment to ``x2``, then after the move checker runs successfully we would
+get the following SIL::
+
+  sil hidden [ossa] @doSomething : $@convention(thin) (@guaranteed Klass) -> () {
+  bb0(%0 : @noImplicitCopy $Klass):
+    %1 = copyable_to_moveonlywrapper [guaranteed] %0 : $Klass
+    debug_value %1 : $@moveOnly Klass, let, name "x", argno 1
+    %4 = begin_borrow %1 : $@moveOnly Klass
+    %5 = function_ref @$s4test5KlassC11doSomethingyyF : $@convention(method) (@guaranteed Klass) -> ()
+    %6 = moveonlywrapper_to_copyable [guaranteed] %4 : $@moveOnly Klass
+    %7 = apply %5(%6) : $@convention(method) (@guaranteed Klass) -> ()
+    end_borrow %4 : $@moveOnly Klass
+    %20 = tuple ()
+    return %20 : $()
+  } // end sil function '$s4test11doSomethingyyAA5KlassCF'
+
+yielding SIL without any copies just as we wanted.
+
+At the ABI level, ``@moveOnly`` does not exist and thus never shows up in a
+SILFunctionType's parameters. Instead, we represent it only in the body of
+functions and on a function's internal SILArgument representation. This is since
+move only wrapped is intended to be a local power tool for controlling lifetimes
+rather than a viral type level annotation that would constrain the type system.
+
+As mentioned above trivial move only wrapped types are actually
+non-trivial. This is because in SIL ownership is tied directly to
+non-trivialness so unless we did that we could not track ownership
+accurately. This is loss of triviality is not an issue for most of the pipeline
+since we eliminate all move only wrapper types for trivial types during the
+guaranteed optimizations after we have run various ownership checkers but before
+we have run diagnostics for trivial types (e.x.: DiagnosticConstantPropagation).
+
+As an example in practice, consider the following Swift::
+
+  func doSomethingWithInt(@_noImplicitCopy _ x: Int) -> Int {
+    x + x
+  }
+
+Today this codegens to the following Swift::
+
+  sil hidden [ossa] @doSomethingWithInt : $@convention(thin) (Int) -> Int {
+  bb0(%0 : @noImplicitCopy $Int):
+    %1 = copyable_to_moveonlywrapper [owned] %0 : $Int
+    %2 = move_value [lexical] %1 : $@moveOnly Int
+    %3 = mark_must_check [no_implicit_copy] %2 : $@moveOnly Int
+    %5 = begin_borrow %3 : $@moveOnly Int
+    %6 = begin_borrow %3 : $@moveOnly Int
+    %7 = function_ref @addIntegers : $@convention(method) (Int, Int Int.Type) -> Int
+    %8 = moveonlywrapper_to_copyable [guaranteed] %5 : $@moveOnly Int
+    %9 = moveonlywrapper_to_copyable [guaranteed] %6 : $@moveOnly Int
+    %10 = apply %7(%8, %9) : $@convention(method) (Int, Int) -> Int
+    end_borrow %6 : $@moveOnly Int
+    end_borrow %5 : $@moveOnly Int
+    destroy_value %3 : $@moveOnly Int
+    return %10 : $Int
+  }
+
+once the checker has run, this becomes::
+
+  sil hidden [ossa] @doSomethingWithInt : $@convention(thin) (Int) -> Int {
+  bb0(%0 : @noImplicitCopy $Int):
+    %1 = copyable_to_moveonlywrapper [owned] %0 : $Int
+    %2 = move_value [lexical] %1 : $@moveOnly Int
+    %5 = begin_borrow %2 : $@moveOnly Int
+    %6 = begin_borrow %2 : $@moveOnly Int
+    %7 = function_ref @addIntegers : $@convention(method) (Int, Int Int.Type) -> Int
+    %8 = moveonlywrapper_to_copyable [guaranteed] %5 : $@moveOnly Int
+    %9 = moveonlywrapper_to_copyable [guaranteed] %6 : $@moveOnly Int
+    %10 = apply %7(%8, %9) : $@convention(method) (Int, Int) -> Int
+    end_borrow %6 : $@moveOnly Int
+    end_borrow %5 : $@moveOnly Int
+    return %10 : $Int
+  }
+
+and once we have run the move only wrapped type lowerer, we get the following
+SIL::
+
+  sil hidden [ossa] @doSomethingWithInt : $@convention(thin) (Int) -> Int {
+  bb0(%0 : @noImplicitCopy $Int):
+    %1 = function_ref @addIntegers : $@convention(method) (Int, Int) -> Int
+    %2 = apply %1(%0, %0) : $@convention(method) (Int, Int) -> Int
+    return %2 : $Int
+  }
+
+exactly what we wanted in the end.
+
+If we are given an owned argument or a let binding, we use a similar
+approach. Consider the following Swift::
+
+  func doSomethingWithKlass(_ x: Klass) -> Klass {
+    @_noImplicitCopy let value = x
+    let value2 = value
+    return value
+  }
+
+A hypothetical SILGen for this code is as follows::
+  
+  sil hidden [ossa] @$s4test20doSomethingWithKlassyAA0E0CADF : $@convention(thin) (@guaranteed Klass) -> @owned Klass {
+  bb0(%0 : @guaranteed $Klass):
+    debug_value %0 : $Klass, let, name "x", argno 1
+    %3 = begin_borrow [lexical] %0 : $Klass
+    %4 = copy_value %3 : $Klass
+    %5 = copyable_to_moveonlywrapper [owned] %4 : $Klass
+    %6 = mark_must_check [no_implicit_copy] %5 : $@moveOnly Klass
+    debug_value %6 : $@moveOnly Klass, let, name "value"
+    %8 = begin_borrow %6 : $@moveOnly Klass
+    %9 = copy_value %8 : $@moveOnly Klass
+    %10 = moveonlywrapper_to_copyable [owned] %9 : $@moveOnly Klass
+    %11 = begin_borrow [lexical] %10 : $Klass
+    debug_value %11 : $Klass, let, name "value2"
+    end_borrow %8 : $@moveOnly Klass
+    %14 = begin_borrow %6 : $@moveOnly Klass
+    %15 = copy_value %14 : $@moveOnly Klass
+    end_borrow %14 : $@moveOnly Klass
+    end_borrow %11 : $Klass
+    destroy_value %10 : $Klass
+    destroy_value %6 : $@moveOnly Klass
+    end_borrow %3 : $Klass
+    %22 = moveonlywrapper_to_copyable [owned] %15 : $@moveOnly Klass
+    return %22 : $Klass
+  } // end sil function '$s4test20doSomethingWithKlassyAA0E0CADF'
+
+Notice above how the ``moveonlywrapper_to_copyable [owned]`` is used to escape
+the no implicit copy value. In fact, if one runs the following SILGen through
+sil-opt, one will see that we actually have an ownership violation due to the
+two uses of "value", one for initializing value2 and the other for the return
+value.
+
 Runtime Failure
 ---------------
 
@@ -7456,14 +7648,20 @@ mark_must_check
                       '[' sil-optimizer-analysis-marker ']'
 
   sil-optimizer-analysis-marker ::= 'no_implicit_copy'
+                                ::= 'no_copy'
 
 A canary value inserted by a SIL generating frontend to signal to the move
 checker to check a specific value.  Valid only in Raw SIL. The relevant checkers
 should remove the `mark_must_check`_ instruction after successfully running the
 relevant diagnostic. The idea here is that instead of needing to introduce
-multiple "flaging" instructions for the optimizer, we can just reuse this one
+multiple "flagging" instructions for the optimizer, we can just reuse this one
 instruction by varying the kind.
 
+If the sil optimizer analysis marker is ``no_implicit_copy`` then the move
+checker is told to check that the result of this instruction is consumed at most
+once. If the marker is ``no_copy``, then the move checker will validate that the
+result of this instruction is never consumed.
+
 No Implicit Copy and No Escape Value Instructions
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

include/swift/AST/DiagnosticsSIL.def

@@ -725,8 +725,10 @@ ERROR(noimplicitcopy_used_on_generic_or_existential, none,
       "binding or a nominal type containing such typed things", ())
 
 // move only checker diagnostics
-ERROR(sil_moveonlychecker_value_consumed_more_than_once, none,
+ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
       "'%0' consumed more than once", (StringRef))
+ERROR(sil_moveonlychecker_guaranteed_value_consumed, none,
+      "'%0' has guaranteed ownership but was consumed", (StringRef))
 NOTE(sil_moveonlychecker_consuming_use_here, none,
      "consuming use", ())
 ERROR(sil_moveonlychecker_not_understand_mark_move, none,

include/swift/SIL/SILBuilder.h

@@ -1291,9 +1291,20 @@ class SILBuilder {
   }
 
   CopyableToMoveOnlyWrapperValueInst *
-  createCopyableToMoveOnlyWrapperValue(SILLocation loc, SILValue src) {
+  createOwnedCopyableToMoveOnlyWrapperValue(SILLocation loc, SILValue src) {
     return insert(new (getModule()) CopyableToMoveOnlyWrapperValueInst(
-        getSILDebugLocation(loc), src));
+        getSILDebugLocation(loc), src,
+        CopyableToMoveOnlyWrapperValueInst::Owned));
+  }
+
+  CopyableToMoveOnlyWrapperValueInst *
+  createGuaranteedCopyableToMoveOnlyWrapperValue(SILLocation loc,
+                                                 SILValue src) {
+    assert(!src->getType().isTrivial(*F) &&
+           "trivial types can only use the owned version of this API");
+    return insert(new (getModule()) CopyableToMoveOnlyWrapperValueInst(
+        getSILDebugLocation(loc), src,
+        CopyableToMoveOnlyWrapperValueInst::Guaranteed));
   }
 
   MoveOnlyWrapperToCopyableValueInst *

include/swift/SIL/SILCloner.h

@@ -1788,11 +1788,18 @@ void SILCloner<ImplClass>::visitMoveOnlyWrapperToCopyableValueInst(
 
 template <typename ImplClass>
 void SILCloner<ImplClass>::visitCopyableToMoveOnlyWrapperValueInst(
-    CopyableToMoveOnlyWrapperValueInst *Inst) {
-  getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
-  auto *MVI = getBuilder().createCopyableToMoveOnlyWrapperValue(
-      getOpLocation(Inst->getLoc()), getOpValue(Inst->getOperand()));
-  recordClonedInstruction(Inst, MVI);
+    CopyableToMoveOnlyWrapperValueInst *inst) {
+  getBuilder().setCurrentDebugScope(getOpScope(inst->getDebugScope()));
+  CopyableToMoveOnlyWrapperValueInst *cvt;
+  if (inst->getOwnershipKind() == OwnershipKind::Owned) {
+    cvt = getBuilder().createOwnedCopyableToMoveOnlyWrapperValue(
+        getOpLocation(inst->getLoc()), getOpValue(inst->getOperand()));
+  } else {
+    assert(inst->getOwnershipKind() == OwnershipKind::Guaranteed);
+    cvt = getBuilder().createGuaranteedCopyableToMoveOnlyWrapperValue(
+        getOpLocation(inst->getLoc()), getOpValue(inst->getOperand()));
+  }
+  recordClonedInstruction(inst, cvt);
 }
 
 template <typename ImplClass>

include/swift/SIL/SILInstruction.h

@@ -1315,6 +1315,7 @@ FirstArgOwnershipForwardingSingleValueInst::classof(SILInstructionKind kind) {
   case SILInstructionKind::InitExistentialRefInst:
   case SILInstructionKind::MarkDependenceInst:
   case SILInstructionKind::MoveOnlyWrapperToCopyableValueInst:
+  case SILInstructionKind::CopyableToMoveOnlyWrapperValueInst:
     return true;
   default:
     return false;
@@ -7565,7 +7566,16 @@ class MarkMustCheckInst
 public:
   enum class CheckKind : unsigned {
     Invalid = 0,
+
+    // A signal to the move only checker to perform no implicit copy checking on
+    // the result of this instruction. This implies that the result can be
+    // consumed at most once.
     NoImplicitCopy,
+
+    // A signal to the move only checker ot perform no copy checking. This
+    // forces the result of this instruction owned value to never be consumed
+    // (still allowing for non-consuming uses of course).
+    NoCopy,
   };
 
 private:
@@ -7584,19 +7594,61 @@ class MarkMustCheckInst
 public:
   CheckKind getCheckKind() const { return kind; }
 
-  bool isNoImplicitCopy() const { return kind == CheckKind::NoImplicitCopy; }
+  bool hasMoveCheckerKind() const {
+    switch (kind) {
+    case CheckKind::Invalid:
+      return false;
+    case CheckKind::NoImplicitCopy:
+    case CheckKind::NoCopy:
+      return true;
+    }
+  }
 };
 
+/// Convert from a non-trivial copyable type to an `@moveOnly` wrapper type.
+///
+/// IMPORTANT: Unlike other forwarding instructions, the ownership of
+/// copyable_to_moveonly is not decided by the operand passed in on
+/// construction. Instead in SILBuilder one must select the specific type of
+/// ownership one wishes by using the following APIs:
+///
+/// * SILBuilder::createOwnedCopyableToMoveOnlyWrapperValueInst
+/// * SILBuilder::createGuaranteedCopyableToMoveOnlyWrapperInst
+///
+/// The reason why this instruction was designed in this manner is that a
+/// frontend chooses the ownership form of this instruction based off of the
+/// semantic place that the value is used. Specifically:
+///
+/// 1. When creating a moveOnly wrapped value for an owned argument or a value,
+/// we use the owned variant.
+///
+/// 2. When creating a moveOnly wrapped value from a guaranteed argument, we use
+/// the guaranteed variant.
 class CopyableToMoveOnlyWrapperValueInst
     : public UnaryInstructionBase<
           SILInstructionKind::CopyableToMoveOnlyWrapperValueInst,
-          SingleValueInstruction> {
+          FirstArgOwnershipForwardingSingleValueInst> {
+public:
+  enum InitialKind {
+    Guaranteed,
+    Owned,
+  };
+
+private:
   friend class SILBuilder;
 
+  InitialKind initialKind;
+
   CopyableToMoveOnlyWrapperValueInst(SILDebugLocation DebugLoc,
-                                     SILValue operand)
-      : UnaryInstructionBase(DebugLoc, operand,
-                             operand->getType().addingMoveOnlyWrapper()) {}
+                                     SILValue operand, InitialKind kind)
+      : UnaryInstructionBase(
+            DebugLoc, operand, operand->getType().addingMoveOnlyWrapper(),
+            kind == InitialKind::Guaranteed ? OwnershipKind::Guaranteed
+                                            : OwnershipKind::Owned),
+        initialKind(kind) {}
+
+public:
+  InitialKind getInitialKind() const { return initialKind; }
 };
 
 /// Convert from an @moveOnly wrapper type to the underlying copyable type. Can
@@ -9824,6 +9876,8 @@ OwnershipForwardingMixin::get(SILInstruction *inst) {
     return result;
   if (auto *result = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(inst))
     return result;
+  if (auto *result = dyn_cast<CopyableToMoveOnlyWrapperValueInst>(inst))
+    return result;
   return nullptr;
 }
 

include/swift/SIL/SILNodes.def

@@ -474,7 +474,9 @@ ABSTRACT_VALUE_AND_INST(SingleValueInstruction, ValueBase, SILInstruction)
   SINGLE_VALUE_INST(MarkMustCheckInst, mark_must_check,
                     SingleValueInstruction, None, DoesNotRelease)
   // Convert a $T to $@moveOnly T. This is the method that one uses to convert a
-  // trivial value to a non-trivial move only value.
+  // trivial value to a non-trivial move only value. Ownership is fixed at construction by
+  // frontend to express specific semantics: guaranteed for guaranteed function arguments
+  // and owned for assignment/owned function arguments.
   SINGLE_VALUE_INST(CopyableToMoveOnlyWrapperValueInst, copyable_to_moveonlywrapper,
                     SingleValueInstruction, None, DoesNotRelease)
   // Convert a $@moveOnly T to $T. Ownership is fixed at construction by