Merge pull request #68024 from gottesmm/pr-41b9d05e5b76061c0db11b5507cf80af25fc6dbe

[silgen] Hide ManagedValue::forUnmanaged and rewrite all uses to use more specific APIs

Author: gottesmm

include/swift/SIL/SILBuilder.h

@@ -551,6 +551,13 @@ class SILBuilder {
       PartialApplyInst::OnStackKind OnStack =
           PartialApplyInst::OnStackKind::NotOnStack,
       const GenericSpecializationInformation *SpecializationInfo = nullptr) {
+    assert(OnStack == PartialApplyInst::OnStackKind::OnStack ||
+           llvm::all_of(Args,
+                        [](SILValue value) {
+                          return value->getOwnershipKind().isCompatibleWith(
+                              OwnershipKind::Owned);
+                        }) &&
+               "Must have an owned compatible object");
     return insert(PartialApplyInst::create(
         getSILDebugLocation(Loc), Fn, Args, Subs, CalleeConvention, *F,
         SpecializationInfo, OnStack));
@@ -900,6 +907,8 @@ class SILBuilder {
   EndBorrowInst *createEndBorrow(SILLocation loc, SILValue borrowedValue) {
     assert(!SILArgument::isTerminatorResult(borrowedValue) &&
                "terminator results do not have end_borrow");
+    assert(!isa<SILFunctionArgument>(borrowedValue) &&
+           "Function arguments should never have an end_borrow");
     return insert(new (getModule())
                       EndBorrowInst(getSILDebugLocation(loc), borrowedValue));
   }

lib/SILGen/Cleanup.cpp

@@ -401,7 +401,9 @@ ManagedValue CleanupCloner::clone(SILValue value) const {
   }
 
   if (!hasCleanup) {
-    return ManagedValue::forUnmanaged(value);
+    if (value->getOwnershipKind().isCompatibleWith(OwnershipKind::Owned))
+      return ManagedValue::forUnmanagedOwnedValue(value);
+    return ManagedValue::forBorrowedRValue(value);
   }
 
   if (writebackBuffer.has_value()) {
@@ -436,19 +438,19 @@ CleanupCloner::cloneForTuplePackExpansionComponent(SILValue tupleAddr,
   }
 
   if (!hasCleanup) {
-    return ManagedValue::forUnmanaged(tupleAddr);
+    return ManagedValue::forBorrowedAddressRValue(tupleAddr);
   }
 
   assert(!writebackBuffer.has_value());
   auto expansionTy = tupleAddr->getType().getTupleElementType(componentIndex);
   if (expansionTy.getPackExpansionPatternType().isTrivial(SGF.F))
-    return ManagedValue::forUnmanaged(tupleAddr);
+    return ManagedValue::forTrivialAddressRValue(tupleAddr);
 
   auto cleanup =
     SGF.enterPartialDestroyRemainingTupleCleanup(tupleAddr, inducedPackType,
                                                  componentIndex,
                                                  /*start at */ SILValue());
-  return ManagedValue(tupleAddr, cleanup);
+  return ManagedValue::forOwnedAddressRValue(tupleAddr, cleanup);
 }
 
 ManagedValue
@@ -460,19 +462,19 @@ CleanupCloner::cloneForPackPackExpansionComponent(SILValue packAddr,
   }
 
   if (!hasCleanup) {
-    return ManagedValue::forUnmanaged(packAddr);
+    return ManagedValue::forBorrowedAddressRValue(packAddr);
   }
 
   assert(!writebackBuffer.has_value());
   auto expansionTy = packAddr->getType().getPackElementType(componentIndex);
   if (expansionTy.getPackExpansionPatternType().isTrivial(SGF.F))
-    return ManagedValue::forUnmanaged(packAddr);
+    return ManagedValue::forTrivialAddressRValue(packAddr);
 
   auto cleanup =
     SGF.enterPartialDestroyRemainingPackCleanup(packAddr, formalPackType,
                                                 componentIndex,
                                                 /*start at */ SILValue());
-  return ManagedValue(packAddr, cleanup);
+  return ManagedValue::forOwnedAddressRValue(packAddr, cleanup);
 }
 
 ManagedValue
@@ -484,7 +486,7 @@ CleanupCloner::cloneForRemainingPackComponents(SILValue packAddr,
   }
 
   if (!hasCleanup) {
-    return ManagedValue::forUnmanaged(packAddr);
+    return ManagedValue::forBorrowedAddressRValue(packAddr);
   }
 
   assert(!writebackBuffer.has_value());
@@ -498,12 +500,12 @@ CleanupCloner::cloneForRemainingPackComponents(SILValue packAddr,
   }
 
   if (isTrivial)
-    return ManagedValue::forUnmanaged(packAddr);
+    return ManagedValue::forTrivialAddressRValue(packAddr);
 
   auto cleanup =
     SGF.enterDestroyRemainingPackComponentsCleanup(packAddr, formalPackType,
                                                    firstComponentIndex);
-  return ManagedValue(packAddr, cleanup);
+  return ManagedValue::forOwnedAddressRValue(packAddr, cleanup);
 }
 
 ManagedValue
