[5.9🍒] Add the drop_deinit instruction & consume-on-all-paths checking for discard

SwiftCompilerSources/Sources/SIL/Instruction.swift

@@ -647,6 +647,10 @@ final public class MoveValueInst : SingleValueInstruction, UnaryInstruction {
   public var fromValue: Value { operand.value }
 }
 
+final public class DropDeinitInst : SingleValueInstruction, UnaryInstruction {
+  public var fromValue: Value { operand.value }
+}
+
 final public class StrongCopyUnownedValueInst : SingleValueInstruction, UnaryInstruction {}
 
 final public class StrongCopyUnmanagedValueInst : SingleValueInstruction, UnaryInstruction  {}

SwiftCompilerSources/Sources/SIL/Registration.swift

@@ -125,6 +125,7 @@ public func registerSILClasses() {
   register(ProjectBoxInst.self)
   register(CopyValueInst.self)
   register(MoveValueInst.self)
+  register(DropDeinitInst.self)
   register(EndCOWMutationInst.self)
   register(ClassifyBridgeObjectInst.self)
   register(PartialApplyInst.self)

docs/SIL.rst

@@ -6148,6 +6148,29 @@ a type `T` into the move only value space.
 The ``lexical`` attribute specifies that the value corresponds to a local
 variable in the Swift source.
 
+
+drop_deinit
+```````````
+
+::
+
+   sil-instruction ::= 'drop_deinit' sil-operand
+
+   %1 = drop_deinit %0 : $T
+   // T must be a move-only type
+   // %1 is an @owned T
+   %3 = drop_deinit %2 : $*T
+   // T must be a move-only type
+   // %2 has type *T
+
+This instruction is a marker for a following destroy instruction to suppress
+the call of the move-only type's deinitializer.
+The instruction accepts an object or address type.
+If its argument is an object type it takes in an `@owned T` and produces a new
+`@owned T`. If its argument is an address type, it's an identity projection.
+
+The instruction is only valid in ownership SIL.
+
 release_value
 `````````````
 

include/swift/AST/DiagnosticsSIL.def

@@ -776,7 +776,11 @@ ERROR(sil_movechecking_notconsumable_but_assignable_was_consumed, none,
 ERROR(sil_movechecking_cannot_destructure_has_deinit, none,
       "cannot partially consume '%0' when it has a deinitializer",
       (StringRef))
+ERROR(sil_movechecking_discard_missing_consume_self, none,
+      "must consume 'self' before exiting method that discards self", ())
 
+NOTE(sil_movechecking_discard_self_here, none,
+     "discarded self here", ())
 NOTE(sil_movechecking_partial_consume_here, none,
      "partially consumed here", ())
 NOTE(sil_movechecking_consuming_use_here, none,

include/swift/SIL/MemAccessUtils.h

@@ -1632,6 +1632,7 @@ inline bool isAccessStorageIdentityCast(SingleValueInstruction *svi) {
   // Simply pass-thru the incoming address.
   case SILInstructionKind::MarkUninitializedInst:
   case SILInstructionKind::MarkMustCheckInst:
+  case SILInstructionKind::DropDeinitInst:
   case SILInstructionKind::MarkUnresolvedReferenceBindingInst:
   case SILInstructionKind::MarkDependenceInst:
   case SILInstructionKind::CopyValueInst:

include/swift/SIL/SILBuilder.h

@@ -1364,6 +1364,14 @@ class SILBuilder {
                                                   operand, isLexical));
   }
 
+  DropDeinitInst *createDropDeinit(SILLocation loc, SILValue operand) {
+    assert(getFunction().hasOwnership());
+    assert(!operand->getType().isTrivial(getFunction()) &&
+           "Should not be passing trivial values to this api.");
+    return insert(new (getModule()) DropDeinitInst(getSILDebugLocation(loc),
+                                                   operand));
+  }
+
   MarkUnresolvedMoveAddrInst *createMarkUnresolvedMoveAddr(SILLocation loc,
                                                            SILValue srcAddr,
                                                            SILValue takeAddr) {

include/swift/SIL/SILCloner.h

@@ -1896,6 +1896,17 @@ void SILCloner<ImplClass>::visitMoveValueInst(MoveValueInst *Inst) {
   recordClonedInstruction(Inst, MVI);
 }
 
+template <typename ImplClass>
+void SILCloner<ImplClass>::visitDropDeinitInst(DropDeinitInst *Inst) {
+  getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
+  if (!getBuilder().hasOwnership()) {
+    return recordFoldedValue(Inst, getOpValue(Inst->getOperand()));
+  }
+  auto *MVI = getBuilder().createDropDeinit(getOpLocation(Inst->getLoc()),
+                                            getOpValue(Inst->getOperand()));
+  recordClonedInstruction(Inst, MVI);
+}
+
 template <typename ImplClass>
 void SILCloner<ImplClass>::visitMarkMustCheckInst(MarkMustCheckInst *Inst) {
   getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));

include/swift/SIL/SILInstruction.h

@@ -8301,6 +8301,15 @@ class MoveValueInst
   void removeIsLexical() { lexical = false; }
 };
 
+class DropDeinitInst
+    : public UnaryInstructionBase<SILInstructionKind::DropDeinitInst,
+                                  SingleValueInstruction> {
+  friend class SILBuilder;
+
+  DropDeinitInst(SILDebugLocation DebugLoc, SILValue operand)
+      : UnaryInstructionBase(DebugLoc, operand, operand->getType()) {}
+};
+
 /// Equivalent to a copy_addr to [init] except that it is used for diagnostics
 /// and should not be pattern matched. During the diagnostic passes, the "move
 /// function" checker for addresses always converts this to a copy_addr [init]

include/swift/SIL/SILNodes.def

@@ -463,6 +463,8 @@ ABSTRACT_VALUE_AND_INST(SingleValueInstruction, ValueBase, SILInstruction)
   // effects relative to other OSSA values like copy_value.
   SINGLE_VALUE_INST(MoveValueInst, move_value, SingleValueInstruction, None,
                     DoesNotRelease)
+  SINGLE_VALUE_INST(DropDeinitInst, drop_deinit, SingleValueInstruction, None,
+                    DoesNotRelease)
   // 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

lib/IRGen/IRGenSIL.cpp

@@ -1248,6 +1248,10 @@ class IRGenSILFunction :
     auto e = getLoweredExplosion(i->getOperand());
     setLoweredExplosion(i, e);
   }
