Skip to content

Commit f05226d

Browse files
ChuanqiXu9tru
ChuanqiXu9
authored and
tru
committed
[C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty
Close #56301 Close #64151 See the summary and the discussion of https://reviews.llvm.org/D157070 to get the full context. As @rjmccall pointed out, the key point of the root cause is that currently we didn't implement the semantics for '@llvm.coro.save' well ("after the await-ready returns false, the coroutine is considered to be suspended ") well. Since the semantics implies that we (the compiler) shouldn't write the spills into the coroutine frame in the await_suspend. But now it is possible due to some combinations of the optimizations so the semantics are broken. And the inlining is the root optimization of such optimizations. So in this patch, we tried to add the `noinline` attribute to the await_suspend call. Also as an optimization, we don't add the `noinline` attribute to the await_suspend call if the awaiter is an empty class. This should be correct since the programmers can't access the local variables in await_suspend if the awaiter is empty. I think this is necessary for the performance since it is pretty common. Another potential optimization is: call @llvm.coro.await_suspend(ptr %awaiter, ptr %handle, ptr @awaitSuspendFn) Then it is much easier to perform the safety analysis in the middle end. If it is safe to inline the call to awaitSuspend, we can replace it in the CoroEarly pass. Otherwise we could replace it in the CoroSplit pass. Reviewed By: rjmccall Differential Revision: https://reviews.llvm.org/D157833
1 parent 2641da8 commit f05226d

File tree

6 files changed

+358
-0
lines changed

6 files changed

+358
-0
lines changed

‎clang/docs/ReleaseNotes.rst

+4
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,10 @@ Bug Fixes in This Version
702702
- Fix a hang on valid C code passing a function type as an argument to
703703
``typeof`` to form a function declaration.
704704
(`#64713 <https://github.com/llvm/llvm-project/issues/64713>_`)
705+
- Fixed an issue where accesses to the local variables of a coroutine during
706+
``await_suspend`` could be misoptimized, including accesses to the awaiter
707+
object itself.
708+
(`#56301 <https://github.com/llvm/llvm-project/issues/56301>`_)
705709

706710
Bug Fixes to Compiler Builtins
707711
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

‎clang/lib/CodeGen/CGCall.cpp

+24
Original file line numberDiff line numberDiff line change
@@ -5487,6 +5487,30 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
54875487
Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::AlwaysInline);
54885488
}
54895489

5490+
// The await_suspend call performed by co_await is essentially asynchronous
5491+
// to the execution of the coroutine. Inlining it normally into an unsplit
5492+
// coroutine can cause miscompilation because the coroutine CFG misrepresents
5493+
// the true control flow of the program: things that happen in the
5494+
// await_suspend are not guaranteed to happen prior to the resumption of the
5495+
// coroutine, and things that happen after the resumption of the coroutine
5496+
// (including its exit and the potential deallocation of the coroutine frame)
5497+
// are not guaranteed to happen only after the end of await_suspend.
5498+
//
5499+
// The short-term solution to this problem is to mark the call as uninlinable.
5500+
// But we don't want to do this if the call is known to be trivial, which is
5501+
// very common.
5502+
//
5503+
// The long-term solution may introduce patterns like:
5504+
//
5505+
// call @llvm.coro.await_suspend(ptr %awaiter, ptr %handle,
5506+
// ptr @awaitSuspendFn)
5507+
//
5508+
// Then it is much easier to perform the safety analysis in the middle end.
5509+
// If it is safe to inline the call to awaitSuspend, we can replace it in the
5510+
// CoroEarly pass. Otherwise we could replace it in the CoroSplit pass.
5511+
if (inSuspendBlock() && mayCoroHandleEscape())
5512+
Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);
5513+
54905514
// Disable inlining inside SEH __try blocks.
54915515
if (isSEHTryScope()) {
54925516
Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);

‎clang/lib/CodeGen/CGCoroutine.cpp

+33
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,36 @@ static bool memberCallExpressionCanThrow(const Expr *E) {
139139
return true;
140140
}
141141

