Merge pull request #65645 from gottesmm/release/5.9/rdar105106470

[5.9][move-only] Address Only Batched Fixes

Author: gottesmm

include/swift/SIL/SILUndef.h

@@ -30,6 +30,12 @@ class SILUndef : public ValueBase {
   void operator delete(void *, size_t) = delete;
 
   static SILUndef *get(SILType ty, SILModule &m);
+
+  /// Return a SILUndef with the same type as the passed in value.
+  static SILUndef *get(SILValue value) {
+    return SILUndef::get(value->getType(), *value->getModule());
+  }
+
   static SILUndef *get(SILType ty, const SILFunction &f);
 
   template <class OwnerTy>

lib/SIL/IR/SILFunctionType.cpp

@@ -2384,7 +2384,7 @@ struct DeallocatorConventions : Conventions {
 
   ParameterConvention
   getIndirectSelfParameter(const AbstractionPattern &type) const override {
-    llvm_unreachable("Deallocators do not have indirect self parameters");
+    return ParameterConvention::Indirect_In;
   }
 
   static bool classof(const Conventions *C) {

lib/SIL/Verifier/SILVerifier.cpp

@@ -674,6 +674,7 @@ struct ImmutableAddressUseVerifier {
       case SILInstructionKind::IndexAddrInst:
       case SILInstructionKind::TailAddrInst:
       case SILInstructionKind::IndexRawPointerInst:
+      case SILInstructionKind::MarkMustCheckInst:
         // Add these to our worklist.
         for (auto result : inst->getResults()) {
           llvm::copy(result->getUses(), std::back_inserter(worklist));

lib/SILGen/SILGenLValue.cpp

@@ -3183,7 +3183,8 @@ RValue SILGenFunction::emitRValueForNonMemberVarDecl(SILLocation loc,
     SILValue accessAddr = UnenforcedFormalAccess::enter(*this, loc, destAddr,
                                                         SILAccessKind::Read);
 
-    if (accessAddr->getType().isMoveOnly()) {
+    if (accessAddr->getType().isMoveOnly() &&
+        !isa<MarkMustCheckInst>(accessAddr)) {
       // When loading an rvalue, we should never need to modify the place
       // we're loading from.
       accessAddr = B.createMarkMustCheckInst(

lib/SILGen/SILGenProlog.cpp

@@ -40,7 +40,7 @@ static void diagnose(ASTContext &Context, SourceLoc loc, Diag<T...> diag,
 
 SILValue SILGenFunction::emitSelfDeclForDestructor(VarDecl *selfDecl) {
   // Emit the implicit 'self' argument.
-  SILType selfType = getLoweredLoadableType(selfDecl->getType());
+  SILType selfType = getLoweredType(selfDecl->getType());
   SILValue selfValue = F.begin()->createFunctionArgument(selfType, selfDecl);
 
   // If we have a move only type, then mark it with mark_must_check so we can't
@@ -748,7 +748,6 @@ class ArgumentInitHelper {
   /// if not null.
   void makeArgumentIntoBinding(SILLocation loc, ParamDecl *pd) {
     ManagedValue argrv = makeArgument(loc, pd);
-
     SILValue value = argrv.getValue();
     if (pd->isInOut()) {
       assert(argrv.getType().isAddress() && "expected inout to be address");
@@ -768,18 +767,52 @@ class ArgumentInitHelper {
     if (!argrv.getType().isAddress()) {
       // NOTE: We setup SGF.VarLocs[pd] in updateArgumentValueForBinding.
       updateArgumentValueForBinding(argrv, loc, pd, value, varinfo);
-    } else {
-      if (auto *allocStack = dyn_cast<AllocStackInst>(value)) {
-        allocStack->setArgNo(ArgNo);
-        if (SGF.getASTContext().SILOpts.supportsLexicalLifetimes(
-                SGF.getModule()) &&
-            SGF.F.getLifetime(pd, value->getType()).isLexical())
-          allocStack->setIsLexical();
-      } else {
-        SGF.B.createDebugValueAddr(loc, value, varinfo);
-      }
+      return;
+    }
+
+    if (auto *allocStack = dyn_cast<AllocStackInst>(value)) {
+      allocStack->setArgNo(ArgNo);
+      if (SGF.getASTContext().SILOpts.supportsLexicalLifetimes(
+              SGF.getModule()) &&
+          SGF.F.getLifetime(pd, value->getType()).isLexical())
+        allocStack->setIsLexical();
       SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(value);
+      return;
+    }
+
+    if (value->getType().isMoveOnly()) {
+      switch (pd->getValueOwnership()) {
+      case ValueOwnership::Default:
+        if (pd->isSelfParameter()) {
+          assert(!isa<MarkMustCheckInst>(value) &&
+                 "Should not have inserted mark must check inst in EmitBBArgs");
+          if (!pd->isInOut()) {
+            value = SGF.B.createMarkMustCheckInst(
+                loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
+          }
+        } else {
+          assert(isa<MarkMustCheckInst>(value) &&
+                 "Should have inserted mark must check inst in EmitBBArgs");
+        }
+        break;
+      case ValueOwnership::InOut:
+        assert(isa<MarkMustCheckInst>(value) &&
+               "Expected mark must check inst with inout to be handled in "
+               "emitBBArgs earlier");
+        break;
+      case ValueOwnership::Owned:
+        value = SGF.B.createMarkMustCheckInst(
+            loc, value, MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
+        break;
+      case ValueOwnership::Shared:
+        value = SGF.B.createMarkMustCheckInst(
+            loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
+        break;
+      }
     }
+
+    SGF.B.createDebugValueAddr(loc, value, varinfo);
+    SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(value);
   }
 
   void emitParam(ParamDecl *PD) {

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -1511,6 +1511,19 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     assert(op->getOperandNumber() == CopyAddrInst::Src &&
            "Should have dest above in memInstMust{Rei,I}nitialize");
 
+    auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress());
+    if (!leafRange)
+      return false;
+
+    // If we have a non-move only type, just treat this as a liveness use.
+    if (!copyAddr->getSrc()->getType().isMoveOnly()) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "Found copy of copyable type. Treating as liveness use! "
+                 << *user);
+      useState.livenessUses.insert({user, *leafRange});
+      return true;
+    }
+
     if (markedValue->getCheckKind() ==
         MarkMustCheckInst::CheckKind::NoConsumeOrAssign) {
       LLVM_DEBUG(llvm::dbgs()
@@ -1520,17 +1533,11 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
       return true;
     }
 
-    auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress());
-    if (!leafRange)
-      return false;
-
     // TODO: Add borrow checking here like below.
 
     // TODO: Add destructure deinit checking here once address only checking is
     // completely brought up.
 
-    // TODO: Add check here that we don't error on trivial/copyable types.
-
     if (copyAddr->isTakeOfSrc()) {
       LLVM_DEBUG(llvm::dbgs() << "Found take: " << *user);
       useState.takeInsts.insert({user, *leafRange});
@@ -1721,9 +1728,30 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
   // Now that we have handled or loadTakeOrCopy, we need to now track our
   // additional pure takes.
   if (::memInstMustConsume(op)) {
+    // If we don't have a consumeable and assignable check kind, then we can't
+    // consume. Emit an error.
+    //
+    // NOTE: Since SILGen eagerly loads loadable types from memory, this
+    // generally will only handle address only types.
+    if (markedValue->getCheckKind() !=
+        MarkMustCheckInst::CheckKind::ConsumableAndAssignable) {
+      auto *fArg = dyn_cast<SILFunctionArgument>(
+          stripAccessMarkers(markedValue->getOperand()));
+      if (fArg && fArg->isClosureCapture() && fArg->getType().isAddress()) {
+        moveChecker.diagnosticEmitter.emitPromotedBoxArgumentError(markedValue,
+                                                                   fArg);
+      } else {
+        moveChecker.diagnosticEmitter
+            .emitAddressEscapingClosureCaptureLoadedAndConsumed(markedValue);
+      }
+      emittedEarlyDiagnostic = true;
+      return true;
+    }
+
     auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress());
     if (!leafRange)
       return false;
+
     LLVM_DEBUG(llvm::dbgs() << "Pure consuming use: " << *user);
     useState.takeInsts.insert({user, *leafRange});
     return true;
@@ -2423,7 +2451,6 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
     LLVM_DEBUG(llvm::dbgs() << "Failed access path visit: " << *markedAddress);
     return false;
   }
-  addressUseState.initializeInOutTermUsers();
 
   // If we found a load [copy] or copy_addr that requires multiple copies or an
   // exclusivity error, then we emitted an early error. Bail now and allow the
@@ -2438,9 +2465,14 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
   if (diagCount != diagnosticEmitter.getDiagnosticCount())
     return true;
 
-  // Then check if we emitted an error. If we did not, return true.
-  if (diagCount != diagnosticEmitter.getDiagnosticCount())
-    return true;
+  // Now that we know that we have run our visitor and did not emit any errors
+  // and successfully visited everything, see if have any
+  // assignable_but_not_consumable of address only types that are consumed.
+  //
+  // DISCUSSION: For non address only types, this is not an issue since we
+  // eagerly load
+
+  addressUseState.initializeInOutTermUsers();
 
   //===---
   // Liveness Checking

test/Interpreter/moveonly.swift

@@ -45,7 +45,7 @@ Tests.test("global destroyed once") {
   do {
     global = FD()
   }
-  expectEqual(0, LifetimeTracked.instances)    
+  expectEqual(0, LifetimeTracked.instances)
 }
 
 @_moveOnly
@@ -104,3 +104,31 @@ Tests.test("empty struct") {
     let _ = consume e
   }
 }
+
+protocol P {
+   var name: String { get }
+}
+
+Tests.test("AddressOnly") {
+    class Klass : P {
+        var name: String { "myName" }
+    }
+
+    @_moveOnly
+    struct S<T : P> {
+        var t: T
+    }
+
+    let e = S(t: Klass())
+    expectEqual(e.t.name, "myName")
+
+    func testGeneric<T : P>(_ x: borrowing S<T>) {
+        expectEqual(x.t.name, "myName")
+    }
+    testGeneric(e)
+
+    if e.t.name.count == 5 {
+        let _ = consume e
+    }
+}
+