[Distributed] deinit aware of remote "properties" (lack thereof in storage)

lib/SILGen/SILGenDestructor.cpp

@@ -201,7 +201,15 @@ void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
                                                 ClassDecl *cd,
                                                 CleanupLocation cleanupLoc) {
   assert(selfValue.getOwnershipKind() == OwnershipKind::Guaranteed);
-  for (VarDecl *vd : cd->getStoredProperties()) {
+  ASTContext &ctx = getASTContext();
+
+  auto loc = SILLocation(cd);
+  loc.markAutoGenerated();
+
+  SILBasicBlock* normalMemberDestroyBB = nullptr;
+  SILBasicBlock* returnBB = nullptr;
+
+  auto destroyVar = [&](VarDecl *vd) {
     const TypeLowering &ti = getTypeLowering(vd->getType());
     if (!ti.isTrivial()) {
       SILValue addr =
@@ -213,15 +221,96 @@ void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
       B.createDestroyAddr(cleanupLoc, addr);
       B.createEndAccess(cleanupLoc, addr, false /*is aborting*/);
     }
+  };
+
+  if (cd->isDistributedActor()) {
+    auto selfTy = cd->getDeclaredInterfaceType();
+
+    Scope scope(Cleanups, CleanupLocation(loc));
+
+    auto isLocalBB = createBasicBlock();
+    normalMemberDestroyBB = createBasicBlock();
+    auto remoteMemberDestroyBB = createBasicBlock();
+    returnBB = createBasicBlock();
+
+    // TODO(distributed): de-duplicate with the thunk logic in SILGenDistributed
+    // if __isRemoteActor(self) {
+    //   ...
+    // } else {
+    //   ...
+    // }
+    {
+      FuncDecl *isRemoteFn = ctx.getIsRemoteDistributedActor();
+      assert(isRemoteFn && "Could not find 'is remote' function, is the "
+                           "'_Distributed' module available?");
+
+      ManagedValue selfAnyObject =
+          B.createInitExistentialRef(loc, getLoweredType(ctx.getAnyObjectType()),
+                                     CanType(selfTy), selfValue, {});
+      auto result = emitApplyOfLibraryIntrinsic(
+          loc, isRemoteFn, SubstitutionMap(), {selfAnyObject}, SGFContext());
+
+      SILValue isRemoteResult =
+          std::move(result).forwardAsSingleValue(*this, loc);
+      SILValue isRemoteResultUnwrapped =
+          emitUnwrapIntegerResult(loc, isRemoteResult);
+
+      B.createCondBranch(loc, isRemoteResultUnwrapped, remoteMemberDestroyBB, isLocalBB);
+    }
+
+    // // if __isRemoteActor(self)
+    // {
+    //  // destroy only self.id and self.actorTransport
+    // }
+    {
+      B.emitBlock(remoteMemberDestroyBB);
+
+      for (VarDecl *vd : cd->getStoredProperties()) {
+        if (!vd->getAttrs().hasAttribute<DistributedActorIndependentAttr>())
+          continue;
+
+        destroyVar(vd);
+      }
+
+      B.createBranch(loc, returnBB);
+    }
+
+    // // else (local distributed actor)
+    // {
+    //   <continue normal deinit>
+    // }
+    {
+      B.emitBlock(isLocalBB);
+
+      B.createBranch(loc, normalMemberDestroyBB);
+    }
+  }
+
+  {
+    if (normalMemberDestroyBB)
+      B.emitBlock(normalMemberDestroyBB);
+
+    for (VarDecl *vd : cd->getStoredProperties())
+      destroyVar(vd);
+
+    if (returnBB)
+      B.createBranch(loc, returnBB);
   }
 
-  if (cd->isRootDefaultActor()) {
-    auto builtinName = getASTContext().getIdentifier(
-      getBuiltinName(BuiltinValueKind::DestroyDefaultActor));
-    auto resultTy = SGM.Types.getEmptyTupleType();
+  {
+    if (returnBB)
+      B.emitBlock(returnBB);
+
+
+    if (cd->isRootDefaultActor()) {
+      // TODO(distributed): we may need to call the distributed destroy here instead?
+      auto builtinName = getASTContext().getIdentifier(
+          getBuiltinName(BuiltinValueKind::DestroyDefaultActor));
+      auto resultTy = SGM.Types.getEmptyTupleType();
 
-    B.createBuiltin(cleanupLoc, builtinName, resultTy, /*subs*/{},
-                    { selfValue.getValue() });
+      B.createBuiltin(cleanupLoc, builtinName, resultTy, /*subs*/{},
+                      { selfValue.getValue() });
+    }
   }
 }
 

lib/SILGen/SILGenDistributed.cpp

@@ -102,7 +102,6 @@ static void emitDistributedActorStore_transport(
     SILValue actorSelf, AbstractFunctionDecl *func,
     SILArgument *transportArg) {
   auto &B = SGF.B;
-  auto &F = SGF.F;
   auto &SGM = SGF.SGM;
   SILGenFunctionBuilder builder(SGM);
 
@@ -118,7 +117,6 @@ static void emitDistributedActorStore_transport(
   auto *var = dyn_cast<VarDecl>(vars.front());
 
   // ----
-  auto *selfTyDecl = func->getParent()->getSelfNominalTypeDecl();
   auto fieldAddr = B.createRefElementAddr(
       loc, actorSelf, var,
       SGF.getLoweredType(var->getInterfaceType()));
@@ -127,7 +125,7 @@ static void emitDistributedActorStore_transport(
   B.createCopyAddr(loc,
                    /*src*/transportArg,
                    /*dest*/fieldAddr,
-                   IsNotTake, IsInitialization); // TODO(distributed): should it be IsTake?
+                   IsNotTake, IsInitialization);
 }
 
 // TODO(distributed): remove this store impl and reuse Store_transport
@@ -151,7 +149,6 @@ emitDistributedActor_init_transportStore(
   ManagedValue transportArgManaged = ManagedValue::forUnmanaged(transportArgValue);
 
   // ----
-  auto *selfTyDecl = ctor->getParent()->getSelfNominalTypeDecl();
   auto transportFieldAddr = B.createRefElementAddr(
       loc, borrowedSelfArg.getValue(), var,
       SGF.getLoweredType(var->getInterfaceType()));
@@ -197,7 +194,7 @@ static void emitDistributedActorStore_id(
   B.createCopyAddr(loc,
                    /*src*/identityArg,
                    /*dest*/fieldAddr,
-                   IsNotTake, IsInitialization); // TODO(distributed): should it be IsTake?
+                   IsNotTake, IsInitialization);
 }
 
 /// Synthesize the distributed actor's identity (`id`) initialization:
@@ -819,7 +816,7 @@ void SILGenFunction::emitDistributedThunk(SILDeclRef thunk) {
     }
   }
 
-  // TODO: to use a emitAndBranch local function, since these four blocks are so similar
+  // TODO(distributed): to use a emitAndBranch local function, since these four blocks are so similar
 
   {
     B.emitBlock(remoteErrorBB);

test/Distributed/Runtime/distributed_actor_remote_fieldsDontCrashDeinit.swift

@@ -8,7 +8,6 @@
 // UNSUPPORTED: use_os_stdlib
 // UNSUPPORTED: back_deployment_runtime
 
-// REQUIRES: rdar78290608
 
 import _Distributed
 

test/SIL/Distributed/distributed_actor_default_deinit_sil.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend -O -primary-file %s -emit-sil -enable-experimental-distributed | %FileCheck %s
+// RUN: %target-swift-frontend -O -primary-file %s -emit-sil -enable-experimental-distributed | %FileCheck %s --dump-input=fail
 
 import _Distributed
 
@@ -38,7 +38,7 @@ distributed actor SimpleEmptyDistributedActor {
 // CHECK: bb3:
 // CHECK:  destroy_addr [[ID_ADDR]] : $*AnyActorIdentity
 // CHECK:  destroy_addr [[TRANSPORT_ADDR]] : $*ActorTransport
-// CHECK:  %16 = builtin "destroyDefaultActor"(%0 : $SimpleEmptyDistributedActor) : $()
-// CHECK:  %17 = unchecked_ref_cast %0 : $SimpleEmptyDistributedActor to $Builtin.NativeObject
-// CHECK:  return %17 : $Builtin.NativeObject
+// CHECK:  [[_:%[0-9]+]] = builtin "destroyDefaultActor"(%0 : $SimpleEmptyDistributedActor) : $()
+// CHECK:  [[SELF:%[0-9]+]] = unchecked_ref_cast %0 : $SimpleEmptyDistributedActor to $Builtin.NativeObject
+// CHECK:  return [[SELF]] : $Builtin.NativeObject
 // CHECK: } // end sil function '$s36distributed_actor_default_deinit_sil27SimpleEmptyDistributedActorCfd'

test/SIL/Distributed/distributed_actor_remote_deinit_sil.swift

@@ -0,0 +1,97 @@
+// RUN: %target-swift-frontend -O -primary-file %s -emit-sil -enable-experimental-distributed | %FileCheck %s --dump-input=fail
+
+import _Distributed
+
+class SomeClass {}
+
+@available(SwiftStdlib 5.5, *)
+actor SimpleActor {
+  let someFieldInLocalActor: SomeClass
+  init(field: SomeClass) {
+    self.someFieldInLocalActor = field
+  }
+}
+
+
+@available(SwiftStdlib 5.5, *)
+distributed actor SimpleEmptyDistributedActor {
+  let localOnlyField: SomeClass
+
+  init(field: SomeClass, transport: ActorTransport) {
+    self.localOnlyField = field
+  }
+}
+
+// ==== ------------------------------------------------------------------------
+// ==== Check that a normal local only actor is left unchanged
+
+// CHECK: // SimpleActor.deinit
+// CHECK: sil hidden{{.*}} @$s35distributed_actor_remote_deinit_sil11SimpleActorCfd : $@convention(method) (@guaranteed SimpleActor) -> @owned Builtin.NativeObject {
+// CHECK: // %0 "self" // users: %6, %5, %2, %1
+// CHECK: bb0(%0 : $SimpleActor):
+// CHECK:  debug_value %0 : $SimpleActor, let, name "self", argno 1, implicit // id: %1
+// CHECK:  %2 = ref_element_addr %0 : $SimpleActor, #SimpleActor.someFieldInLocalActor // user: %3
+// CHECK:  %3 = load %2 : $*SomeClass // user: %4
+// CHECK:  strong_release %3 : $SomeClass // id: %4
+// CHECK:  %5 = builtin "destroyDefaultActor"(%0 : $SimpleActor) : $()
+// CHECK:  %6 = unchecked_ref_cast %0 : $SimpleActor to $Builtin.NativeObject // user: %7
+// CHECK:  return %6 : $Builtin.NativeObject // id: %7
+// CHECK: } // end sil function '$s35distributed_actor_remote_deinit_sil11SimpleActorCfd'
+
+
+
+
+// ==== deinit must have the extra "if remote..." path emitted for the
+// distributed actor only. That path will not attempt deallocating the
+// `localOnly...` fields, since they were never initialized and have no storage.
+
+// CHECK: //  SimpleEmptyDistributedActor.deinit
+// sil hidden [available 12.0] @$s35distributed_actor_remote_deinit_sil27SimpleEmptyDistributedActorCfd : $@convention(method) (@guaranteed SimpleEmptyDistributedActor) -> @owned Builtin.NativeObject {
+// CHECK: //  [[SELF:%[0-9]+]] "self"
+// CHECK: bb0(%0 : $SimpleEmptyDistributedActor):
+// CHECK:  debug_value [[SELF]] : $SimpleEmptyDistributedActor, let, name "self", argno 1, implicit
+// CHECK:  [[IDENTITY_ADDR:%[0-9]+]] = ref_element_addr %0 : $SimpleEmptyDistributedActor, #SimpleEmptyDistributedActor.id // users: %13, %24
+// CHECK:  [[TRANSPORT_ADDR:%[0-9]+]] = ref_element_addr %0 : $SimpleEmptyDistributedActor, #SimpleEmptyDistributedActor.actorTransport
+// CHECK:  [[SELF_1:%[0-9]+]] = init_existential_ref %0 : $SimpleEmptyDistributedActor : $SimpleEmptyDistributedActor, $AnyObject
+// CHECK:  //  function_ref swift_distributed_actor_is_remote
+// CHECK:  [[IS_REMOTE_FN_1:%[0-9]+]] = function_ref @swift_distributed_actor_is_remote : $@convention(thin) (@guaranteed AnyObject) -> Bool
+// CHECK:  [[IS_REMOTE_FN_RES_1:%[0-9]+]] = apply [[IS_REMOTE_FN_1]]([[SELF_1]]) : $@convention(thin) (@guaranteed AnyObject) -> Bool
+// CHECK:  [[IS_REMOTE_BOOL_1:%[0-9]+]] = struct_extract [[IS_REMOTE_FN_RES_1]] : $Bool, #Bool._value
+// CHECK:  cond_br [[IS_REMOTE_BOOL_1]], [[BB_CONT_1:bb[0-9]+]], [[BB_RESIGN_DIST_IDENTITY:bb[0-9]+]]
+
+// CHECK: [[BB_CONT_1]]:
+// CHECK:  br [[BB_CHECK_REMOTE_OR_LOCAL_MEMBER_DEINIT_TYPE:bb[0-9]+]]
+
+// It was a local actor, so we resign the address:
+// CHECK: [[BB_RESIGN_DIST_IDENTITY]]:
+//         %11 = open_existential_addr immutable_access %4 : $*ActorTransport to $*@opened({{.*}}) ActorTransport // users: %13, %13, %12
+// CHECK:  [[RESIGN_FN:%[0-9]+]] = witness_method $@opened({{.*}}) ActorTransport, #ActorTransport.resignIdentity
+// CHECK:  [[RESIGNED:%[0-9]+]] = apply [[RESIGN_FN]]<@opened({{.*}}) ActorTransport>(%3, %11) : $@convention(witness_method: ActorTransport) <τ_0_0 where τ_0_0 : ActorTransport> (@in_guaranteed AnyActorIdentity, @in_guaranteed τ_0_0) -> ()
+// CHECK:  br [[BB_CHECK_REMOTE_OR_LOCAL_MEMBER_DEINIT_TYPE]]
+
+// Check if we must skip destroying local storage because actor was remote
+// CHECK: [[BB_CHECK_REMOTE_OR_LOCAL_MEMBER_DEINIT_TYPE]]:
+// CHECK:  %15 = init_existential_ref %0 : $SimpleEmptyDistributedActor : $SimpleEmptyDistributedActor, $AnyObject
+// CHECK:  %16 = apply %6(%15) : $@convention(thin) (@guaranteed AnyObject) -> Bool
+// CHECK:  %17 = struct_extract %16 : $Bool, #Bool._value // user: %18
+// CHECK:  cond_br %17, [[BB_CONT_REMOTE_DONT_DESTROY_LOCAL_MEMBERS:bb[0-9]+]], [[BB_CONT_DESTROY_LOCAL_THEN_INDEPENDENT_MEMBERS:bb[0-9]+]]
+
+// CHECK: [[BB_CONT_REMOTE_DONT_DESTROY_LOCAL_MEMBERS]]:
+// CHECK:  br [[BB_DESTROY_INDEPENDENT_MEMBERS:bb[0-9]+]]
+
+// We were a local instance after all, and thus must destroy local properties
+// CHECK: [[BB_CONT_DESTROY_LOCAL_THEN_INDEPENDENT_MEMBERS]]:
+// CHECK:  [[FIELD_ADDR:%[0-9]+]] = ref_element_addr %0 : $SimpleEmptyDistributedActor, #SimpleEmptyDistributedActor.localOnlyField
+// CHECK:  [[LOAD_FIELD_ADDR:%[0-9]+]] = load [[FIELD_ADDR]] : $*SomeClass
+// CHECK:  strong_release [[LOAD_FIELD_ADDR]] : $SomeClass
+// CHECK:  br [[BB_DESTROY_INDEPENDENT_MEMBERS]]
+
+// Destroy "distributed nonisolated" fields and the actor itself
+// CHECK: [[BB_DESTROY_INDEPENDENT_MEMBERS]]:
+// CHECK:  destroy_addr [[IDENTITY_ADDR]] : $*AnyActorIdentity
+// CHECK:  destroy_addr [[TRANSPORT_ADDR]] : $*ActorTransport
+// CHECK:  {{.*}} = builtin "destroyDefaultActor"(%0 : $SimpleEmptyDistributedActor) : $()
+// CHECK:  dealloc_ref [[SELF]] : $SimpleEmptyDistributedActor
+// CHECK:  [[EMPTY:%[0-9]+]] = tuple ()
+// CHECK:  return [[EMPTY]] : $()
+// } // end sil function '$s35distributed_actor_remote_deinit_sil27SimpleEmptyDistributedActorCfD'