Skip to content

[Clang] Propagate elide safe context through [[clang::coro_await_elidable_argument]] #108474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,10 @@ Attribute Changes in Clang
(#GH106864)

- Introduced a new attribute ``[[clang::coro_await_elidable]]`` on coroutine return types
to express elideability at call sites where the coroutine is co_awaited as a prvalue.
to express elideability at call sites where the coroutine is invoked under a safe elide context.

- Introduced a new attribute ``[[clang::coro_await_elidable_argument]]`` on function parameters
to propagate safe elide context to arguments if such function is also under a safe elide context.

Improvements to Clang's diagnostics
-----------------------------------
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/Basic/Attr.td
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,14 @@ def CoroAwaitElidable : InheritableAttr {
let SimpleHandler = 1;
}

def CoroAwaitElidableArgument : InheritableAttr {
let Spellings = [Clang<"coro_await_elidable_argument">];
let Subjects = SubjectList<[ParmVar]>;
let LangOpts = [CPlusPlus];
let Documentation = [CoroAwaitElidableArgumentDoc];
let SimpleHandler = 1;
}

// OSObject-based attributes.
def OSConsumed : InheritableParamAttr {
let Spellings = [Clang<"os_consumed">];
Expand Down
83 changes: 73 additions & 10 deletions clang/include/clang/Basic/AttrDocs.td
Original file line number Diff line number Diff line change
Expand Up @@ -8258,15 +8258,23 @@ but do not pass them to the underlying coroutine or pass them by value.
def CoroAwaitElidableDoc : Documentation {
let Category = DocCatDecl;
let Content = [{
The ``[[clang::coro_await_elidable]]`` is a class attribute which can be applied
to a coroutine return type.
The ``[[clang::coro_await_elidable]]`` is a class attribute which can be
applied to a coroutine return type. It provides a hint to the compiler to apply
Heap Allocation Elision more aggressively.

When a coroutine function that returns such a type calls another coroutine function,
the compiler performs heap allocation elision when the call to the coroutine function
is immediately co_awaited as a prvalue. In this case, the coroutine frame for the
callee will be a local variable within the enclosing braces in the caller's stack
frame. And the local variable, like other variables in coroutines, may be collected
into the coroutine frame, which may be allocated on the heap.
When a coroutine function returns such a type, a direct call expression therein
that returns a prvalue of a type attributed ``[[clang::coro_await_elidable]]``
is said to be under a safe elide context if one of the following is true:
Comment on lines +8265 to +8267
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a coroutine function returns such a type, a direct call expression therein that returns a prvalue of a type attributed [[clang::coro_await_elidable]] [...]

If I am reading the code correctly, it is not actually a requirement that the containing / callee coroutine itself is also annotated as coro_await_elidable. Only the called coroutine needs to be annotated as coro_await_elidable.

I.e., we would also apply HALO for something like

  class [[clang::coro_await_elidable]] ElidableTask { ... };
  class NonElidableTask { ... };

  ElidableTask foo();
  NonElidableTask bar() {
    co_await foo(); // foo()'s coroutine frame on this line is elidable
  }

Or did I misread the code?

Assuming I understood code correctly, I would propose something like

A call to a coroutine function returning such a type is said to be safe-to-elide
if one of the following is true:
- it is the immediate right-hand side operand to a co_await expression.
- it is an argument to a [[clang::coro_must_await]] parameter or parameter pack
   of another safe-to-elide function call.

Do note that the safe-to-elide context applies only to the call expression itself,
and the context does not transitively include any of its subexpressions unless
``[[clang::coro_must_await]]`` is used to opt-in to transivitely applying safe-to-elide.

The compiler performs heap allocation elision on call expressions on safe-to-elide
calls, if the callee is a coroutine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading the code correctly, it is not actually a requirement that the containing / callee coroutine itself is also annotated as coro_await_elidable.

It is required. See CurFnAwaitElidable. Your example call to foo() of NonElidableTask won't have any elisions because we don't control NonElidableTask::Promise::await_transform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I misread the code.

In general: Is checking if the callee coroutine is annotated actually necessary? To my understanding from the other thread, so far it was necessary because of await_transform and operator co_await. After we make this checking more fine-granular, are there any other reasons why we would still need to check if the callee coroutine is annotated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is checking if the callee coroutine is annotated actually necessary?

Depends on the new spec which I haven't thought about yet but most likely I'll say it's still necessary for simplicity. We can make await_transform and operator co_await propagate the safe elide context. Just having the awaiter type attributed, and asserting that a prvalue of the awaiter returned from operator co_await must satisfy the requirements may do the work. I haven't thought it through.

- it is the immediate right-hand side operand to a co_await expression.
- it is an argument to a ``[[clang::coro_await_elidable_argument]]`` parameter
or parameter pack of another direct call expression under a safe elide context.

Do note that the safe elide context applies only to the call expression itself,
and the context does not transitively include any of its subexpressions unless
exceptional rules of ``[[clang::coro_await_elidable_argument]]`` apply.

The compiler performs heap allocation elision on call expressions under a safe
elide context, if the callee is a coroutine.

Example:

Expand All @@ -8281,8 +8289,63 @@ Example:
co_await t;
}

The behavior is undefined if the caller coroutine is destroyed earlier than the
callee coroutine.
Such elision replaces the heap allocated activation frame of the callee coroutine
with a local variable within the enclosing braces in the caller's stack frame.
The local variable, like other variables in coroutines, may be collected into the
coroutine frame, which may be allocated on the heap. The behavior is undefined
if the caller coroutine is destroyed earlier than the callee coroutine.

}];
}

def CoroAwaitElidableArgumentDoc : Documentation {
let Category = DocCatDecl;
let Content = [{

The ``[[clang::coro_await_elidable_argument]]`` is a function parameter attribute.
It works in conjunction with ``[[clang::coro_await_elidable]]`` to propagate a
safe elide context to a parameter or parameter pack if the function is called
under a safe elide context.

This is sometimes necessary on utility functions used to compose or modify the
behavior of a callee coroutine.

Example:

.. code-block:: c++

template <typename T>
class [[clang::coro_await_elidable]] Task { ... };

template <typename... T>
class [[clang::coro_await_elidable]] WhenAll { ... };

// `when_all` is a utility function that composes coroutines. It does not
// need to be a coroutine to propagate.
template <typename... T>
WhenAll<T...> when_all([[clang::coro_await_elidable_argument]] Task<T> tasks...);

Task<int> foo();
Task<int> bar();
Task<void> example1() {
// `when_all``, `foo``, and `bar` are all elide safe because `when_all` is
// under a safe elide context and, thanks to the [[clang::coro_await_elidable_argument]]
// attribute, such context is propagated to foo and bar.
co_await when_all(foo(), bar());
}

Task<void> example2() {
// `when_all` and `bar` are elide safe. `foo` is not elide safe.
auto f = foo();
co_await when_all(f, bar());
}


Task<void> example3() {
// None of the calls are elide safe.
auto t = when_all(foo(), bar());
co_await t;
}

}];
}
Expand Down
40 changes: 27 additions & 13 deletions clang/lib/Sema/SemaCoroutine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -849,12 +849,28 @@ static bool isAttributedCoroAwaitElidable(const QualType &QT) {
return Record && Record->hasAttr<CoroAwaitElidableAttr>();
}

static bool isCoroAwaitElidableCall(Expr *Operand) {
if (!Operand->isPRValue()) {
return false;
}
static void applySafeElideContext(Expr *Operand) {
auto *Call = dyn_cast<CallExpr>(Operand->IgnoreImplicit());
if (!Call || !Call->isPRValue())
return;

if (!isAttributedCoroAwaitElidable(Call->getType()))
return;

Call->setCoroElideSafe();

return isAttributedCoroAwaitElidable(Operand->getType());
// Check parameter
auto *Fn = llvm::dyn_cast_if_present<FunctionDecl>(Call->getCalleeDecl());
if (!Fn)
return;

size_t ParmIdx = 0;
for (ParmVarDecl *PD : Fn->parameters()) {
if (PD->hasAttr<CoroAwaitElidableArgumentAttr>())
applySafeElideContext(Call->getArg(ParmIdx));

ParmIdx++;
}
}

// Attempts to resolve and build a CoawaitExpr from "raw" inputs, bailing out to
Expand All @@ -880,14 +896,12 @@ ExprResult Sema::BuildUnresolvedCoawaitExpr(SourceLocation Loc, Expr *Operand,
}

auto *RD = Promise->getType()->getAsCXXRecordDecl();
bool AwaitElidable =
isCoroAwaitElidableCall(Operand) &&
isAttributedCoroAwaitElidable(
getCurFunctionDecl(/*AllowLambda=*/true)->getReturnType());

if (AwaitElidable)
if (auto *Call = dyn_cast<CallExpr>(Operand->IgnoreImplicit()))
Call->setCoroElideSafe();

bool CurFnAwaitElidable = isAttributedCoroAwaitElidable(
getCurFunctionDecl(/*AllowLambda=*/true)->getReturnType());
Copy link
Member

@vogelsgesang vogelsgesang Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we actually check if the return type of àwait_transform is marked as elideable, instead of the immediate argument to co_await? Or maybe we should check that both the immediate argument and the transformed awaitable are annotated?

(Not really related to this commit. I just noticed this as I was reviewing this code change here. Fell free to ship this PR without addressing this comment. In case this is actually something we want to change, we should probably do so in a separate PR, anyway...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the piece of code that performs this check. See applySafeElideContext.

But what you said is potentially a good point, we actually skipped checking two things: operator co_await on the callee return object, and the await_transform. As these operations may not be safe. The original intention was to make this a user's liability to ensure early caller destruction cannot happen in both customization points. It's not as granular as I'd like.

With this patch, we are actually making it possible. await_transform and operator co_await here are just function calls whose arguments can be attributed with [[clang::coro_must_await]].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see. So after this patch, we could actually check await_transform and also operator co_await and thereby get rid of the CurFnAwaitElidable check? Such that a ElideableTask coawaited within a NonElidableTask (as discussed in the other thread) would become elideable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So after this patch, we could actually check await_transform and also operator co_await and thereby get rid of the CurFnAwaitElidable check?

No, not yet. I am just saying that we can potentially check the entire expression built here including the CallerType::await_transform and CalleeType::operator co_await part. Right now we are just checking the immediate operand before any transformation customization points.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


if (CurFnAwaitElidable)
applySafeElideContext(Operand);

Expr *Transformed = Operand;
if (lookupMember(*this, "await_transform", RD, Loc)) {
Expand Down
40 changes: 40 additions & 0 deletions clang/test/CodeGenCoroutines/coro-await-elidable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,44 @@ Task<int> nonelidable() {
co_return 1;
}

// CHECK-LABEL: define{{.*}} @_Z8addTasksO4TaskIiES1_{{.*}} {
Task<int> addTasks([[clang::coro_await_elidable_argument]] Task<int> &&t1, Task<int> &&t2) {
int i1 = co_await t1;
int i2 = co_await t2;
co_return i1 + i2;
}

// CHECK-LABEL: define{{.*}} @_Z10returnSamei{{.*}} {
Task<int> returnSame(int i) {
co_return i;
}

// CHECK-LABEL: define{{.*}} @_Z21elidableWithMustAwaitv{{.*}} {
Task<int> elidableWithMustAwait() {
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 2) #[[ELIDE_SAFE]]
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 3){{$}}
co_return co_await addTasks(returnSame(2), returnSame(3));
}

template <typename... Args>
Task<int> sumAll([[clang::coro_await_elidable_argument]] Args && ... tasks);

// CHECK-LABEL: define{{.*}} @_Z16elidableWithPackv{{.*}} {
Task<int> elidableWithPack() {
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 1){{$}}
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 2) #[[ELIDE_SAFE]]
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 3) #[[ELIDE_SAFE]]
auto t = returnSame(1);
co_return co_await sumAll(t, returnSame(2), returnSame(3));
}


// CHECK-LABEL: define{{.*}} @_Z25elidableWithPackRecursivev{{.*}} {
Task<int> elidableWithPackRecursive() {
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 1) #[[ELIDE_SAFE]]
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 2){{$}}
// CHECK: call void @_Z10returnSamei(ptr {{.*}}, i32 noundef 3) #[[ELIDE_SAFE]]
co_return co_await sumAll(addTasks(returnSame(1), returnSame(2)), returnSame(3));
}

// CHECK: attributes #[[ELIDE_SAFE]] = { coro_elide_safe }
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
// CHECK-NEXT: ConsumableSetOnRead (SubjectMatchRule_record)
// CHECK-NEXT: Convergent (SubjectMatchRule_function)
// CHECK-NEXT: CoroAwaitElidable (SubjectMatchRule_record)
// CHECK-NEXT: CoroAwaitElidableArgument (SubjectMatchRule_variable_is_parameter)
// CHECK-NEXT: CoroDisableLifetimeBound (SubjectMatchRule_function)
// CHECK-NEXT: CoroLifetimeBound (SubjectMatchRule_record)
// CHECK-NEXT: CoroOnlyDestroyWhenComplete (SubjectMatchRule_record)
Expand Down
Loading