@@ -515,7 +517,7 @@ CleanupCloner::cloneForRemainingTupleComponents(SILValue tupleAddr,
   }
 
   if (!hasCleanup) {
-    return ManagedValue::forUnmanaged(tupleAddr);
+    return ManagedValue::forBorrowedAddressRValue(tupleAddr);
   }
 
   assert(!writebackBuffer.has_value());
@@ -529,11 +531,11 @@ CleanupCloner::cloneForRemainingTupleComponents(SILValue tupleAddr,
   }
 
   if (isTrivial)
-    return ManagedValue::forUnmanaged(tupleAddr);
+    return ManagedValue::forTrivialAddressRValue(tupleAddr);
 
   auto cleanup =
     SGF.enterDestroyRemainingTupleElementsCleanup(tupleAddr,
                                                   inducedPackType,
                                                   firstComponentIndex);
-  return ManagedValue(tupleAddr, cleanup);
+  return ManagedValue::forOwnedAddressRValue(tupleAddr, cleanup);
 }

lib/SILGen/Initialization.h

@@ -323,7 +323,8 @@ class TemporaryInitialization : public SingleBufferInitialization {
   CleanupHandle getInitializedCleanup() const { return Cleanup; }
 
   ManagedValue getManagedAddress() const  {
-    return ManagedValue(getAddress(), getInitializedCleanup());
+    return ManagedValue::forOwnedAddressRValue(getAddress(),
+                                               getInitializedCleanup());
   }
 };
 

lib/SILGen/ManagedValue.h

