Merge pull request #66314 from atrick/fix-dropdeinit

[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/SILOptimizer/PassManager/Passes.def

@@ -464,7 +464,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

@@ -1250,8 +1250,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

@@ -1581,6 +1581,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/Verifier/SILVerifier.cpp

@@ -3556,6 +3556,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");
+      }
     }
   }
 
@@ -5978,12 +5985,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) {

lib/SILGen/SILGenDestructor.cpp

@@ -262,7 +262,7 @@ void SILGenFunction::emitDeallocatingMoveOnlyDestructor(DestructorDecl *dd) {
   // Clean up our members, consuming our +1 self value as we do it.
   emitMoveOnlyMemberDestruction(selfValue,
                                 dd->getDeclContext()->getSelfNominalTypeDecl(),
-                                cleanupLoc, nullptr);
+                                cleanupLoc);
 
   // Return.
   B.createReturn(loc, emitEmptyTuple(loc));
@@ -497,111 +497,59 @@ void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
 
 void SILGenFunction::emitMoveOnlyMemberDestruction(SILValue selfValue,
                                                    NominalTypeDecl *nom,
-                                                   CleanupLocation cleanupLoc,
-                                                   SILBasicBlock *finishBB) {
+                                                   CleanupLocation cleanupLoc) {
+  // drop_deinit invalidates any user-defined struct/enum deinit
+  // before the individual members are destroyed.
   selfValue = B.createDropDeinit(cleanupLoc, selfValue);
-  if (selfValue->getType().isAddress()) {
-    if (auto *structDecl = dyn_cast<StructDecl>(nom)) {
-      for (VarDecl *vd : nom->getStoredProperties()) {
-        const TypeLowering &ti = getTypeLowering(vd->getType());
-        if (ti.isTrivial())
-          continue;
-
-        SILValue addr = B.createStructElementAddr(
-            cleanupLoc, selfValue, vd, ti.getLoweredType().getAddressType());
-        addr = B.createBeginAccess(cleanupLoc, addr, SILAccessKind::Deinit,
-                                   SILAccessEnforcement::Static,
-                                   false /*noNestedConflict*/,
-                                   false /*fromBuiltin*/);
-        B.createDestroyAddr(cleanupLoc, addr);
-        B.createEndAccess(cleanupLoc, addr, false /*is aborting*/);
-      }
-    } else {
-      auto *origBlock = B.getInsertionBB();
-      auto *enumDecl = cast<EnumDecl>(nom);
-      SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 8>
-          caseCleanups;
-      auto *contBlock = createBasicBlock();
-
-      for (auto *enumElt : enumDecl->getAllElements()) {
-        auto *enumBlock = createBasicBlock();
-        SILBuilder builder(enumBlock, enumBlock->begin());
-
-        if (enumElt->hasAssociatedValues()) {
-          auto *take = builder.createUncheckedTakeEnumDataAddr(
-              cleanupLoc, selfValue, enumElt);
-          builder.createDestroyAddr(cleanupLoc, take);
-        }
-
-        // Branch to the continue trampoline block.
-        builder.createBranch(cleanupLoc, contBlock);
-        caseCleanups.emplace_back(enumElt, enumBlock);
-
-        // Set the insertion point to after this enum block so we insert the
-        // next new block after this block.
-        B.setInsertionPoint(enumBlock);
-      }
+  if (selfValue->getType().isObject()) {
+    // A destroy value that uses the result of a drop_deinit implicitly performs
+    // memberwise destruction.
+    B.emitDestroyValueOperation(cleanupLoc, selfValue);
+    return;
+  }
+  if (auto *structDecl = dyn_cast<StructDecl>(nom)) {
+    for (VarDecl *vd : nom->getStoredProperties()) {
+      const TypeLowering &ti = getTypeLowering(vd->getType());
+      if (ti.isTrivial())
+        continue;
 
-      B.setInsertionPoint(origBlock);
-      B.createSwitchEnumAddr(cleanupLoc, selfValue, nullptr, caseCleanups);
-      B.setInsertionPoint(contBlock);
+      SILValue addr = B.createStructElementAddr(
+          cleanupLoc, selfValue, vd, ti.getLoweredType().getAddressType());
+      addr = B.createBeginAccess(
+          cleanupLoc, addr, SILAccessKind::Deinit, SILAccessEnforcement::Static,
+          false /*noNestedConflict*/, false /*fromBuiltin*/);
+      B.createDestroyAddr(cleanupLoc, addr);
+      B.createEndAccess(cleanupLoc, addr, false /*is aborting*/);
     }
   } else {
-    if (auto *sd = dyn_cast<StructDecl>(nom)) {
-      if (llvm::any_of(sd->getStoredProperties(),
-                       [&](VarDecl *vd) {
-                         auto &lowering = getTypeLowering(vd->getType());
-                         return !lowering.isTrivial();
-                       })) {
-        auto *d = B.createDestructureStruct(cleanupLoc, selfValue,
-                                            OwnershipKind::Owned);
-        for (auto result : d->getResults()) {
-          B.emitDestroyValueOperation(cleanupLoc, result);
-        }
-      } else {
-        // If we only contain trivial uses, we don't have anything to cleanup,
-        // so just insert an end_lifetime.
-        B.createEndLifetime(cleanupLoc, selfValue);
+    auto *origBlock = B.getInsertionBB();
+    auto *enumDecl = cast<EnumDecl>(nom);
+    SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 8> caseCleanups;
+    auto *contBlock = createBasicBlock();
+
+    for (auto *enumElt : enumDecl->getAllElements()) {
+      auto *enumBlock = createBasicBlock();
+      SILBuilder builder(enumBlock, enumBlock->begin());
+
+      if (enumElt->hasAssociatedValues()) {
+        auto *take = builder.createUncheckedTakeEnumDataAddr(
+            cleanupLoc, selfValue, enumElt);
+        builder.createDestroyAddr(cleanupLoc, take);
       }
-    } else {
-      auto *origBlock = B.getInsertionBB();
-      auto *enumDecl = dyn_cast<EnumDecl>(nom);
-      SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 8>
-          caseCleanups;
-      auto *contBlock = createBasicBlock();
-
-      for (auto *enumElt : enumDecl->getAllElements()) {
-        auto *enumBlock = createBasicBlock();
-        SILBuilder builder(enumBlock, enumBlock->begin());
-
-        if (enumElt->hasAssociatedValues()) {
-          auto caseType = selfValue->getType().getEnumElementType(
-              enumElt, enumBlock->getParent());
-          auto *phiArg =
-              enumBlock->createPhiArgument(caseType, OwnershipKind::Owned);
-          builder.emitDestroyValueOperation(cleanupLoc, phiArg);
-        }
 
-        // Branch to the continue trampoline block.
-        builder.createBranch(cleanupLoc, contBlock);
-        caseCleanups.emplace_back(enumElt, enumBlock);
+      // Branch to the continue trampoline block.
+      builder.createBranch(cleanupLoc, contBlock);
+      caseCleanups.emplace_back(enumElt, enumBlock);
 
-        // Set the insertion point to after this enum block so we insert the
-        // next new block after this block.
-        B.setInsertionPoint(enumBlock);
-      }
-
-      B.setInsertionPoint(origBlock);
-      B.createSwitchEnum(cleanupLoc, selfValue, nullptr, caseCleanups);
-      B.setInsertionPoint(contBlock);
+      // Set the insertion point to after this enum block so we insert the
+      // next new block after this block.
+      B.setInsertionPoint(enumBlock);
     }
-  }
 
-  if (finishBB)
-    B.createBranch(cleanupLoc, finishBB);
-
-  if (finishBB)
-    B.emitBlock(finishBB);
+    B.setInsertionPoint(origBlock);
+    B.createSwitchEnumAddr(cleanupLoc, selfValue, nullptr, caseCleanups);
+    B.setInsertionPoint(contBlock);
+  }
 }
 
 void SILGenFunction::emitObjCDestructor(SILDeclRef dtor) {

lib/SILGen/SILGenFunction.h

@@ -815,12 +815,8 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   ///
   /// \param selfValue The 'self' value.
   /// \param nd The nominal declaration whose members are being destroyed.
-  /// \param finishBB If set, used as the basic block after members have been
-  ///                 destroyed, and we're ready to perform final cleanups
-  ///                 before returning.
   void emitMoveOnlyMemberDestruction(SILValue selfValue, NominalTypeDecl *nd,
-                                     CleanupLocation cleanupLoc,
-                                     SILBasicBlock *finishBB);
+                                     CleanupLocation cleanupLoc);
 
   /// Generates code to destroy linearly recursive data structures, without
   /// building up the call stack.