142+
/// Return true when the coroutine handle may escape from the await-suspend
143+
/// (`awaiter.await_suspend(std::coroutine_handle)` expression).
144+
/// Return false only when the coroutine wouldn't escape in the await-suspend
145+
/// for sure.
146+
///
147+
/// While it is always safe to return true, return falses can bring better
148+
/// performances.
149+
///
150+
/// See https://github.com/llvm/llvm-project/issues/56301 and
151+
/// https://reviews.llvm.org/D157070 for the example and the full discussion.
152+
///
153+
/// FIXME: It will be much better to perform such analysis in the middle end.
154+
/// See the comments in `CodeGenFunction::EmitCall` for example.
155+
static bool MayCoroHandleEscape(CoroutineSuspendExpr const &S) {
156+
CXXRecordDecl *Awaiter =
157+
S.getCommonExpr()->getType().getNonReferenceType()->getAsCXXRecordDecl();
158+
159+
// Return true conservatively if the awaiter type is not a record type.
160+
if (!Awaiter)
161+
return true;
162+
163+
// In case the awaiter type is empty, the suspend wouldn't leak the coroutine
164+
// handle.
165+
//
166+
// TODO: We can improve this by looking into the implementation of
167+
// await-suspend and see if the coroutine handle is passed to foreign
168+
// functions.
169+
return !Awaiter->field_empty();
170+
}
171+
142172
// Emit suspend expression which roughly looks like:
143173
//
144174
// auto && x = CommonExpr();
@@ -199,8 +229,11 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
199229
auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
200230

