[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind abandoned this revision. lxfind added a comment. Abandoning in favor of D99227 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98638/new/ https://reviews.llvm.org/D98638

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. @bruno Thanks for the review! Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( bruno wrote: > ChuanqiXu wrote: > > lxfind wrote: > > >

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

junparser added a comment. In D98638#2630786 , @lxfind wrote: > Well, I guess another potential solution is to force emitting lifetime > intrinsics for this part of coroutine in the front-end. > Like this: > > diff --git a/clang/lib/CodeGen/CGDecl.cpp

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

bruno added a comment. Hi Xun, great to see more improvements in this area. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( ChuanqiXu wrote: > lxfind wrote: > >

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( lxfind wrote: > ChuanqiXu wrote: > > ChuanqiXu wrote: > > > can we rewrite it into:

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( ChuanqiXu wrote: > ChuanqiXu wrote: > > can we rewrite it into: > > ``` > > else if

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. > Can't we did as inline comments? No, because it would have already been too late. SuspendExpr returns the result of __builtin_coro_resume(awaiter.await_suspend().address()), which is different from the result of awaiter.await_suspend(). We need to be able to control

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( ChuanqiXu wrote: > can we rewrite it into: > ``` > else if (SuspendRet != nullptr

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. In D98638#2630864 , @lxfind wrote: > That's a fair point. I agree that we have no guarantee these are the only two > cases. > It does seem to me that coroutine implementation somewhat relies on proper > lifetime markers so

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. > Then if we want to put the result of the await_suspend in the stack, I think > we can do it under CodeGen part only. It should be easy to judge the return > type of await_suspend and create a call to llvm.coro.forcestack to the return > value of await_suspend. We

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. > Then we need to answer the question: how can we **prove** that the result of > symmetric transfer and %gro are the **only** exceptions from the above rules. > Or how can we know the list of exceptions wouldn't get longer and longer in > the future? > > Then go back to

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. In D98638#2630778 , @lxfind wrote: > What do you think is the fundamental problem, though? It is hard to give a formal description for the problem. Let me try to explain it. What I want to say here is about rules that decide

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. Well, I guess another potential solution is to force emitting lifetime intrinsics for this part of coroutine in the front-end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98638/new/ https://reviews.llvm.org/D98638

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. > Here what I want to say is we **shouldn't** handle all the symmetric > transfer from the above analysis. And we shouldn't change the ASTNodes and > Sema part. We need to solve about the above pattern. It is not easy to give a > solution since user could implement

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. In D98638#2630607 , @lxfind wrote: > In D98638#2628082 , @ChuanqiXu wrote: > >> I am a little confused about the first problem. Would it cause the program >> to crash? (e.g., we access

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added inline comments. Comment at: clang/include/clang/AST/ExprCXX.h:4695 +/// afterwards on the stack. class CoroutineSuspendExpr : public Expr { friend class ASTStmtReader; lxfind wrote: > ChuanqiXu wrote: > > It looks strange for the change of

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. In D98638#2628082 , @ChuanqiXu wrote: > I am a little confused about the first problem. Would it cause the program to > crash? (e.g., we access the fields of coroutine frame after the frame gets > destroyed). Or it just wastes

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added inline comments. Comment at: clang/include/clang/AST/ExprCXX.h:4695 +/// afterwards on the stack. class CoroutineSuspendExpr : public Expr { friend class ASTStmtReader; ChuanqiXu wrote: > It looks strange for the change of `CoroutineSuspendExpr`

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind added a comment. In D98638#2628082 , @ChuanqiXu wrote: > It looks like there are two things this patch wants to do: > > 1. Don't put the temporary generated by symmetric-transfer on the coroutine > frame. > 2. Offer a mechanism to force some

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. It looks like there are two things this patch wants to do: 1. Don't put the temporary generated by symmetric-transfer on the coroutine frame. 2. Offer a mechanism to force some values (it is easy to extend Alloca to Value) to put in the stack instead of the coroutine

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

lxfind created this revision. Herald added subscribers: ChuanqiXu, hoy, modimo, wenlei, hiraditya. lxfind requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert. Herald added projects: clang, LLVM. One of the challenges with the alloca analysis in