+  void visitDropDeinitInst(DropDeinitInst *i) {
+    auto e = getLoweredExplosion(i->getOperand());
+    setLoweredExplosion(i, e);
+  }
   void visitMarkMustCheckInst(MarkMustCheckInst *i) {
     llvm_unreachable("Invalid in Lowered SIL");
   }

lib/SIL/IR/OperandOwnership.cpp

@@ -457,6 +457,12 @@ OperandOwnershipClassifier::visitStoreBorrowInst(StoreBorrowInst *i) {
   return OperandOwnership::TrivialUse;
 }
 
+OperandOwnership
+OperandOwnershipClassifier::visitDropDeinitInst(DropDeinitInst *i) {
+  return i->getType().isAddress() ? OperandOwnership::TrivialUse
+                                  : OperandOwnership::ForwardingConsume;
+}
+
 // Get the OperandOwnership for instantaneous apply, yield, and return uses.
 // This does not apply to uses that begin an explicit borrow scope in the
 // caller, such as begin_apply.

lib/SIL/IR/SILPrinter.cpp

@@ -1999,6 +1999,10 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
     *this << getIDAndType(I->getOperand());
   }
 
+  void visitDropDeinitInst(DropDeinitInst *I) {
+    *this << getIDAndType(I->getOperand());
+  }
+
   void visitMarkMustCheckInst(MarkMustCheckInst *I) {
     using CheckKind = MarkMustCheckInst::CheckKind;
     switch (I->getCheckKind()) {

lib/SIL/IR/ValueOwnership.cpp

@@ -355,6 +355,10 @@ ValueOwnershipKind ValueOwnershipKindClassifier::visitLoadInst(LoadInst *LI) {
   llvm_unreachable("Unhandled LoadOwnershipQualifier in switch.");
 }
 
+ValueOwnershipKind ValueOwnershipKindClassifier::visitDropDeinitInst(DropDeinitInst *ddi) {
+  return ddi->getType().isAddress() ? OwnershipKind::None : OwnershipKind::Owned;
+}
+
 ValueOwnershipKind
 ValueOwnershipKindClassifier::visitPartialApplyInst(PartialApplyInst *PA) {
   // partial_apply instructions are modeled as creating an owned value during

lib/SIL/Parser/ParseSIL.cpp

@@ -3702,6 +3702,15 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
     break;
   }
 
+  case SILInstructionKind::DropDeinitInst: {
+    if (parseTypedValueRef(Val, B))
+      return true;
+    if (parseSILDebugLocation(InstLoc, B))
+      return true;
+    ResultVal = B.createDropDeinit(InstLoc, Val);
+    break;
+  }
+
   case SILInstructionKind::MarkMustCheckInst: {
     StringRef AttrName;
     if (!parseSILOptional(AttrName, *this)) {

lib/SIL/Utils/AddressWalker.cpp

@@ -157,7 +157,7 @@ AddressUseKind TransitiveAddressWalker::walk(SILValue projectedAddress) && {
         isa<BeginAccessInst>(user) || isa<TailAddrInst>(user) ||
         isa<IndexAddrInst>(user) || isa<StoreBorrowInst>(user) ||
         isa<UncheckedAddrCastInst>(user) || isa<MarkMustCheckInst>(user) ||
-        isa<MarkUninitializedInst>(user) ||
+        isa<MarkUninitializedInst>(user) || isa<DropDeinitInst>(user) ||
         isa<ProjectBlockStorageInst>(user) || isa<UpcastInst>(user) ||
         isa<TuplePackElementAddrInst>(user)) {
       transitiveResultUses(op);

lib/SIL/Utils/InstructionUtils.cpp

@@ -441,6 +441,7 @@ RuntimeEffect swift::getRuntimeEffect(SILInstruction *inst, SILType &impactType)
   case SILInstructionKind::ValueToBridgeObjectInst:
   case SILInstructionKind::MarkDependenceInst:
   case SILInstructionKind::MoveValueInst:
+  case SILInstructionKind::DropDeinitInst:
   case SILInstructionKind::MarkMustCheckInst:
   case SILInstructionKind::MarkUnresolvedReferenceBindingInst:
   case SILInstructionKind::CopyableToMoveOnlyWrapperValueInst:

lib/SIL/Utils/MemAccessUtils.cpp

@@ -1866,6 +1866,7 @@ AccessPathDefUseTraversal::visitSingleValueUser(SingleValueInstruction *svi,
     return IgnoredUse;
   }
 
+  case SILInstructionKind::DropDeinitInst:
   case SILInstructionKind::MarkMustCheckInst: {
     // Mark must check goes on the project_box, so it isn't a ref.
     assert(!dfs.isRef());

lib/SIL/Verifier/SILVerifier.cpp

@@ -5968,6 +5968,14 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
             "Result and operand must have the same type, today.");
   }
 
+  void checkDropDeinitInst(DropDeinitInst *ddi) {
+    require(ddi->getType() == ddi->getOperand()->getType(),
+            "Result and operand must have the same type.");
+    require(ddi->getType().isMoveOnlyNominalType(),
+            "drop_deinit only allowed for move-only types");
+    require(F.hasOwnership(), "drop_deinit only allowed in OSSA");
+  }
+
   void checkMarkMustCheckInst(MarkMustCheckInst *i) {
     require(i->getModule().getStage() == SILStage::Raw,
             "Only valid in Raw SIL! Should have been eliminated by /some/ "

lib/SILGen/SILGenDestructor.cpp

@@ -499,6 +499,7 @@ void SILGenFunction::emitMoveOnlyMemberDestruction(SILValue selfValue,
                                                    NominalTypeDecl *nom,
                                                    CleanupLocation cleanupLoc,
                                                    SILBasicBlock *finishBB) {
+  selfValue = B.createDropDeinit(cleanupLoc, selfValue);
   if (selfValue->getType().isAddress()) {
     if (auto *structDecl = dyn_cast<StructDecl>(nom)) {
       for (VarDecl *vd : nom->getStoredProperties()) {

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -691,6 +691,9 @@ struct UseState {
   /// [assign] that are reinits that we will convert to inits and true reinits.
   llvm::SmallMapVector<SILInstruction *, TypeTreeLeafTypeRange, 4> reinitInsts;
 
+  /// The set of drop_deinits of this mark_must_check
+  SmallSetVector<SILInstruction *, 2> dropDeinitInsts;
+
   /// A "inout terminator use" is an implicit liveness use of the entire value
   /// placed on a terminator. We use this both so we add liveness for the
   /// terminator user and so that we can use the set to quickly identify later
@@ -712,6 +715,31 @@ struct UseState {
     return inoutTermUsers.count(inst);
   }
 
+  /// Returns true if the given instruction is within the same block as a reinit
+  /// and precedes a reinit instruction in that block.
+  bool precedesReinitInSameBlock(SILInstruction *inst) const {
+    SILBasicBlock *block = inst->getParent();
+    SmallSetVector<SILInstruction *, 8> sameBlockReinits;
+
+    // First, search for all reinits that are within the same block.
+    for (auto &reinit : reinitInsts) {
+      if (reinit.first->getParent() != block)
+        continue;
+      sameBlockReinits.insert(reinit.first);
+    }
+
+    if (sameBlockReinits.empty())
+      return false;
+
+    // Walk down from the given instruction to see if we encounter a reinit.
+    for (auto ii = std::next(inst->getIterator()); ii != block->end(); ++ii) {
+      if (sameBlockReinits.contains(&*ii))
+        return true;
+    }
+
+    return false;
+  }
+
   void clear() {
     address = nullptr;
     destroys.clear();
@@ -721,6 +749,7 @@ struct UseState {
     takeInsts.clear();
     initInsts.clear();
     reinitInsts.clear();
+    dropDeinitInsts.clear();
     inoutTermUsers.clear();
     debugValue = nullptr;
   }
@@ -755,6 +784,10 @@ struct UseState {
     for (auto pair : reinitInsts) {
       llvm::dbgs() << *pair.first;
     }
+    llvm::dbgs() << "DropDeinits:\n";
+    for (auto *inst : dropDeinitInsts) {
+      llvm::dbgs() << *inst;
+    }
     llvm::dbgs() << "InOut Term Users:\n";
     for (auto *inst : inoutTermUsers) {
       llvm::dbgs() << *inst;
@@ -1737,6 +1770,12 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     LLVM_DEBUG(llvm::dbgs() << "Running copy propagation!\n");
     moveChecker.changed |= moveChecker.canonicalizer.canonicalize();
 
+    // Export the drop_deinit's discovered by the ObjectChecker into the
+    // AddressChecker to preserve it for later use. We need to do this since
+    // the ObjectChecker's state gets cleared after running on this LoadInst.
+    for (auto *dropDeinit : moveChecker.canonicalizer.getDropDeinitUses())
+      moveChecker.addressUseState.dropDeinitInsts.insert(dropDeinit);
+
     // If we are asked to perform no_consume_or_assign checking or
     // assignable_but_not_consumable checking, if we found any consumes of our
     // load, then we need to emit an error.
@@ -2458,10 +2497,41 @@ void MoveOnlyAddressCheckerPImpl::rewriteUses(
     FieldSensitiveMultiDefPrunedLiveRange &liveness,
     const FieldSensitivePrunedLivenessBoundary &boundary) {
   LLVM_DEBUG(llvm::dbgs() << "MoveOnlyAddressChecker Rewrite Uses!\n");
-  // First remove all destroy_addr that have not been claimed.
+
+  /// Whether the marked value appeared in a discard statement.
+  const bool isDiscardingContext = !addressUseState.dropDeinitInsts.empty();
+
+  // Process destroys
   for (auto destroyPair : addressUseState.destroys) {
-    if (!consumes.claimConsume(destroyPair.first, destroyPair.second)) {
+    /// Is this destroy instruction a final consuming use?
+    bool isFinalConsume =
+        consumes.claimConsume(destroyPair.first, destroyPair.second);
+
+    // Remove destroys that are not the final consuming use.
+    if (!isFinalConsume) {
       destroyPair.first->eraseFromParent();
+      continue;
+    }
+
+    // Otherwise, if we're in a discarding context, flag this final destroy_addr
+    // as a point where we're missing an explicit `consume self`. The reasoning
+    // here is that if a destroy of self is the final consuming use,
+    // then these are the points where we implicitly destroy self to clean-up
+    // that self var before exiting the scope. An explicit 'consume self'
+    // that is thrown away is a consume of this mark_must_check'd var and not a
+    // destroy of it, according to the use classifier.
+    if (isDiscardingContext) {
+
+      // Since the boundary computations treat a newly-added destroy prior to
+      // a reinit within that same block as a "final consuming use", exclude
+      // such destroys-before-reinit. We are only interested in the final
+      // destroy of a var, not intermediate destroys of the var.
+      if (addressUseState.precedesReinitInSameBlock(destroyPair.first))
+        continue;
+
+      auto *dropDeinit = addressUseState.dropDeinitInsts.front();
+      diagnosticEmitter.emitMissingConsumeInDiscardingContext(destroyPair.first,
+                                                              dropDeinit);
     }
   }
 

lib/SILOptimizer/Mandatory/MoveOnlyDeinitInsertion.cpp

@@ -62,7 +62,8 @@ static bool performTransform(SILFunction &fn) {
 
       if (auto *dvi = dyn_cast<DestroyValueInst>(inst)) {
         auto destroyType = dvi->getOperand()->getType();
-        if (destroyType.isMoveOnlyNominalType()) {
+        if (destroyType.isMoveOnlyNominalType() &&
+            !isa<DropDeinitInst>(lookThroughOwnershipInsts(dvi->getOperand()))) {
           LLVM_DEBUG(llvm::dbgs() << "Handling: " << *dvi);
           auto *nom = destroyType.getNominalOrBoundGenericNominal();
           assert(nom);
@@ -100,7 +101,8 @@ static bool performTransform(SILFunction &fn) {
 
       if (auto *dai = dyn_cast<DestroyAddrInst>(inst)) {
         auto destroyType = dai->getOperand()->getType();
-        if (destroyType.isLoadable(fn) && destroyType.isMoveOnlyNominalType()) {
+        if (destroyType.isLoadable(fn) && destroyType.isMoveOnlyNominalType() &&
+            !isa<DropDeinitInst>(dai->getOperand())) {
           LLVM_DEBUG(llvm::dbgs() << "Handling: " << *dai);
           auto *nom = destroyType.getNominalOrBoundGenericNominal();
           assert(nom);