201231
CGF.CurCoro.InSuspendBlock = true;
232+
CGF.CurCoro.MayCoroHandleEscape = MayCoroHandleEscape(S);
202233
auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
203234
CGF.CurCoro.InSuspendBlock = false;
235+
CGF.CurCoro.MayCoroHandleEscape = false;
236+
204237
if (SuspendRet != nullptr && SuspendRet->getType()->isIntegerTy(1)) {
205238
// Veto suspension if requested by bool returning await_suspend.
206239
BasicBlock *RealSuspendBlock =

‎clang/lib/CodeGen/CodeGenFunction.h

+5
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,7 @@ class CodeGenFunction : public CodeGenTypeCache {
334334
struct CGCoroInfo {
335335
std::unique_ptr<CGCoroData> Data;
336336
bool InSuspendBlock = false;
337+
bool MayCoroHandleEscape = false;
337338
CGCoroInfo();
338339
~CGCoroInfo();
339340
};
@@ -347,6 +348,10 @@ class CodeGenFunction : public CodeGenTypeCache {
347348
return isCoroutine() && CurCoro.InSuspendBlock;
348349
}
349350

351+
bool mayCoroHandleEscape() const {
352+
return isCoroutine() && CurCoro.MayCoroHandleEscape;
353+
}
354+
350355
/// CurGD - The GlobalDecl for the current function being compiled.
351356
GlobalDecl CurGD;
352357

‎clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp

+207
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
// Tests that we can mark await-suspend as noinline correctly.
2+
//
3+
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s \
4+
// RUN: -disable-llvm-passes | FileCheck %s
5+
6+
#include "Inputs/coroutine.h"
7+
8+
struct Task {
9+
struct promise_type {
10+
struct FinalAwaiter {
11+
bool await_ready() const noexcept { return false; }
12+
template <typename PromiseType>
13+
std::coroutine_handle<> await_suspend(std::coroutine_handle<PromiseType> h) noexcept {
14+
return h.promise().continuation;
15+
}
16+
void await_resume() noexcept {}
17+
};
18+
19+
Task get_return_object() noexcept {
20+
return std::coroutine_handle<promise_type>::from_promise(*this);
21+
}
22+
23+
std::suspend_always initial_suspend() noexcept { return {}; }
24+
FinalAwaiter final_suspend() noexcept { return {}; }
25+
void unhandled_exception() noexcept {}
26+
void return_void() noexcept {}
27+
28+
std::coroutine_handle<> continuation;
29+
};
30+
31+
Task(std::coroutine_handle<promise_type> handle);
32+
~Task();
33+
34+
private:
35+
std::coroutine_handle<promise_type> handle;
36+
};
37+
38+
struct StatefulAwaiter {
39+
int value;
40+
bool await_ready() const noexcept { return false; }
41+
template <typename PromiseType>
42+
void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {}
43+
void await_resume() noexcept {}
44+
};
45+
46+
typedef std::suspend_always NoStateAwaiter;
47+
using AnotherStatefulAwaiter = StatefulAwaiter;
48+
49+
template <class T>
50+
struct TemplatedAwaiter {
51+
T value;
52+
bool await_ready() const noexcept { return false; }
53+
template <typename PromiseType>
54+
void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {}
55+
void await_resume() noexcept {}
56+
};
57+
58+
59+
class Awaitable {};
60+
StatefulAwaiter operator co_await(Awaitable) {
61+
return StatefulAwaiter{};
62+
}
63+
64+
StatefulAwaiter GlobalAwaiter;
65+
class Awaitable2 {};
66+
StatefulAwaiter& operator co_await(Awaitable2) {
67+
return GlobalAwaiter;
68+
}
69+
70+
Task testing() {
71+
co_await std::suspend_always{};
72+
co_await StatefulAwaiter{};
73+
co_await AnotherStatefulAwaiter{};
74+
75+
// Test lvalue case.
76+
StatefulAwaiter awaiter;
77+
co_await awaiter;
78+
79+
// The explicit call to await_suspend is not considered suspended.
80+
awaiter.await_suspend(std::coroutine_handle<void>::from_address(nullptr));
81+
82+
co_await TemplatedAwaiter<int>{};
83+
TemplatedAwaiter<int> TemplatedAwaiterInstace;
84+
co_await TemplatedAwaiterInstace;
85+
86+
co_await Awaitable{};
87+
co_await Awaitable2{};
88+
}
89+
90+
// CHECK-LABEL: @_Z7testingv
91+
92+
// Check `co_await __promise__.initial_suspend();` Since it returns std::suspend_always,
93+
// which is an empty class, we shouldn't generate optimization blocker for it.
94+
// CHECK: call token @llvm.coro.save
95+
// CHECK: call void @_ZNSt14suspend_always13await_suspendESt16coroutine_handleIvE{{.*}}#[[NORMAL_ATTR:[0-9]+]]
96+
97+
// Check the `co_await std::suspend_always{};` expression. We shouldn't emit the optimization
98+
// blocker for it since it is an empty class.
99+
// CHECK: call token @llvm.coro.save
100+
// CHECK: call void @_ZNSt14suspend_always13await_suspendESt16coroutine_handleIvE{{.*}}#[[NORMAL_ATTR]]
101+
102+
// Check `co_await StatefulAwaiter{};`. We need to emit the optimization blocker since
103+
// the awaiter is not empty.
104+
// CHECK: call token @llvm.coro.save
105+
// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR:[0-9]+]]
106+
107+
// Check `co_await AnotherStatefulAwaiter{};` to make sure that we can handle TypedefTypes.
108+
// CHECK: call token @llvm.coro.save
109+
// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]]
110+
111+
// Check `co_await awaiter;` to make sure we can handle lvalue cases.
112+
// CHECK: call token @llvm.coro.save
113+
// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]]
114+
115+
// Check `awaiter.await_suspend(...)` to make sure the explicit call the await_suspend won't be marked as noinline
116+
// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIvEEvSt16coroutine_handleIT_E{{.*}}#[[NORMAL_ATTR]]
117+
118+
// Check `co_await TemplatedAwaiter<int>{};` to make sure we can handle specialized template
119+
// type.
120+
// CHECK: call token @llvm.coro.save
121+
// CHECK: call void @_ZN16TemplatedAwaiterIiE13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]]
122+
123+
// Check `co_await TemplatedAwaiterInstace;` to make sure we can handle the lvalue from
124+
// specialized template type.
125+
// CHECK: call token @llvm.coro.save
126+
// CHECK: call void @_ZN16TemplatedAwaiterIiE13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]]
127+
128+
// Check `co_await Awaitable{};` to make sure we can handle awaiter returned by
129+
// `operator co_await`;
130+
// CHECK: call token @llvm.coro.save
131+
// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]]
132+
133+
// Check `co_await Awaitable2{};` to make sure we can handle awaiter returned by
134+
// `operator co_await` which returns a reference;
135+
// CHECK: call token @llvm.coro.save
136+
// CHECK: call void @_ZN15StatefulAwaiter13await_suspendIN4Task12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NOINLINE_ATTR]]
137+
138+
// Check `co_await __promise__.final_suspend();`. We don't emit an blocker here since it is
139+
// empty.
140+
// CHECK: call token @llvm.coro.save
141+
// CHECK: call ptr @_ZN4Task12promise_type12FinalAwaiter13await_suspendIS0_EESt16coroutine_handleIvES3_IT_E{{.*}}#[[NORMAL_ATTR]]
142+
143+
struct AwaitTransformTask {
144+
struct promise_type {
145+
struct FinalAwaiter {
146+
bool await_ready() const noexcept { return false; }
147+
template <typename PromiseType>
148+
std::coroutine_handle<> await_suspend(std::coroutine_handle<PromiseType> h) noexcept {
149+
return h.promise().continuation;
150+
}
151+
void await_resume() noexcept {}
152+
};
153+
154+
AwaitTransformTask get_return_object() noexcept {
155+
return std::coroutine_handle<promise_type>::from_promise(*this);
156+
}
157+
158+
std::suspend_always initial_suspend() noexcept { return {}; }
159+
FinalAwaiter final_suspend() noexcept { return {}; }
160+
void unhandled_exception() noexcept {}
161+
void return_void() noexcept {}
162+
163+
template <typename Awaitable>
164+
auto await_transform(Awaitable &&awaitable) {
165+
return awaitable;
166+
}
167+
168+
std::coroutine_handle<> continuation;
169+
};
170+
171+
AwaitTransformTask(std::coroutine_handle<promise_type> handle);
172+
~AwaitTransformTask();
173+
174+
private:
175+
std::coroutine_handle<promise_type> handle;
176+
};
177+
178+
struct awaitableWithGetAwaiter {
179+
bool await_ready() const noexcept { return false; }
180+
template <typename PromiseType>
181+
void await_suspend(std::coroutine_handle<PromiseType> h) noexcept {}
182+
void await_resume() noexcept {}
183+
};
184+
185+
AwaitTransformTask testingWithAwaitTransform() {
186+
co_await awaitableWithGetAwaiter{};
187+
}
188+
189+
// CHECK-LABEL: @_Z25testingWithAwaitTransformv
190+
191+
// Init suspend
192+
// CHECK: call token @llvm.coro.save
193+
// CHECK-NOT: call void @llvm.coro.opt.blocker(
194+
// CHECK: call void @_ZNSt14suspend_always13await_suspendESt16coroutine_handleIvE{{.*}}#[[NORMAL_ATTR]]
195+
196+
// Check `co_await awaitableWithGetAwaiter{};`.
197+
// CHECK: call token @llvm.coro.save
198+
// CHECK-NOT: call void @llvm.coro.opt.blocker(
199+
// Check call void @_ZN23awaitableWithGetAwaiter13await_suspendIN18AwaitTransformTask12promise_typeEEEvSt16coroutine_handleIT_E{{.*}}#[[NORMAL_ATTR]]
200+
201+
// Final suspend
202+
// CHECK: call token @llvm.coro.save
203+
// CHECK-NOT: call void @llvm.coro.opt.blocker(
204+
// CHECK: call ptr @_ZN18AwaitTransformTask12promise_type12FinalAwaiter13await_suspendIS0_EESt16coroutine_handleIvES3_IT_E{{.*}}#[[NORMAL_ATTR]]
205+
206+
// CHECK-NOT: attributes #[[NORMAL_ATTR]] = noinline
207+
// CHECK: attributes #[[NOINLINE_ATTR]] = {{.*}}noinline

0 commit comments

Comments
 (0)