@@ -66,50 +66,62 @@ class ManagedValue {
     : valueAndFlag(value, isLValue), cleanup(cleanup) {
   }
 
-public:
-  
-  ManagedValue() = default;
+  /// Create a managed value for a +0 rvalue.
+  ///
+  /// Please do not introduce new uses of this method! Instead use one of the
+  /// static constructors below!
+  static ManagedValue forUnmanaged(SILValue value) {
+    assert(value && "No value specified");
+    return ManagedValue(value, false, CleanupHandle::invalid());
+  }
 
   /// Create a managed value for a +1 rvalue.
   ///
   /// Please do not introduce new uses of this method! Instead use one of the
   /// static constructors below.
-  ManagedValue(SILValue value, CleanupHandle cleanup)
-    : valueAndFlag(value, false), cleanup(cleanup) {
+  explicit ManagedValue(SILValue value,
+                        CleanupHandle cleanup = CleanupHandle::invalid())
+      : valueAndFlag(value, false), cleanup(cleanup) {
     assert(value && "No value specified?!");
     assert((!getType().isObject() ||
             value->getOwnershipKind() != OwnershipKind::None ||
             !hasCleanup()) &&
            "Objects with trivial ownership should never have a cleanup");
   }
 
-  /// Create a managed value for a +0 rvalue.
+public:
+  ManagedValue() = default;
+
+  /// Sometimes SILGen wants to represent an owned value or owned address
+  /// without a cleanup as a +0 value that must be copied to be consumed.
   ///
-  /// Please do not introduce new uses of this method! Instead use one of the
-  /// static constructors below!
-  static ManagedValue forUnmanaged(SILValue value) {
-    assert(value && "No value specified");
-    return ManagedValue(value, false, CleanupHandle::invalid());
+  /// Please do not introduce new uses of this.
+  ///
+  /// DISCUSSION: We purposely provide a specific API for code paths that use
+  /// owned values (and assert the values are owned) so that users do not
+  /// attempt to use this for borrowed values. All borrowed values need to use
+  /// the borrowed value APIs.
+  static ManagedValue forUnmanagedOwnedValue(SILValue value) {
+    assert(value);
+    assert(!value->getType().isObject() ||
+           value->getOwnershipKind().isCompatibleWith(OwnershipKind::Owned));
+    return ManagedValue(value);
+  }
+
+  /// Wrap a value with OwnershipKind::Unowned in a ManagedValue. This must be
+  /// copied before it is used.
+  static ManagedValue forUnownedObjectValue(SILValue value) {
+    assert(value);
+    assert(value->getType().isObject());
+    assert(value->getOwnershipKind().isCompatibleWith(OwnershipKind::Unowned));
+    return ManagedValue(value);
   }
 
   enum class ScopeKind {
     Lexical,
     FormalAccess,
   };
 
-  /// Given a value \p value, create a copy of it and return the relevant
-  /// ManagedValue.
-  static ManagedValue forCopyOwnedObjectRValue(SILGenFunction &SGF,
-                                               SILLocation loc, SILValue value,
-                                               ScopeKind kind) {
-    assert(value && "No value specified");
-    assert(value->getType().isObject());
-    auto mv = ManagedValue::forUnmanaged(value);
-    if (kind == ScopeKind::Lexical)
-      return mv.copy(SGF, loc);
-    return mv.formalAccessCopy(SGF, loc);
-  }
-
   /// Create a managed value for a SILValue whose ownership is
   /// forwarded. Creates a new cleanup for +1 values. Forwarded +0 values
   /// require no cleanup.
@@ -160,7 +172,10 @@ class ManagedValue {
     assert(value && "No value specified");
     assert(value->getType().isObject() &&
            "Expected borrowed rvalues to be objects");
-    assert(value->getOwnershipKind() != OwnershipKind::None);
+    if (value->getOwnershipKind() == OwnershipKind::None) {
+      return forObjectRValueWithoutOwnership(value);
+    }
+    assert(value->getOwnershipKind() == OwnershipKind::Guaranteed);
     return ManagedValue(value, false, CleanupHandle::invalid());
   }
 
@@ -169,6 +184,13 @@ class ManagedValue {
   forBorrowedAddressRValue(SILValue value) {
     assert(value && "No value specified");
     assert(value->getType().isAddress() && "Expected value to be an address");
+    // We check for value->getFunction() here since we /could/ be passed
+    // SILUndef here.
+    if (auto *f = value->getFunction()) {
+      if (value->getType().isTrivial(f)) {
+        return forTrivialAddressRValue(value);
+      }
+    }
     assert(value->getOwnershipKind() == OwnershipKind::None &&
            "Addresses always have trivial ownership");
     return ManagedValue(value, false, CleanupHandle::invalid());
@@ -193,6 +215,16 @@ class ManagedValue {
   static ManagedValue forTrivialAddressRValue(SILValue value) {
     assert(value->getType().isAddress() && "Expected an address");
     assert(value->getOwnershipKind() == OwnershipKind::None);
+
+    // TODO: Add an assert that we have a trivial type here.
+    //
+    // DISCUSSION: We cannot do this today since we have problems along certain
+    // materialization paths where we want to emit a borrow operation. To handle
+    // those cases, we have loosened the rules of OSSA by allowing for store
+    // [trivial] to take non-trivial .none parameters. This has hidden bugs
+    // where SILGen emits a borrow to materialize a parameter for an @in
+    // parameter. It just coincidently works since when we emit the
+    // store_borrow, we use the store [trivial] instead. This should be fixed.
     return ManagedValue(value, false, CleanupHandle::invalid());
   }
 
@@ -476,8 +508,8 @@ class ConsumableManagedValue {
   ///
   /// You probably should not be using this; it's here to make it easy
   /// to find code that is probably wrong.
-  ManagedValue asUnmanagedValue() const {
-    return ManagedValue::forUnmanaged(Value.getValue());
+  ManagedValue asUnmanagedOwnedValue() const {
+    return ManagedValue::forUnmanagedOwnedValue(Value.getValue());
   }
 
   /// Return the consumption rules appropriate for the final use of
@@ -489,15 +521,19 @@ class ConsumableManagedValue {
   ConsumableManagedValue asBorrowedOperand(SILGenFunction &SGF,
                                            SILLocation loc) const {
     if (getType().isAddress())
-      return {asUnmanagedValue(), CastConsumptionKind::CopyOnSuccess};
-    return {asUnmanagedValue().borrow(SGF, loc),
+      return {asUnmanagedOwnedValue(), CastConsumptionKind::CopyOnSuccess};
+
+    if (Value.getOwnershipKind() == OwnershipKind::Guaranteed)
+      return {Value, CastConsumptionKind::BorrowAlways};
+
+    return {asUnmanagedOwnedValue().borrow(SGF, loc),
             CastConsumptionKind::BorrowAlways};
   }
 
   /// Return a managed value that's appropriate for copying this value and
   /// always consuming it.
   ConsumableManagedValue copy(SILGenFunction &SGF, SILLocation loc) const {
-    return ConsumableManagedValue::forOwned(asUnmanagedValue().copy(SGF, loc));
+    return ConsumableManagedValue::forOwned(Value.copy(SGF, loc));
   }
 };
 

lib/SILGen/ResultPlan.cpp

@@ -810,11 +810,13 @@ class ForeignAsyncInitializationPlan final : public ResultPlan {
     if (handlerIsOptional) {
       block = SGF.B.createOptionalSome(loc, block, impTy);
     }
-    
+
     // We don't need to manage the block because it's still on the stack. We
     // know we won't escape it locally so the callee can be responsible for
     // _Block_copy-ing it.
-    return ManagedValue::forUnmanaged(block);
+    //
+    // InitBlockStorageHeader always has Unowned ownership.
+    return ManagedValue::forUnownedObjectValue(block);
   }
 
   void deferExecutorBreadcrumb(ExecutorBreadcrumb &&crumb) override {
@@ -899,9 +901,9 @@ class ForeignAsyncInitializationPlan final : public ResultPlan {
             SILValue(wrappedContinuation));
         SGF.emitApplyOfLibraryIntrinsic(
             loc, errorIntrinsic, subs,
-            {continuationMV, ManagedValue::forCopyOwnedObjectRValue(
-                                 SGF, loc, bridgedForeignError,
-                                 ManagedValue::ScopeKind::Lexical)},
+            {continuationMV,
+             SGF.B.copyOwnedObjectRValue(loc, bridgedForeignError,
+                                         ManagedValue::ScopeKind::Lexical)},
             SGFContext());
 
         // Second, emit a branch from the end of the foreign error block to the

lib/SILGen/SILGenApply.cpp

@@ -1360,12 +1360,13 @@ class SILGenApply : public Lowering::ExprVisitor<SILGenApply> {
       // we installed.  Install a new special cleanup.
       if (superMV.getValue() != SGF.InitDelegationSelf.getValue()) {
         SILValue underlyingSelf = SGF.InitDelegationSelf.getValue();
-        SGF.InitDelegationSelf = ManagedValue::forUnmanaged(underlyingSelf);
+        SGF.InitDelegationSelf =
+            ManagedValue::forUnmanagedOwnedValue(underlyingSelf);
         CleanupHandle newWriteback = SGF.enterOwnedValueWritebackCleanup(
             SGF.InitDelegationLoc.value(), SGF.InitDelegationSelfBox,
             superMV.forward(SGF));
-        SGF.SuperInitDelegationSelf =
-            ManagedValue(superMV.getValue(), newWriteback);
+        SGF.SuperInitDelegationSelf = ManagedValue::forOwnedObjectRValue(
+            superMV.getValue(), newWriteback);
         super = RValue(SGF, SGF.InitDelegationLoc.value(), superFormalType,
                        SGF.SuperInitDelegationSelf);
       }
@@ -4328,7 +4329,7 @@ static void emitBorrowedLValueRecursive(SILGenFunction &SGF,
         value = SGF.B.createFormalAccessLoadCopy(loc, value);
         // Strip off the cleanup from the load [copy] since we do not want the
         // cleanup to be forwarded.
-        value = ManagedValue::forUnmanaged(value.getValue());
+        value = ManagedValue::forUnmanagedOwnedValue(value.getValue());
       } else {
         value = SGF.B.createFormalAccessLoadBorrow(loc, value);
       }
@@ -4507,7 +4508,7 @@ class BoxInitialization : public SingleBufferInitialization {
   }
 
   ManagedValue getManagedBox() const {
-    return ManagedValue(box, initCleanup);
+    return ManagedValue::forOwnedObjectRValue(box, initCleanup);
   }
 };
 
@@ -6224,7 +6225,7 @@ void SILGenFunction::emitUninitializedArrayDeallocation(SILLocation loc,
   auto subMap = arrayTy->getContextSubstitutionMap(SGM.M.getSwiftModule(),
                                                    Ctx.getArrayDecl());
   emitApplyOfLibraryIntrinsic(loc, deallocate, subMap,
-                              ManagedValue::forUnmanaged(array),
+                              ManagedValue::forUnmanagedOwnedValue(array),
                               SGFContext());
 }
 
@@ -6247,9 +6248,9 @@ ManagedValue SILGenFunction::emitUninitializedArrayFinalization(SILLocation loc,
   // Invoke the intrinsic.
   auto subMap = arrayTy->getContextSubstitutionMap(SGM.M.getSwiftModule(),
                                                    Ctx.getArrayDecl());
-  RValue result = emitApplyOfLibraryIntrinsic(loc, finalize, subMap,
-                              ManagedValue::forUnmanaged(arrayVal),
-                              SGFContext());
+  RValue result = emitApplyOfLibraryIntrinsic(
+      loc, finalize, subMap, ManagedValue::forUnmanagedOwnedValue(arrayVal),
+      SGFContext());
   return std::move(result).getScalarValue();
 }
 

lib/SILGen/SILGenBackDeploy.cpp

@@ -143,9 +143,9 @@ static void emitBackDeployForwardApplyAndReturnOrThrow(
 
     // Emit error block.
     SGF.B.emitBlock(errorBB);
-    SILValue error = errorBB->createPhiArgument(
-        SGF.F.mapTypeIntoContext(fnConv.getSILErrorType(TEC)),
-        OwnershipKind::Owned);
+    ManagedValue error =
+        SGF.B.createPhi(SGF.F.mapTypeIntoContext(fnConv.getSILErrorType(TEC)),
+                        OwnershipKind::Owned);
     SGF.B.createBranch(loc, SGF.ThrowDest.getBlock(), {error});
 
     // Emit normal block.