Clean up the TaskGroup ABI:

rjmccall · rjmccall · commit efeb8181610f · 2021-04-09T03:06:31.000-04:00
- stop storing the parent task in the TaskGroup at the .swift level
- make sure that swift_taskGroup_isCancelled is implied by the parent
  task being cancelled
- make the TaskGroup structs frozen
- make the withTaskGroup functions inlinable
- remove swift_taskGroup_create
- teach IRGen to allocate memory for the task group
- don't deallocate the task group in swift_taskGroup_destroy

To achieve the allocation change, introduce paired create/destroy builtins.

Furthermore, remove the _swiftRetain and _swiftRelease functions and
several calls to them.  Replace them with uses of the appropriate builtins.
I should probably change the builtins to return retained, since they're
working with a managed type, but I'll do that in a separate commit.
diff --git a/include/swift/AST/Builtins.def b/include/swift/AST/Builtins.def
@@ -740,6 +740,14 @@ BUILTIN_MISC_OPERATION(ResumeThrowingContinuationThrowing,
 BUILTIN_MISC_OPERATION(BuildSerialExecutorRef,
                        "buildSerialExecutorRef", "", Special)
 
+/// Create a task group.
+BUILTIN_MISC_OPERATION(CreateTaskGroup,
+                       "createTaskGroup", "", Special)
+
+/// Destroy a task group.
+BUILTIN_MISC_OPERATION(DestroyTaskGroup,
+                       "destroyTaskGroup", "", Special)
+
 // BUILTIN_MISC_OPERATION_WITH_SILGEN - Miscellaneous operations that are
 // specially emitted during SIL generation.
 //
diff --git a/include/swift/Basic/Features.def b/include/swift/Basic/Features.def
@@ -44,5 +44,6 @@ LANGUAGE_FEATURE(GlobalActors, 0, "Global actors", langOpts.EnableExperimentalCo
 LANGUAGE_FEATURE(BuiltinJob, 0, "Builtin.Job type", true)
 LANGUAGE_FEATURE(Sendable, 0, "Sendable and @Sendable", true)
 LANGUAGE_FEATURE(BuiltinContinuation, 0, "Continuation builtins", true)
+LANGUAGE_FEATURE(BuiltinTaskGroup, 0, "TaskGroup builtins", true)
 
 #undef LANGUAGE_FEATURE
diff --git a/include/swift/Runtime/Concurrency.h b/include/swift/Runtime/Concurrency.h
@@ -178,17 +178,6 @@ void swift_taskGroup_wait_next_throwing(
     OpaqueValue *resultPointer, SWIFT_ASYNC_CONTEXT AsyncContext *rawContext,
     TaskGroup *group, const Metadata *successType);
 
-/// Create a new `TaskGroup`.
-/// The caller is responsible for retaining and managing the group's lifecycle.
-///
-/// Its Swift signature is
-///
-/// \code
-/// func swift_taskGroup_create() -> Builtin.RawPointer
-/// \endcode
-SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(swift)
-TaskGroup* swift_taskGroup_create(); // TODO: probably remove this call, and just use the initialize always
-
 /// Initialize a `TaskGroup` in the passed `group` memory location.
 /// The caller is responsible for retaining and managing the group's lifecycle.
 ///
diff --git a/include/swift/Runtime/RuntimeFunctions.def b/include/swift/Runtime/RuntimeFunctions.def
@@ -1672,21 +1672,21 @@ FUNCTION(DefaultActorDeallocateResilient,
          ARGS(RefCountedPtrTy),
          ATTRS(NoUnwind))
 
-//// void swift_taskGroup_initialize(AsyncTask *task, TaskGroup *group);
-//FUNCTION(TaskGroupInitialize,
-//         swift_taskGroup_initialize, SwiftCC,
-//         ConcurrencyAvailability,
-//         RETURNS(VoidTy),
-//         ARGS(RefCountedPtrTy, RefCountedPtrTy),
-//         ATTRS(NoUnwind))
-//
-//// void swift_taskGroup_destroy(AsyncTask *task, TaskGroup *group);
-//FUNCTION(TaskGroupDestroy,
-//         swift_taskGroup_destroy, SwiftCC,
-//         ConcurrencyAvailability,
-//         RETURNS(VoidTy),
-//         ARGS(RefCountedPtrTy, RefCountedPtrTy),
-//         ATTRS(NoUnwind))
+// void swift_taskGroup_initialize(TaskGroup *group);
+FUNCTION(TaskGroupInitialize,
+         swift_taskGroup_initialize, SwiftCC,
+         ConcurrencyAvailability,
+         RETURNS(VoidTy),
+         ARGS(Int8PtrTy),
+         ATTRS(NoUnwind))
+
+// void swift_taskGroup_destroy(TaskGroup *group);
+FUNCTION(TaskGroupDestroy,
+         swift_taskGroup_destroy, SwiftCC,
+         ConcurrencyAvailability,
+         RETURNS(VoidTy),
+         ARGS(Int8PtrTy),
+         ATTRS(NoUnwind))
 
 // AutoDiffLinearMapContext *swift_autoDiffCreateLinearMapContext(size_t);
 FUNCTION(AutoDiffCreateLinearMapContext,
diff --git a/lib/AST/ASTPrinter.cpp b/lib/AST/ASTPrinter.cpp
@@ -2724,6 +2724,10 @@ static bool usesFeatureBuiltinContinuation(Decl *decl) {
   return false;
 }
 
+static bool usesFeatureBuiltinTaskGroup(Decl *decl) {
+  return false;
+}
+
 /// Determine the set of "new" features used on a given declaration.
 ///
 /// Note: right now, all features we check for are "new". At some point, we'll
diff --git a/lib/AST/Builtins.cpp b/lib/AST/Builtins.cpp
@@ -1481,6 +1481,18 @@ static ValueDecl *getResumeContinuationThrowing(ASTContext &ctx,
                             _void);
 }
 
+static ValueDecl *getCreateTaskGroup(ASTContext &ctx, Identifier id) {
+  return getBuiltinFunction(ctx, id, _thin,
+                            _parameters(),
+                            _rawPointer);
+}
+
+static ValueDecl *getDestroyTaskGroup(ASTContext &ctx, Identifier id) {
+  return getBuiltinFunction(ctx, id, _thin,
+                            _parameters(_rawPointer),
+                            _void);
+}
+
 static ValueDecl *getBuildSerialExecutorRef(ASTContext &ctx, Identifier id) {
   // TODO: restrict the generic parameter to the SerialExecutor protocol
   return getBuiltinFunction(ctx, id, _thin,
@@ -2728,6 +2740,12 @@ ValueDecl *swift::getBuiltinValueDecl(ASTContext &Context, Identifier Id) {
   case BuiltinValueKind::DestroyDefaultActor:
     return getDefaultActorInitDestroy(Context, Id);
 
+  case BuiltinValueKind::CreateTaskGroup:
+    return getCreateTaskGroup(Context, Id);
+
+  case BuiltinValueKind::DestroyTaskGroup:
+    return getDestroyTaskGroup(Context, Id);
+
   case BuiltinValueKind::ResumeNonThrowingContinuationReturning:
   case BuiltinValueKind::ResumeThrowingContinuationReturning:
     return getResumeContinuationReturning(Context, Id);
diff --git a/lib/IRGen/GenBuiltin.cpp b/lib/IRGen/GenBuiltin.cpp
@@ -223,6 +223,17 @@ void irgen::emitBuiltinCall(IRGenFunction &IGF, const BuiltinInfo &Builtin,
   // getCurrentActor has no arguments.
   if (Builtin.ID == BuiltinValueKind::GetCurrentExecutor) {
     emitGetCurrentExecutor(IGF, out);
+
+    return;
+  }
+
+  if (Builtin.ID == BuiltinValueKind::CreateTaskGroup) {
+    out.add(emitCreateTaskGroup(IGF));
+    return;
+  }
+
+  if (Builtin.ID == BuiltinValueKind::DestroyTaskGroup) {
+    emitDestroyTaskGroup(IGF, args.claimNext());
     return;
   }
 
diff --git a/lib/IRGen/GenConcurrency.cpp b/lib/IRGen/GenConcurrency.cpp
@@ -170,3 +170,27 @@ void irgen::emitGetCurrentExecutor(IRGenFunction &IGF, Explosion &out) {
 
   IGF.emitAllExtractValues(call, IGF.IGM.SwiftExecutorTy, out);
 }
+
+llvm::Value *irgen::emitCreateTaskGroup(IRGenFunction &IGF) {
+  auto ty = llvm::ArrayType::get(IGF.IGM.Int8PtrTy, NumWords_TaskGroup);
+  auto address = IGF.createAlloca(ty, Alignment(Alignment_TaskGroup));
+  auto group = IGF.Builder.CreateBitCast(address.getAddress(),
+                                         IGF.IGM.Int8PtrTy);
+  IGF.Builder.CreateLifetimeStart(group);
+
+  auto *call = IGF.Builder.CreateCall(IGF.IGM.getTaskGroupInitializeFn(),
+                                      {group});
+  call->setDoesNotThrow();
+  call->setCallingConv(IGF.IGM.SwiftCC);
+
+  return group;
+}
+
+void irgen::emitDestroyTaskGroup(IRGenFunction &IGF, llvm::Value *group) {
+  auto *call = IGF.Builder.CreateCall(IGF.IGM.getTaskGroupDestroyFn(),
+                                      {group});
+  call->setDoesNotThrow();
+  call->setCallingConv(IGF.IGM.SwiftCC);
+
+  IGF.Builder.CreateLifetimeEnd(group);
+}
diff --git a/lib/IRGen/GenConcurrency.h b/lib/IRGen/GenConcurrency.h
@@ -36,6 +36,12 @@ void emitBuildSerialExecutorRef(IRGenFunction &IGF, llvm::Value *actor,
 /// Emit the getCurrentExecutor builtin.
 void emitGetCurrentExecutor(IRGenFunction &IGF, Explosion &out);
 
+/// Emit the createTaskGroup builtin.
+llvm::Value *emitCreateTaskGroup(IRGenFunction &IGF);
+
+/// Emit the destroyTaskGroup builtin.
+void emitDestroyTaskGroup(IRGenFunction &IGF, llvm::Value *group);
+
 } // end namespace irgen
 } // end namespace swift
 
diff --git a/lib/SIL/IR/OperandOwnership.cpp b/lib/SIL/IR/OperandOwnership.cpp
@@ -744,6 +744,8 @@ BUILTIN_OPERAND_OWNERSHIP(InstantaneousUse, PoundAssert)
 BUILTIN_OPERAND_OWNERSHIP(InstantaneousUse, GlobalStringTablePointer)
 BUILTIN_OPERAND_OWNERSHIP(InstantaneousUse, TypePtrAuthDiscriminator)
 BUILTIN_OPERAND_OWNERSHIP(InstantaneousUse, IntInstrprofIncrement)
+BUILTIN_OPERAND_OWNERSHIP(InstantaneousUse, CreateTaskGroup)
+BUILTIN_OPERAND_OWNERSHIP(InstantaneousUse, DestroyTaskGroup)
 
 BUILTIN_OPERAND_OWNERSHIP(ForwardingConsume, COWBufferForReading)
 BUILTIN_OPERAND_OWNERSHIP(ForwardingConsume, UnsafeGuaranteed)
diff --git a/lib/SIL/IR/ValueOwnership.cpp b/lib/SIL/IR/ValueOwnership.cpp
@@ -553,6 +553,8 @@ CONSTANT_OWNERSHIP_BUILTIN(None, ResumeNonThrowingContinuationReturning)
 CONSTANT_OWNERSHIP_BUILTIN(None, ResumeThrowingContinuationReturning)
 CONSTANT_OWNERSHIP_BUILTIN(None, ResumeThrowingContinuationThrowing)
 CONSTANT_OWNERSHIP_BUILTIN(None, BuildSerialExecutorRef)
+CONSTANT_OWNERSHIP_BUILTIN(None, CreateTaskGroup)
+CONSTANT_OWNERSHIP_BUILTIN(None, DestroyTaskGroup)
 
 #undef CONSTANT_OWNERSHIP_BUILTIN
 
diff --git a/lib/SIL/Utils/MemAccessUtils.cpp b/lib/SIL/Utils/MemAccessUtils.cpp
@@ -1858,6 +1858,8 @@ static void visitBuiltinAddress(BuiltinInst *builtin,
     case BuiltinValueKind::InitializeDefaultActor:
     case BuiltinValueKind::DestroyDefaultActor:
     case BuiltinValueKind::GetCurrentExecutor:
+    case BuiltinValueKind::CreateTaskGroup:
+    case BuiltinValueKind::DestroyTaskGroup:
       return;
 
     // General memory access to a pointer in first operand position.
diff --git a/lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp b/lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp
@@ -144,6 +144,8 @@ static bool isBarrier(SILInstruction *inst) {
     case BuiltinValueKind::GetCurrentAsyncTask:
     case BuiltinValueKind::GetCurrentExecutor:
     case BuiltinValueKind::AutoDiffCreateLinearMapContext:
+    case BuiltinValueKind::CreateTaskGroup:
+    case BuiltinValueKind::DestroyTaskGroup:
       return false;
 
     // Handle some rare builtins that may be sensitive to object lifetime
diff --git a/stdlib/public/CompatibilityOverride/CompatibilityOverrideConcurrency.def b/stdlib/public/CompatibilityOverride/CompatibilityOverrideConcurrency.def
@@ -145,10 +145,6 @@ OVERRIDE_TASK_GROUP(taskGroup_initialize, void,
                     SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),
                     swift::, (TaskGroup *group), (group))
 
-OVERRIDE_TASK_GROUP(taskGroup_create, TaskGroup *,
-                    SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),
-                    swift::, , )
-
 OVERRIDE_TASK_GROUP(taskGroup_attachChild, void,
                     SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),
                     swift::, (TaskGroup *group, AsyncTask *child),
diff --git a/stdlib/public/Concurrency/Task.cpp b/stdlib/public/Concurrency/Task.cpp
@@ -346,6 +346,10 @@ static AsyncTaskAndContext swift_task_create_group_future_commonImpl(
   if (flags.task_isChildTask()) {
     parent = swift_task_getCurrent();
     assert(parent != nullptr && "creating a child task with no active task");
+
+    // Inherit the priority of the parent task if unspecified.
+    if (flags.getPriority() == JobPriority::Unspecified)
+      flags.setPriority(parent->getPriority());
   }
 
   // Figure out the size of the header.
diff --git a/stdlib/public/Concurrency/Task.swift b/stdlib/public/Concurrency/Task.swift
@@ -58,7 +58,7 @@ extension Task {
     // FIXME: This retain seems pretty wrong, however if we don't we WILL crash
     //        with "destroying a task that never completed" in the task's destroy.
     //        How do we solve this properly?
-    _swiftRetain(_task)
+    Builtin.retain(_task)
 
     return Task(_task)
   }
@@ -561,7 +561,7 @@ extension Task {
     // FIXME: This retain seems pretty wrong, however if we don't we WILL crash
     //        with "destroying a task that never completed" in the task's destroy.
     //        How do we solve this properly?
-    _swiftRetain(_task)
+    Builtin.retain(_task)
     return UnsafeCurrentTask(_task)
   }
 }
@@ -591,7 +591,7 @@ public func withUnsafeCurrentTask<T>(body: (UnsafeCurrentTask?) throws -> T) ret
   // FIXME: This retain seems pretty wrong, however if we don't we WILL crash
   //        with "destroying a task that never completed" in the task's destroy.
   //        How do we solve this properly?
-  _swiftRetain(_task)
+  Builtin.retain(_task)
 
   return try body(UnsafeCurrentTask(_task))
 }
diff --git a/stdlib/public/Concurrency/TaskGroup.cpp b/stdlib/public/Concurrency/TaskGroup.cpp
@@ -470,18 +470,11 @@ static void swift_taskGroup_initializeImpl(TaskGroup *group) {
   assert(impl == record && "the group IS the task record");
 
   // ok, now that the group actually is initialized: attach it to the task
-  swift_task_addStatusRecord(record);
-}
+  bool notCancelled = swift_task_addStatusRecord(record);
 
-// =============================================================================
-// ==== create -----------------------------------------------------------------
-SWIFT_CC(swift)
-static TaskGroup *swift_taskGroup_createImpl() {
-  // TODO: John suggested we should rather create from a builtin, which would allow us to optimize allocations even more?
-  void *allocation = swift_task_alloc(sizeof(TaskGroup));
-  auto group = reinterpret_cast<TaskGroup *>(allocation);
-  swift_taskGroup_initialize(group);
-  return group;
+  // If the task has already been cancelled, reflect that immediately in
+  // the group status.
+  if (!notCancelled) impl->statusCancel();
 }
 
 // =============================================================================
@@ -514,9 +507,6 @@ void TaskGroupImpl::destroy() {
     taskDequeued = readyQueue.dequeue(item);
   }
   mutex.unlock(); // TODO: remove fragment lock, and use status for synchronization
-
-  // TODO: get the parent task, do we need to store it?
-  swift_task_dealloc(this);
 }
 
 // =============================================================================
diff --git a/stdlib/public/Concurrency/TaskGroup.swift b/stdlib/public/Concurrency/TaskGroup.swift
diff --git a/test/IRGen/async/builtins.sil b/test/IRGen/async/builtins.sil
diff --git a/unittests/runtime/CompatibilityOverrideConcurrency.cpp b/unittests/runtime/CompatibilityOverrideConcurrency.cpp
