Skip to content

Commit f1e885c

Browse files
committed
Improvements in response to comments
- Switch back to a member function attribute so that each overload can independently decide whether it uses `await_suspend_destroy()` - Emit a diagnostic when `await_suspend` and `await_suspend_destroy` have mismatched types - Add UseAwaitSuspendDestroy bit to CoSuspendExpr - Set UseAwaitSuspendDestroy bit from SemaCoroutine - Use UseAwaitSuspendDestroy in CGCoroutine - Move `lit` tests for diagnostics for the new attribute into `test/SemaCXX/` - Add new IR test for issue #148380 - Improve main IR test per feedback. - Improve main IR test to cover "multiple overloads in one awaiter" - Improve AttrDocs per feedback - clang-format
1 parent cf26f6b commit f1e885c

File tree

15 files changed

+535
-318
lines changed

15 files changed

+535
-318
lines changed

clang/docs/ReleaseNotes.rst

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,10 +154,12 @@ Attribute Changes in Clang
154154
--------------------------
155155

156156
- Introduced a new attribute ``[[clang::coro_await_suspend_destroy]]``. When
157-
applied to a coroutine awaiter class, it causes suspensions into this awaiter
158-
to use a new `await_suspend_destroy(Promise&)` method instead of the standard
159-
`await_suspend(std::coroutine_handle<...>)`. The coroutine is then destroyed.
160-
This improves code speed & size for "short-circuiting" coroutines.
157+
applied to an `await_suspend(std::coroutine_handle<Promise>)` member of a
158+
coroutine awaiter, it causes suspensions into this awaiter to use a new
159+
`await_suspend_destroy(Promise&)` method. The coroutine is then immediately
160+
destroyed. This flow bypasses the original `await_suspend()` (though it
161+
must contain a compatibility stub), and omits suspend intrinsics. The net
162+
effect is improved code speed & size for "short-circuiting" coroutines.
161163

