Skip to content

Commit

Permalink
Revert "[C++20] [Coroutines] Mark await_suspend as noinline if the aw…
Browse files Browse the repository at this point in the history
…aiter is not empty"

This reverts commit 9d9c25f.
This reverts commit 19ab266.
This reverts commit c467245.

As the issue #65018 shows,
the previous fix introduce a regression actually. So this commit reverts
the fix by our policies.
  • Loading branch information
ChuanqiXu9 committed Aug 28, 2023
1 parent beeb37a commit 572cc8d
Show file tree
Hide file tree
Showing 6 changed files with 0 additions and 382 deletions.
7 changes: 0 additions & 7 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,6 @@ Bug Fixes in This Version
``abs`` builtins.
(`#45129 <https://github.com/llvm/llvm-project/issues/45129>`_,
`#45794 <https://github.com/llvm/llvm-project/issues/45794>`_)
- Fixed an issue where accesses to the local variables of a coroutine during
``await_suspend`` could be misoptimized, including accesses to the awaiter
object itself.
(`#56301 <https://github.com/llvm/llvm-project/issues/56301>`_)
The current solution may bring performance regressions if the awaiters have
non-static data members. See
`#64945 <https://github.com/llvm/llvm-project/issues/64945>`_ for details.
- Clang now prints unnamed members in diagnostic messages instead of giving an
empty ''. Fixes
(`#63759 <https://github.com/llvm/llvm-project/issues/63759>`_)
Expand Down
28 changes: 0 additions & 28 deletions clang/lib/CodeGen/CGCall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5527,34 +5527,6 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::AlwaysInline);
}

// The await_suspend call performed by co_await is essentially asynchronous
// to the execution of the coroutine. Inlining it normally into an unsplit
// coroutine can cause miscompilation because the coroutine CFG misrepresents
// the true control flow of the program: things that happen in the
// await_suspend are not guaranteed to happen prior to the resumption of the
// coroutine, and things that happen after the resumption of the coroutine
// (including its exit and the potential deallocation of the coroutine frame)
// are not guaranteed to happen only after the end of await_suspend.
//
// The short-term solution to this problem is to mark the call as uninlinable.
// But we don't want to do this if the call is known to be trivial, which is
// very common.
//
// The long-term solution may introduce patterns like:
//
// 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.
//
// If the `await_suspend()` function is marked as `always_inline` explicitly,
// we should give the user the right to control the codegen.
if (inSuspendBlock() && mayCoroHandleEscape() &&
!TargetDecl->hasAttr<AlwaysInlineAttr>())
Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);

// Disable inlining inside SEH __try blocks.
if (isSEHTryScope()) {
Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);
Expand Down
33 changes: 0 additions & 33 deletions clang/lib/CodeGen/CGCoroutine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,36 +139,6 @@ static bool memberCallExpressionCanThrow(const Expr *E) {
return true;
}

/// Return true when the coroutine handle may escape from the await-suspend
/// (`awaiter.await_suspend(std::coroutine_handle)` expression).
/// Return false only when the coroutine wouldn't escape in the await-suspend
/// for sure.
///
/// While it is always safe to return true, return falses can bring better
/// performances.
///
/// See https://github.com/llvm/llvm-project/issues/56301 and
/// https://reviews.llvm.org/D157070 for the example and the full discussion.
///
/// FIXME: It will be much better to perform such analysis in the middle end.
/// See the comments in `CodeGenFunction::EmitCall` for example.
static bool MayCoroHandleEscape(CoroutineSuspendExpr const &S) {
CXXRecordDecl *Awaiter =
S.getCommonExpr()->getType().getNonReferenceType()->getAsCXXRecordDecl();

// Return true conservatively if the awaiter type is not a record type.
if (!Awaiter)
return true;

// In case the awaiter type is empty, the suspend wouldn't leak the coroutine
// handle.
//
// TODO: We can improve this by looking into the implementation of
// await-suspend and see if the coroutine handle is passed to foreign
// functions.
return !Awaiter->field_empty();
}

// Emit suspend expression which roughly looks like:
//
// auto && x = CommonExpr();
Expand Down Expand Up @@ -229,11 +199,8 @@ static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});

CGF.CurCoro.InSuspendBlock = true;
CGF.CurCoro.MayCoroHandleEscape = MayCoroHandleEscape(S);
auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
CGF.CurCoro.InSuspendBlock = false;
CGF.CurCoro.MayCoroHandleEscape = false;

if (SuspendRet != nullptr && SuspendRet->getType()->isIntegerTy(1)) {
// Veto suspension if requested by bool returning await_suspend.
BasicBlock *RealSuspendBlock =
Expand Down
5 changes: 0 additions & 5 deletions clang/lib/CodeGen/CodeGenFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,6 @@ class CodeGenFunction : public CodeGenTypeCache {
struct CGCoroInfo {
std::unique_ptr<CGCoroData> Data;
bool InSuspendBlock = false;
bool MayCoroHandleEscape = false;
CGCoroInfo();
~CGCoroInfo();
};
Expand All @@ -348,10 +347,6 @@ class CodeGenFunction : public CodeGenTypeCache {
return isCoroutine() && CurCoro.InSuspendBlock;
}

bool mayCoroHandleEscape() const {
return isCoroutine() && CurCoro.MayCoroHandleEscape;
}

/// CurGD - The GlobalDecl for the current function being compiled.
GlobalDecl CurGD;

Expand Down
224 changes: 0 additions & 224 deletions clang/test/CodeGenCoroutines/coro-awaiter-noinline-suspend.cpp

This file was deleted.

0 comments on commit 572cc8d

Please sign in to comment.