From 519b1e907dc2f49be3f33fe8e55e05cea52ff97d Mon Sep 17 00:00:00 2001 From: Arnold Schwaighofer Date: Sat, 15 May 2021 07:44:27 -0700 Subject: [PATCH 1/2] [5.5] Make sure we tail call optimize a call in concurrency runtime's switch_task_impl. Without this hack the call will leave a stack frame around (not tail call optimized) and blow the stack if we call switch_task often enough. Ideally, clang would emit this call as `musttail` but currently it does not. rdar://76652421 --- stdlib/public/Concurrency/Actor.cpp | 11 +++++- test/Interpreter/async_fib.swift | 60 +++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 test/Interpreter/async_fib.swift diff --git a/stdlib/public/Concurrency/Actor.cpp b/stdlib/public/Concurrency/Actor.cpp index 7b4bae5c8733d..c3cba1e414517 100644 --- a/stdlib/public/Concurrency/Actor.cpp +++ b/stdlib/public/Concurrency/Actor.cpp @@ -1802,6 +1802,13 @@ static bool tryAssumeThreadForSwitch(ExecutorRef newExecutor, return false; } +__attribute__((noinline)) +SWIFT_CC(swiftasync) +static void force_tail_call_hack(AsyncTask *task) { + // This *should* be executed as a tail call. + return task->runInFullyEstablishedContext(); +} + /// Given that we've assumed control of an executor on this thread, /// continue to run the given task on it. SWIFT_CC(swiftasync) @@ -1815,7 +1822,9 @@ static void runOnAssumedThread(AsyncTask *task, ExecutorRef executor, oldTracking->setActiveExecutor(executor); // FIXME: force tail call - return task->runInFullyEstablishedContext(); + // return task->runInFullyEstablishedContext(); + // This hack "ensures" that this call gets executed as a tail call. + return force_tail_call_hack(task); } // Otherwise, set up tracking info. diff --git a/test/Interpreter/async_fib.swift b/test/Interpreter/async_fib.swift new file mode 100644 index 0000000000000..a447cc24dd10d --- /dev/null +++ b/test/Interpreter/async_fib.swift @@ -0,0 +1,60 @@ +// RUN: %empty-directory(%t) +// RUN: %target-build-swift -Xfrontend -enable-experimental-concurrency %s -parse-as-library -module-name main -o %t/main +// RUN: %target-codesign %t/main +// RUN: %target-run %t/main | %FileCheck %s + +// REQUIRES: concurrency +// REQUIRES: executable_test +// UNSUPPORTED: use_os_stdlib +// UNSUPPORTED: back_deployment_runtime +// UNSUPPORTED: OS=windows-msvc + +var gg = 0 + +@inline(never) +public func identity(_ x: T) -> T { + gg += 1 + return x +} + +actor Actor { + var x: Int = 0 + init(x: Int) { self.x = x } + + @inline(never) + func get(_ i: Int ) async -> Int { + return i + x + } +} + +// Used to crash with an stack overflow with m >= 18 +let m = 22 + +@inline(never) +func asyncFib(_ n: Int, _ a1: Actor, _ a2: Actor) async -> Int { + if n == 0 { + return await a1.get(n) + } + if n == 1 { + return await a2.get(n) + } + + let first = await asyncFib(n-2, a1, a2) + let second = await asyncFib(n-1, a1, a2) + + let result = first + second + + return result +} + +@main struct Main { + static func main() async { + let a1 = Actor(x: 0) + let a2 = Actor(x: 0) + _ = await asyncFib(identity(m), a1, a2) + + // CHECK: result: 0 + await print("result: \(a1.x)"); + await print(a2.x) + } +} From 684dbe2c44c877a2454788ac92f1a99575703fbe Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Mon, 17 May 2021 12:23:03 -0700 Subject: [PATCH 2/2] [5.5] [Runtime] Remove FIXMEs and hacks for tail calls. Reverts hack from 9b4f7d62cd2d09e9f24866d0d1f51de4b493ddc2. rdar://76652421 --- include/swift/ABI/Task.h | 12 ++++++++---- stdlib/public/Concurrency/Actor.cpp | 30 ++++++++++------------------- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/include/swift/ABI/Task.h b/include/swift/ABI/Task.h index 1ab356f57d33d..aaa4cd6f54d05 100644 --- a/include/swift/ABI/Task.h +++ b/include/swift/ABI/Task.h @@ -99,13 +99,15 @@ class alignas(2 * alignof(void*)) Job : public HeapObject { /// Given that we've fully established the job context in the current /// thread, actually start running this job. To establish the context /// correctly, call swift_job_run or runJobInExecutorContext. + SWIFT_CC(swiftasync) void runInFullyEstablishedContext(); /// Given that we've fully established the job context in the /// current thread, and that the job is a simple (non-task) job, /// actually start running this job. + SWIFT_CC(swiftasync) void runSimpleInFullyEstablishedContext() { - RunJob(this); + return RunJob(this); // 'return' forces tail call } }; @@ -224,8 +226,9 @@ class AsyncTask : public Job { /// in the current thread, start running this task. To establish /// the job context correctly, call swift_job_run or /// runInExecutorContext. + SWIFT_CC(swiftasync) void runInFullyEstablishedContext() { - ResumeTask(ResumeContext); + return ResumeTask(ResumeContext); // 'return' forces tail call } /// Check whether this task has been cancelled. @@ -486,11 +489,12 @@ static_assert(sizeof(AsyncTask) == 14 * sizeof(void*), static_assert(alignof(AsyncTask) == 2 * alignof(void*), "AsyncTask alignment is wrong"); +SWIFT_CC(swiftasync) inline void Job::runInFullyEstablishedContext() { if (auto task = dyn_cast(this)) - task->runInFullyEstablishedContext(); + return task->runInFullyEstablishedContext(); // 'return' forces tail call else - runSimpleInFullyEstablishedContext(); + return runSimpleInFullyEstablishedContext(); // 'return' forces tail call } /// An asynchronous context within a task. Generally contexts are diff --git a/stdlib/public/Concurrency/Actor.cpp b/stdlib/public/Concurrency/Actor.cpp index c3cba1e414517..5579d1a6e2a2c 100644 --- a/stdlib/public/Concurrency/Actor.cpp +++ b/stdlib/public/Concurrency/Actor.cpp @@ -1473,6 +1473,7 @@ static void swift_job_runImpl(Job *job, ExecutorRef executor) { /// the actor lives for the duration of job execution. /// Note that this may conflict with the retain/release /// design in the DefaultActorImpl, but it does fix bugs! +SWIFT_CC(swiftasync) static void processDefaultActor(DefaultActorImpl *currentActor, RunningJobInfo runner) { #if SWIFT_TASK_PRINTF_DEBUG @@ -1547,6 +1548,7 @@ static void processDefaultActor(DefaultActorImpl *currentActor, swift_release(actor); } +SWIFT_CC(swiftasync) void ProcessInlineJob::process(Job *job) { DefaultActorImpl *actor = DefaultActorImpl::fromInlineJob(job); @@ -1555,11 +1557,11 @@ void ProcessInlineJob::process(Job *job) { auto targetPriority = job->getPriority(); auto runner = RunningJobInfo::forInline(targetPriority); - // FIXME: force tail call swift_retain(actor); - return processDefaultActor(actor, runner); + return processDefaultActor(actor, runner); // 'return' forces tail call } +SWIFT_CC(swiftasync) void ProcessOutOfLineJob::process(Job *job) { auto self = cast(job); DefaultActorImpl *actor = self->Actor; @@ -1571,11 +1573,11 @@ void ProcessOutOfLineJob::process(Job *job) { delete self; - // FIXME: force tail call swift_retain(actor); - return processDefaultActor(actor, runner); + return processDefaultActor(actor, runner); // 'return' forces tail call } +SWIFT_CC(swiftasync) void ProcessOverrideJob::process(Job *job) { auto self = cast(job); @@ -1583,9 +1585,8 @@ void ProcessOverrideJob::process(Job *job) { auto actor = self->Actor; auto runner = RunningJobInfo::forOverride(self); - // FIXME: force tail call swift_retain(actor); - return processDefaultActor(actor, runner); + return processDefaultActor(actor, runner); // 'return' forces tail call } void DefaultActorImpl::enqueue(Job *job) { @@ -1802,13 +1803,6 @@ static bool tryAssumeThreadForSwitch(ExecutorRef newExecutor, return false; } -__attribute__((noinline)) -SWIFT_CC(swiftasync) -static void force_tail_call_hack(AsyncTask *task) { - // This *should* be executed as a tail call. - return task->runInFullyEstablishedContext(); -} - /// Given that we've assumed control of an executor on this thread, /// continue to run the given task on it. SWIFT_CC(swiftasync) @@ -1821,10 +1815,7 @@ static void runOnAssumedThread(AsyncTask *task, ExecutorRef executor, if (oldTracking) { oldTracking->setActiveExecutor(executor); - // FIXME: force tail call - // return task->runInFullyEstablishedContext(); - // This hack "ensures" that this call gets executed as a tail call. - return force_tail_call_hack(task); + return task->runInFullyEstablishedContext(); // 'return' forces tail call } // Otherwise, set up tracking info. @@ -1867,8 +1858,7 @@ static void swift_task_switchImpl(SWIFT_ASYNC_CONTEXT AsyncContext *resumeContex // we can just immediately continue running with the resume function // we were passed in. if (!currentExecutor.mustSwitchToRun(newExecutor)) { - // FIXME: force tail call - return resumeFunction(resumeContext); + return resumeFunction(resumeContext); // 'return' forces tail call } auto task = swift_task_getCurrent(); @@ -1892,7 +1882,7 @@ static void swift_task_switchImpl(SWIFT_ASYNC_CONTEXT AsyncContext *resumeContex fprintf(stderr, "[%p] switch succeeded, task %p assumed thread for executor %p\n", pthread_self(), task, newExecutor.getIdentity()); #endif giveUpThreadForSwitch(currentExecutor, runner); - // FIXME: force tail call + // 'return' forces tail call return runOnAssumedThread(task, newExecutor, trackingInfo, runner); }