Merge pull request #66357 from atrick/5.9-fix-dropdeinit

[5.9][move-only] Fix SIL representation and SILOptimizer to preserve value deinits.

Author: atrick

include/swift/SIL/InstructionUtils.h

@@ -40,6 +40,12 @@ SILValue stripCastsWithoutMarkDependence(SILValue V);
 /// begin_borrow instructions.
 SILValue lookThroughOwnershipInsts(SILValue v);
 
+/// Reverse of lookThroughOwnershipInsts.
+///
+/// Return true if \p visitor returned true for all uses.
+bool visitNonOwnershipUses(SILValue value,
+                           function_ref<bool(Operand *)> visitor);
+
 /// Return the underlying SILValue after looking through all copy_value
 /// instructions.
 SILValue lookThroughCopyValueInsts(SILValue v);

include/swift/SIL/SILMoveOnlyDeinit.h

@@ -61,7 +61,10 @@ class SILMoveOnlyDeinit final : public SILAllocated<SILMoveOnlyDeinit> {
 
   NominalTypeDecl *getNominalDecl() const { return nominalDecl; }
 
-  SILFunction *getImplementation() const { return funcImpl; }
+  SILFunction *getImplementation() const {
+    assert(funcImpl);
+    return funcImpl;
+  }
 
   IsSerialized_t isSerialized() const {
     return serialized ? IsSerialized : IsNotSerialized;

include/swift/SIL/SILNodes.def

@@ -463,8 +463,15 @@ 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 drop_deinit has no user-level side effects. It does, however, change the
+  // information about the owenership of its operand, and therefore cannot be
+  // arbitrarily deleted. It effectively wraps the type of the operand so that
+  // the resulting value is an equivalent type without a user-defined deinit.
+  //
+  // TODO: Add an OwnershipEffect type of side effect for instructions like
+  // drop_deinit to indicate their ownership effects.
+  SINGLE_VALUE_INST(DropDeinitInst, drop_deinit, SingleValueInstruction,
+                    MayHaveSideEffects, 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

include/swift/SILOptimizer/PassManager/Passes.def

@@ -462,7 +462,7 @@ PASS(PartialApplySimplification, "partial-apply-simplification",
      "Transform partial_apply instructions into explicit closure box constructions")
 PASS(MovedAsyncVarDebugInfoPropagator, "sil-moved-async-var-dbginfo-propagator",
      "Propagate debug info from moved async vars after coroutine funclet boundaries")
-PASS(MoveOnlyDeinitInsertion, "sil-move-only-deinit-insertion",
+PASS(MoveOnlyDeinitDevirtualization, "sil-move-only-deinit-devirtualization",
      "After running move only checking, convert last destroy_values to deinit calls")
 PASS(MoveOnlyBorrowToDestructureTransform,
      "sil-move-only-borrow-to-destructure",

include/swift/SILOptimizer/Utils/InstOptUtils.h

@@ -141,6 +141,16 @@ void collectUsesOfValue(SILValue V,
 /// value itself)
 void eraseUsesOfValue(SILValue value);
 
+/// Return true if \p type is a value type (struct/enum) that requires
+/// deinitialization beyond destruction of its members.
+bool hasValueDeinit(SILType type);
+
+/// Return true if \p value has a value type (struct/enum) that requires
+/// deinitialization beyond destruction of its members.
+inline bool hasValueDeinit(SILValue value) {
+  return hasValueDeinit(value->getType());
+}
+
 /// Gets the concrete value which is stored in an existential box.
 /// Returns %value in following pattern:
 ///
@@ -381,8 +391,10 @@ ignore_expect_uses(ValueBase *value) {
 /// operations from it. These can be simplified and removed.
 bool simplifyUsers(SingleValueInstruction *inst);
 
-///  True if a type can be expanded
-/// without a significant increase to code size.
+/// True if a type can be expanded without a significant increase to code size.
+///
+/// False if expanding a type is invalid. For example, expanding a
+/// struct-with-deinit drops the deinit.
 bool shouldExpand(SILModule &module, SILType ty);
 
 /// Check if the value of value is computed by means of a simple initialization.

lib/IRGen/IRGenSIL.cpp

@@ -1249,8 +1249,7 @@ class IRGenSILFunction :
     setLoweredExplosion(i, e);
   }
   void visitDropDeinitInst(DropDeinitInst *i) {
-    auto e = getLoweredExplosion(i->getOperand());
-    setLoweredExplosion(i, e);
+    llvm_unreachable("only valid in ownership SIL");
   }
   void visitMarkMustCheckInst(MarkMustCheckInst *i) {
     llvm_unreachable("Invalid in Lowered SIL");

lib/SIL/IR/TypeLowering.cpp

@@ -1575,6 +1575,19 @@ namespace {
       return B.createStruct(loc, getLoweredType(), values);
     }
 
+    void
+    emitLoweredDestroyValue(SILBuilder &B, SILLocation loc, SILValue aggValue,
+                            TypeExpansionKind loweringStyle) const override {
+      // A value type with a deinit cannot be memberwise destroyed.
+      if (auto *nominal = getLoweredType().getNominalOrBoundGenericNominal()) {
+        if (nominal->getValueTypeDestructor()) {
+          emitDestroyValue(B, loc, aggValue);
+          return;
+        }
+      }
+      Super::emitLoweredDestroyValue(B, loc, aggValue, loweringStyle);
+    }
+
   private:
     void lowerChildren(TypeConverter &TC,
                        SmallVectorImpl<Child> &children) const override {

lib/SIL/Utils/InstructionUtils.cpp

@@ -39,6 +39,30 @@ SILValue swift::lookThroughOwnershipInsts(SILValue v) {
   }
 }
 
+bool swift::visitNonOwnershipUses(SILValue value,
+                                  function_ref<bool(Operand *)> visitor) {
+  // All ownership insts have a single operand, so a recursive walk is
+  // sufficient and cannot revisit operands.
+  for (Operand *use : value->getUses()) {
+    auto *user = use->getUser();
+    switch (user->getKind()) {
+    default:
+      if (!visitor(use))
+        return false;
+
+      break;
+    case SILInstructionKind::MoveValueInst:
+    case SILInstructionKind::CopyValueInst:
+    case SILInstructionKind::BeginBorrowInst:
+      if (!visitNonOwnershipUses(cast<SingleValueInstruction>(user), visitor))
+        return false;
+
+      break;
+    }
+  }
+  return true;
+}
+
 SILValue swift::lookThroughCopyValueInsts(SILValue val) {
   while (auto *cvi =
              dyn_cast_or_null<CopyValueInst>(val->getDefiningInstruction())) {

lib/SIL/Utils/MemAccessUtils.cpp

@@ -2698,6 +2698,7 @@ void swift::visitAccessedAddress(SILInstruction *I,
   case SILInstructionKind::DeinitExistentialValueInst:
   case SILInstructionKind::DestroyAddrInst:
   case SILInstructionKind::DestroyValueInst:
+  case SILInstructionKind::DropDeinitInst:
   case SILInstructionKind::EndAccessInst:
   case SILInstructionKind::EndApplyInst:
   case SILInstructionKind::EndBorrowInst:

lib/SIL/Verifier/SILVerifier.cpp

@@ -3547,6 +3547,13 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                 "destructure with none ownership kind operand and non-none "
                 "ownership kind result?!");
       }
+      if (operandTy.getNominalOrBoundGenericNominal()
+          ->getValueTypeDestructor()) {
+        require(
+          isa<DropDeinitInst>(lookThroughOwnershipInsts(DSI->getOperand())),
+            "a destructure of a move-only-type-with-deinit requires a "
+            "drop_deinit");
+      }
     }
   }
 
@@ -5968,12 +5975,40 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
             "Result and operand must have the same type, today.");
   }
 
+  // check that a drop_deinit can only ever be destroyed or destructured
+  void checkDropDeinitUses(DropDeinitInst *ddi) {
+    // Address-type drop_deinit has no special structural requirements. It just
+    // sits there and blocks optimization on the allocation and downstream uses
+    // of the address. If we want to optimize around address-type drop_deinit,
+    // then we need a seperate verifier for its requirements.
+    if (ddi->getType().isAddress())
+      return;
+
+    visitNonOwnershipUses(ddi, [&](Operand *use) {
+      auto *user = use->getUser();
+      require(isa<DestroyValueInst>(user)
+              || isa<EndLifetimeInst>(user)
+              || isa<DestructureStructInst>(user)
+              || isa<SwitchEnumInst>(user),
+              "A drop_deinit can only be destroyed or destructured");
+      return true;
+    });
+  }
+
   void checkDropDeinitInst(DropDeinitInst *ddi) {
-    require(ddi->getType() == ddi->getOperand()->getType(),
+    require(F.hasOwnership(), "drop_deinit only allowed in OSSA");
+
+    auto type = ddi->getType();
+    require(type == ddi->getOperand()->getType(),
             "Result and operand must have the same type.");
-    require(ddi->getType().isMoveOnlyNominalType(),
+    require(type.isMoveOnlyNominalType(),
             "drop_deinit only allowed for move-only types");
-    require(F.hasOwnership(), "drop_deinit only allowed in OSSA");
+    require(type.getNominalOrBoundGenericNominal()
+            ->getValueTypeDestructor(), "drop_deinit only allowed for "
+            "struct/enum types that define a deinit");
+    assert(!type.isTrivial(F) && "a type with a deinit is nontrivial");
+
+    checkDropDeinitUses(ddi);
   }
 
   void checkMarkMustCheckInst(MarkMustCheckInst *i) {