-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we actually check if the return type of (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...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not the piece of code that performs this check. See But what you said is potentially a good point, we actually skipped checking two things: With this patch, we are actually making it possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, I see. So after this patch, we could actually check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, not yet. I am just saying that we can potentially check the entire expression built here including the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) { | ||
|
There was a problem hiding this comment.
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
. Only the called coroutine needs to be annotated ascoro_await_elidable
.I.e., we would also apply HALO for something like
Or did I misread the code?
Assuming I understood code correctly, I would propose something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required. See
CurFnAwaitElidable
. Your example call tofoo()
ofNonElidableTask
won't have any elisions because we don't controlNonElidableTask::Promise::await_transform
.There was a problem hiding this comment.
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
andoperator 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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andoperator 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.