[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc7a39c833af1: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines (authored by lxfind). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99227/new/ https://reviews.llvm.org/D99227 _

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

lxfind updated this revision to Diff 38. lxfind added a comment. Address comments, and fix all tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99227/new/ https://reviews.llvm.org/D99227 Files: clang/lib/CodeGen/CGCoroutine.cpp clang/li

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

lxfind added a comment. In D99227#2646819 , @rjmccall wrote: > Is it feasible to outline the initial segment that you don't want to be part > of the coroutine, and then have coroutine splitting force that outlined > function to be inlined into the ramp f

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. In D99227#2646710 , @lxfind wrote: >> I think you just set `ShouldEmitLifetimeMarkers` correctly in the first >> place instead of adding this as an extra condition to every place that >> considers it, however. > > This was set w

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. Is it feasible to outline the initial segment that you don't want to be part of the coroutine, and then have coroutine splitting force that outlined function to be inlined into the ramp function? IIUC, you were saying that the splitting patch was difficult, but maybe

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

ChuanqiXu added a comment. In D99227#2646719 , @lxfind wrote: > In D99227#2646568 , @ChuanqiXu wrote: > >> Only one problem I had for emitting lifetime markers even at O0 is that >> would action make allocas to be

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

lxfind added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1318 /// otherwise llvm::Value *CodeGenFunction::EmitLifetimeStart(uint64_t Size, llvm::Value *Addr) { ChuanqiXu wrote: > Can we sure fronten

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

lxfind added a comment. In D99227#2646568 , @ChuanqiXu wrote: > Only one problem I had for emitting lifetime markers even at O0 is that would > action make allocas to be optimized even at O0? If so, I wonder if it > confuses programmers since they may fi

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

lxfind added a subscriber: lewissbaker. lxfind added a comment. > I think you just set `ShouldEmitLifetimeMarkers` correctly in the first place > instead of adding this as an extra condition to every place that considers > it, however. This was set when a CodeGenFunction is constructed, at that

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

ChuanqiXu added a comment. Only one problem I had for emitting lifetime markers even at O0 is that would action make allocas to be optimized even at O0? If so, I wonder if it confuses programmers since they may find some variables disappear surprisingly. Or there would be no optimization since

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. In D99227#2646532 , @rjmccall wrote: > I am skeptical that it's reasonable to do this for *correctness*, however; I > don't think the frontend unconditionally emits lifetime intrinsics. Sorry, I re-read this after posting, and i

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. I have no objection to trying to always emit lifetime intrinsics in coroutines since it has a less-trivial runtime cost. I am skeptical that it's reasonable to do this for *correctness*, however; I don't think the frontend unconditionally emits lifetime intrinsics. B

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

lxfind created this revision. Herald added subscribers: ChuanqiXu, hoy, modimo, wenlei. lxfind requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. tl;dr Correct implementation of Corouintes requires having lifetime intrinsics available. Corou