162164
Improvements to Clang's diagnostics
163165
-----------------------------------
@@ -179,7 +181,7 @@ Improvements to Clang's diagnostics
179181
"format specifies type 'unsigned int' but the argument has type 'int', which differs in signedness [-Wformat-signedness]"
180182
"signedness of format specifier 'u' is incompatible with 'c' [-Wformat-signedness]"
181183
and the API-visible diagnostic id will be appropriate.
182-
184+
183185
- Fixed false positives in ``-Waddress-of-packed-member`` diagnostics when
184186
potential misaligned members get processed before they can get discarded.
185187
(#GH144729)

clang/include/clang/AST/ExprCXX.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5266,6 +5266,7 @@ class CoroutineSuspendExpr : public Expr {
52665266
: Expr(SC, Resume->getType(), Resume->getValueKind(),
52675267
Resume->getObjectKind()),
52685268
KeywordLoc(KeywordLoc), OpaqueValue(OpaqueValue) {
5269+
CoroutineSuspendExprBits.UseAwaitSuspendDestroy = false;
52695270
SubExprs[SubExpr::Operand] = Operand;
52705271
SubExprs[SubExpr::Common] = Common;
52715272
SubExprs[SubExpr::Ready] = Ready;
@@ -5279,6 +5280,7 @@ class CoroutineSuspendExpr : public Expr {
52795280
: Expr(SC, Ty, VK_PRValue, OK_Ordinary), KeywordLoc(KeywordLoc) {
52805281
assert(Common->isTypeDependent() && Ty->isDependentType() &&
52815282
"wrong constructor for non-dependent co_await/co_yield expression");
5283+
CoroutineSuspendExprBits.UseAwaitSuspendDestroy = false;
52825284
SubExprs[SubExpr::Operand] = Operand;
52835285
SubExprs[SubExpr::Common] = Common;
52845286
SubExprs[SubExpr::Ready] = nullptr;
@@ -5288,13 +5290,22 @@ class CoroutineSuspendExpr : public Expr {
52885290
}
52895291

52905292
CoroutineSuspendExpr(StmtClass SC, EmptyShell Empty) : Expr(SC, Empty) {
5293+
CoroutineSuspendExprBits.UseAwaitSuspendDestroy = false;
52915294
SubExprs[SubExpr::Operand] = nullptr;
52925295
SubExprs[SubExpr::Common] = nullptr;
52935296
SubExprs[SubExpr::Ready] = nullptr;
52945297
SubExprs[SubExpr::Suspend] = nullptr;
52955298
SubExprs[SubExpr::Resume] = nullptr;
52965299
}
52975300

5301+
bool useAwaitSuspendDestroy() const {
5302+
return CoroutineSuspendExprBits.UseAwaitSuspendDestroy;
5303+
}
5304+
5305+
void setUseAwaitSuspendDestroy(bool Use) {
5306+
CoroutineSuspendExprBits.UseAwaitSuspendDestroy = Use;
5307+
}
5308+
52985309
Expr *getCommonExpr() const {
52995310
return static_cast<Expr*>(SubExprs[SubExpr::Common]);
53005311
}

clang/include/clang/AST/Stmt.h

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1258,12 +1258,23 @@ class alignas(void *) Stmt {
12581258

12591259
//===--- C++ Coroutines bitfields classes ---===//
12601260

1261-
class CoawaitExprBitfields {
1262-
friend class CoawaitExpr;
1261+
class CoroutineSuspendExprBitfields {
1262+
friend class CoroutineSuspendExpr;
12631263

12641264
LLVM_PREFERRED_TYPE(ExprBitfields)
12651265
unsigned : NumExprBits;
12661266

1267+
LLVM_PREFERRED_TYPE(bool)
1268+
unsigned UseAwaitSuspendDestroy : 1;
1269+
};
1270+
enum { NumCoroutineSuspendExprBits = NumExprBits + 1 };
1271+
1272+
class CoawaitExprBitfields {
1273+
friend class CoawaitExpr;
1274+
1275+
LLVM_PREFERRED_TYPE(CoroutineSuspendExprBitfields)
1276+
unsigned : NumCoroutineSuspendExprBits;
1277+
12671278
LLVM_PREFERRED_TYPE(bool)
12681279
unsigned IsImplicit : 1;
12691280
};
@@ -1388,6 +1399,7 @@ class alignas(void *) Stmt {
13881399
PackIndexingExprBitfields PackIndexingExprBits;
13891400

13901401
// C++ Coroutines expressions
1402+
CoroutineSuspendExprBitfields CoroutineSuspendExprBits;
13911403
CoawaitExprBitfields CoawaitBits;
13921404

13931405
// Obj-C Expressions

clang/include/clang/Basic/Attr.td

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1354,7 +1354,7 @@ def CoroAwaitElidableArgument : InheritableAttr {
13541354

13551355
def CoroAwaitSuspendDestroy: InheritableAttr {
13561356
let Spellings = [Clang<"coro_await_suspend_destroy">];
1357-
let Subjects = SubjectList<[CXXRecord]>;
1357+
let Subjects = SubjectList<[CXXMethod]>;
13581358
let LangOpts = [CPlusPlus];
13591359
let Documentation = [CoroAwaitSuspendDestroyDoc];
13601360
let SimpleHandler = 1;

clang/include/clang/Basic/AttrDocs.td

Lines changed: 66 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -9364,101 +9364,109 @@ Example:
93649364
}
93659365

93669366
def CoroAwaitSuspendDestroyDoc : Documentation {
9367-
let Category = DocCatDecl;
9367+
let Category = DocCatFunction;
93689368
let Content = [{
93699369

9370-
The ``[[clang::coro_await_suspend_destroy]]`` attribute may be applied to a C++
9371-
coroutine awaiter type. When this attribute is present, the awaiter must
9372-
implement ``void await_suspend_destroy(Promise&)``. If ``await_ready()``
9373-
returns ``false`` at a suspension point, ``await_suspend_destroy`` will be
9374-
called directly. The coroutine being suspended will then be immediately
9375-
destroyed.
9370+
The ``[[clang::coro_await_suspend_destroy]]`` attribute applies to an
9371+
``await_suspend(std::coroutine_handle<Promise>)`` member function of a
9372+
coroutine awaiter. When applied, suspensions into the awaiter use an optimized
9373+
call path that bypasses standard suspend intrinsics, and immediately destroys
9374+
the suspending coro.
93769375

9377-
The new behavior is equivalent to this standard code:
9376+
Each annotated ``await_suspend`` member must contain a compatibility stub:
93789377

93799378
.. code-block:: c++
93809379

9381-
void await_suspend_destroy(YourPromise&) { ... }
9382-
void await_suspend(auto handle) {
9380+
[[clang::coro_await_suspend_destroy]]
9381+
void await_suspend(std::coroutine_handle<Promise> handle) {
93839382
await_suspend_destroy(handle.promise());
93849383
handle.destroy();
93859384
}
93869385

9387-
This enables `await_suspend_destroy()` usage in portable awaiters — just add a
9388-
stub ``await_suspend()`` as above. Without ``coro_await_suspend_destroy``
9389-
support, the awaiter will behave nearly identically, with the only difference
9390-
being heap allocation instead of stack allocation for the coroutine frame.
9386+
An awaiter type may provide both annotated and non-annotated overloads of
9387+
`await_suspend()`, as long as each invocation of an annotated overload has a
9388+
corresponding `await_suspend_destroy(Promise&)` overload.
9389+
9390+
Instead of calling the annotated ``await_suspend()``, the coroutine calls
9391+
``await_suspend_destroy(Promise&)`` and immediately destroys the coroutine
9392+
``await_suspend_destroy()`` must return ``void`` (Note: if desired, it
9393+
would be straightforward to also support the "symmetric transfer"
9394+
`std::coroutine_handle` return type.)
9395+
9396+
This optimization improves code speed and size for "short-circuiting"
9397+
coroutines — those that use coroutine syntax **exclusively** for early returns
9398+
and control flow rather than true asynchronous operations.
9399+
9400+
Specifically, a short-circuiting awaiter is one that either proceeds
9401+
immediately (``await_ready()`` returns ``true``, skipping to
9402+
``await_resume()``) or terminates the coroutine execution.
93919403

9392-
This attribute helps optimize short-circuiting coroutines.
9404+
Then, a short-circuiting coroutine is one where **all** the awaiters (including
9405+
``co_await``, ``co_yield``, initial, and final suspend) are short-circuiting.
93939406

9394-
A short-circuiting coroutine is one where every ``co_await`` or ``co_yield``
9395-
either immediately produces a value, or exits the coroutine. In other words,
9396-
they use coroutine syntax to concisely branch out of a synchronous function.
9397-
Here are close analogs in other languages:
9407+
The short-circuiting coroutine concept introduced above has close analogs in
9408+
other languages:
93989409

93999410
- Rust has ``Result<T>`` and a ``?`` operator to unpack it, while
9400-
``folly::result<T>`` is a C++ short-circuiting coroutine, with ``co_await``
9401-
acting just like ``?``.
9411+
``folly::result<T>`` is a C++ short-circuiting coroutine, within which
9412+
``co_await or_unwind(someResult())`` acts just like ``someResult()?``.
94029413

94039414
- Haskell has ``Maybe`` & ``Error`` monads. A short-circuiting ``co_await``
94049415
loosely corresponds to the monadic ``>>=``, whereas a short-circuiting
94059416
``std::optional`` coro would be an exact analog of ``Maybe``.
94069417

9407-
The C++ implementation relies on short-circuiting awaiters. These either
9408-
resume synchronously, or immediately destroy the awaiting coroutine and return
9409-
control to the parent:
9418+
Returning to C++, even non-short-circuiting coroutines, including asynchronous
9419+
ones that suspend, may contain short-circuiting awaiters, and those might still
9420+
see some performance benefit if annotated.
94109421

9411-
.. code-block:: c++
9422+
Marking your ``await_suspend_destroy`` as ``noexcept`` can sometimes further
9423+
improve optimization.
94129424

9413-
T val;
9414-
if (awaiter.await_ready()) {
9415-
val = awaiter.await_resume();
9416-
} else {
9417-
awaiter.await_suspend();
9418-
return /* value representing the "execution short-circuited" outcome */;
9419-
}
9425+
However, if **all** awaiters within a coroutine are short-circuiting, then the
9426+
coro frame **can reliably be allocated on-stack**, making short-circuiting
9427+
coros behave qualitatively more like plain functions -- with better
9428+
optimization & more predictable behavior under memory pressure.
94209429

9421-
Then, a short-ciruiting coroutine is one where all the suspend points are
9422-
either (i) trivial (like ``std::suspend_never``), or (ii) short-circuiting.
9430+
Technical aside: Heap elision becomes reliable because LLVM is allowed to elide
9431+
heap allocations whenever it can prove that the handle doesn't "escape" from
9432+
the coroutine. User code can only access the handle via suspend intrinsics,
9433+
and annotated short-circuiting awaiters simply don't use any.
94239434

9424-
Although the coroutine machinery makes them harder to optimize, logically,
9425-
short-circuiting coroutines are like syntax sugar for regular functions where:
9435+
Note that a short-circuiting coroutine differs in one important way from a
9436+
function that replaced each `co_await awaiter` with explicit control flow:
94269437

9427-
- `co_await` allows expressions to return early.
9428-
9429-
- `unhandled_exception()` lets the coroutine promise type wrap the function
9430-
body in an implicit try-catch. This mandatory exception boundary behavior
9431-
can be desirable in robust, return-value-oriented programs that benefit from
9432-
short-circuiting coroutines. If not, the promise can always re-throw.
9433-
9434-
This attribute improves short-circuiting coroutines in a few ways:
9435-
9436-
- **Avoid heap allocations for coro frames**: Allocating short-circuiting
9437-
coros on the stack makes code more predictable under memory pressure.
9438-
Without this attribute, LLVM cannot elide heap allocation even when all
9439-
awaiters are short-circuiting.
9440-
9441-
- **Performance**: Significantly faster execution and smaller code size.
9438+
.. code-block:: c++
94429439

9443-
- **Build time**: Faster compilation due to less IR being generated.
9440+
T value;
9441+
if (awaiter.await_ready()) {
9442+
value = awaiter.await_resume();
9443+
} else {
9444+
// ... content of `await_suspend_destroy` ...
9445+
return /* early-termination return object */;
9446+
}
94449447

9445-
Marking your ``await_suspend_destroy`` method as ``noexcept`` can sometimes
9446-
further improve optimization.
9448+
That key difference is that `unhandled_exception()` lets the promise type wrap
9449+
the function body in an implicit try-catch. This automatic exception boundary
9450+
behavior can be desirable in robust, return-value-oriented programs that
9451+
benefit from short-circuiting coroutines. If not, the promise can re-throw.
94479452

9448-
Here is a toy example of a portable short-circuiting awaiter:
9453+
Here is an example of a short-circuiting awaiter for a hypothetical
9454+
`std::optional` coroutine:
94499455

94509456
.. code-block:: c++
94519457

94529458
template <typename T>
9453-
struct [[clang::coro_await_suspend_destroy]] optional_awaiter {
9459+
struct optional_awaiter {
94549460
std::optional<T> opt_;
94559461
bool await_ready() const noexcept { return opt_.has_value(); }
94569462
T await_resume() { return std::move(opt_).value(); }
94579463
void await_suspend_destroy(auto& promise) {
9458-
// Assume the return object of the outer coro defaults to "empty".
9464+
// The return object of `promise`'s coro should default to "empty".
9465+
assert(!promise.returned_optional_ptr_->has_value());
94599466
}
9460-
// Fallback for when `coro_await_suspend_destroy` is unavailable.
9467+
[[clang::coro_await_suspend_destroy]]
94619468
void await_suspend(auto handle) {
9469+
// Fallback for when `coro_await_suspend_destroy` is unavailable.
94629470
await_suspend_destroy(handle.promise());
94639471
handle.destroy();
94649472
}

clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12510,6 +12510,9 @@ def err_await_suspend_invalid_return_type : Error<
1251012510
def err_await_suspend_destroy_invalid_return_type : Error<
1251112511
"return type of 'await_suspend_destroy' is required to be 'void' (have %0)"
1251212512
>;
12513+
def err_await_suspend_suspend_destroy_return_type_mismatch : Error<
12514+
"return type of 'await_suspend' (%1) must match return type of 'await_suspend_destroy' (%0)"
12515+
>;
1251312516
def note_await_ready_no_bool_conversion : Note<
1251412517
"return type of 'await_ready' is required to be contextually convertible to 'bool'"
1251512518
>;

clang/lib/CodeGen/CGCoroutine.cpp

Lines changed: 5 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -174,66 +174,6 @@ static bool StmtCanThrow(const Stmt *S) {
174174
return false;
175175
}
176176

177-
// Check if this suspend should be calling `await_suspend_destroy`
178-
static bool useCoroAwaitSuspendDestroy(const CoroutineSuspendExpr &S) {
179-
// This can only be an `await_suspend_destroy` suspend expression if it
180-
// returns void -- `buildCoawaitCalls` in `SemaCoroutine.cpp` asserts this.
181-
// Moreover, when `await_suspend` returns a handle, the outermost method call
182-
// is `.address()` -- making it harder to get the actual class or method.
183-
if (S.getSuspendReturnType() !=
184-
CoroutineSuspendExpr::SuspendReturnType::SuspendVoid) {
185-
return false;
186-
}
187-
188-
// `CGCoroutine.cpp` & `SemaCoroutine.cpp` must agree on whether this suspend
189-
// expression uses `[[clang::coro_await_suspend_destroy]]`.
190-
//
191-
// Any mismatch is a serious bug -- we would either double-free, or fail to
192-
// destroy the promise type. For this reason, we make our decision based on
193-
// the method name, and fatal outside of the happy path -- including on
194-
// failure to find a method name.
195-
//
196-
// As a debug-only check we also try to detect the `AwaiterClass`. This is
197-
// secondary, because detection of the awaiter type can be silently broken by
198-
// small `buildCoawaitCalls` AST changes.
199-
StringRef SuspendMethodName; // Primary
200-
CXXRecordDecl *AwaiterClass = nullptr; // Debug-only, best-effort
201-
if (auto *SuspendCall =
202-
dyn_cast<CallExpr>(S.getSuspendExpr()->IgnoreImplicit())) {
203-
if (auto *SuspendMember = dyn_cast<MemberExpr>(SuspendCall->getCallee())) {
204-
if (auto *BaseExpr = SuspendMember->getBase()) {
205-
// `IgnoreImplicitAsWritten` is critical since `await_suspend...` can be
206-
// invoked on the base of the actual awaiter, and the base need not have
207-
// the attribute. In such cases, the AST will show the true awaiter
208-
// being upcast to the base.
209-
AwaiterClass = BaseExpr->IgnoreImplicitAsWritten()
210-
->getType()
211-
->getAsCXXRecordDecl();
212-
}
213-
if (auto *SuspendMethod =
214-
dyn_cast<CXXMethodDecl>(SuspendMember->getMemberDecl())) {
215-
SuspendMethodName = SuspendMethod->getName();
216-
}
217-
}
218-
}
219-
if (SuspendMethodName == "await_suspend_destroy") {
220-
assert(!AwaiterClass ||
221-
AwaiterClass->hasAttr<CoroAwaitSuspendDestroyAttr>());
222-
return true;
223-
} else if (SuspendMethodName == "await_suspend") {
224-
assert(!AwaiterClass ||
225-
!AwaiterClass->hasAttr<CoroAwaitSuspendDestroyAttr>());
226-
return false;
227-
} else {
228-
llvm::report_fatal_error(
229-
"Wrong method in [[clang::coro_await_suspend_destroy]] check: "
230-
"expected 'await_suspend' or 'await_suspend_destroy', but got '" +
231-
SuspendMethodName + "'");
232-
}
233-
234-
return false;
235-
}
236-
237177
// Emit suspend expression which roughly looks like:
238178
//
239179
// auto && x = CommonExpr();
@@ -391,10 +331,10 @@ static void emitStandardAwaitSuspend(
391331
CGF.EmitBranchThroughCleanup(Coro.CleanupJD);
392332
}
393333

394-
static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Coro,
395-
CoroutineSuspendExpr const &S,
396-
AwaitKind Kind, AggValueSlot aggSlot,
397-
bool ignoreResult, bool forLValue) {
334+
static LValueOrRValue
335+
emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Coro,
336+
CoroutineSuspendExpr const &S, AwaitKind Kind,
337+
AggValueSlot aggSlot, bool ignoreResult, bool forLValue) {
398338
auto *E = S.getCommonExpr();
399339

400340
auto CommonBinder =
@@ -429,7 +369,7 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
429369
CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF);
430370
llvm::Value *Frame = CGF.CurCoro.Data->CoroBegin;
431371

432-
if (useCoroAwaitSuspendDestroy(S)) { // Call `await_suspend_destroy` & cleanup
372+
if (S.useAwaitSuspendDestroy()) { // Call `await_suspend_destroy` & cleanup
433373
emitAwaitSuspendDestroy(CGF, Coro, SuspendWrapper, Awaiter, Frame,
434374
AwaitSuspendCanThrow);
435375
} else { // Normal suspend path -- can actually suspend, uses intrinsics

0 commit comments

Comments
 (0)