In D93497#2514162, @aprantl wrote:

I looked into the duplicate variables. For your first testcase, there is first the function parameter t:

ParmVarDecl 0x1585a3de8 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:10, col:24> col:24 used t 'struct test &'

followed by an implicit VarDecl (thus the DW_AT_artificial attribute) for another t:

VarDecl 0x1585a8c30 </Users/adrian/Downloads/test/coro-debug-2.cpp:30:6> col:6 implicit used __coro_gro 'coro' nrvo cinit
`-CXXMemberCallExpr 0x1585a8b88 <col:6> 'coro'
  `-MemberExpr 0x1585a8b58 <col:6> '<bound member function type>' .get_return_object 0x1585a0990
    `-DeclRefExpr 0x1585a8b38 <col:6> 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type' lvalue Var 0x1585a57a8 '__promise' 'std::experimental::__coroutine_traits_sfinae<coro>::promise_type':'coro::promise_type'

The second one is produced by void EmitGroAlloca() in CGCoroutine.cpp. I'm assuming that this is the declaration of the function parameter as it is being captured for the CoroFrame function argument that is being passed to the various .resume functions.

hi @mivnay, could you please rebase this and D93762 ? So we can test it.
It say that this patch can improve mcf 8%, we are interested at this because we also found that mcf has performance regression caused by ILP data dependency.

Thanks for fix this. Furthermore, we can add runtime check to see whether this pointer is current coroutine frame, and optimize them separately.

In D94388#2499897, @junparser wrote:

In D94388#2494294, @aprantl wrote:

Do we understand *why* the frontend is emitting the variables twice? If that just a bug in the clang frontend (which would be my first guess) then we should aim at fixing the frontend instead. If the frontend is trying to do something different, I would love to understand what that is.

Hi @aprantl,
According to the standard, coroutine needs to copy parameters (calling constructor) passed to the coroutine function by the original caller into the coroutine frame. So in frontend, we fake local variables for this behavior, that why we see two variables in debuginfo.

In D94388#2494294, @aprantl wrote:

Do we understand *why* the frontend is emitting the variables twice? If that just a bug in the clang frontend (which would be my first guess) then we should aim at fixing the frontend instead. If the frontend is trying to do something different, I would love to understand what that is.

In D93497#2488109, @aprantl wrote:

Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

I was hoping that in an optimization pipeline the coroutine splitting would be invoked before mem2reg. This patch is really depending on the semantics of dbg.declare, which guarantees that the address of the variable doesn't change throughout the entire function. With a dbg.value we don't have any such guarantees, since it's just a snapshot at one particular location.

Thanks for the patch! Although I may not the right person to accept the patch. BTW, Do you have any plan to salvage debug info under O2 when most of the dbg.declare are changed to dbg.value?

LGTM

Thanks for review the patch !

Remove the change that do not have testcases

In D93289#2454658, @RKSimon wrote:

LGTM with one minor

address the comment, update the testcase.

@RKSimon Thank for review the patch. Would please take a look at the similar D93289 and D93083. Thanks a lot.

address the comment.

Address the comment, update the testcase. Also fix same issue in foldBitcastSelect.

kindly ping~

hi @lxfind, sorry for the late reply. After D85812, we think it might be better to move both of attribute and intrinsic of coroutine into core library, however such changes are too heavy to do.
The reason I suggest adding the attribute into EnumAttr is that i believe that we may need to use presplit attribute in other places (such as lower debug intrinsic. ) and we do not want to write the string anywhere cross different parts. Add API in Function will reduce the impact, so LGTM, thanks for the patch.

In D92462#2428824, @bruno wrote:

Thanks for following up with this, I did miss to incorporate this suggestion! Out of curiosity, are you using lldb or gdb to print the variable? In lldb I already get values instead of address before this patch, so I wonder if one debugger needs more specific info than the other.

In D85812#2406785, @lxfind wrote:

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

Thanks for the patch！

In D91305#2392970, @lxfind wrote:

In D91305#2392934, @junparser wrote:

I'm not sure whether we should fix this case by case. Maybe we should use memoryssa to handle this.

In general yes I agree with you that we don't want to handle case by case and make this over-complicated. However there are good reasons that this patch is quite necessary.

1. When optimizations are enabled, there are usually pretty accurate lifetime information, and hence we don't usually even need the alloca visitor to track them. So the alloca visitor is crucial when optimizations are turned off and there is no lifetime information. When optimizations are turned off, because there isn't mem2reg, almost all data accesses are done through the patterns of stores followed with loads. What I found is that in such case, when optimizations are turned off, almost all allocas are put on the frame because they are all used in store instructions, making this a bit useless. This patch is a key to make the alloca tracking effective and so that majority of allocas can be moved out of frame. Hence the effect of this patch is quite significant comparing to the effort. It's unlikely that I will add any more logic to track other cases.
2. This is also a continuation of D90990, which is to avoid frame-use-after-destroy. What I found is that, even I fixed the front-end such that the use of temporaries after a call to await_suspend() are contained locally that does not go across suspension points, because they are all involved in this store/load pattern, they are still being put on the frame, causing memory corruptions in non-optimized mode. This patch can fix that issue completely.

I'm not sure whether we should fix this case by case. Maybe we should use memoryssa to handle this.

In D90772#2384353, @bruno wrote:

Hi @junparser,

One of the another issue is that it is also necessary to track coroutine function parameters correctly under O1/O2 level which use dbg.value. Thoughts?

I haven't looked up the coroutine function parameter debugging quality yet, does it even work fine for -O0? I'm trying to go over the issues I find for -O0 first and incrementally improve on top of that. Any specific testcase in mind?

Hi Bruno,
One of the another issue is that it is also necessary to track coroutine function parameters correctly under O1/O2 level which use dbg.value. Thoughts?

LGTM, Thanks!

This patch works for llvm.dbg.addr. So what is status of llvm.dbg.addr? does anyone know about it ?

Thanks for the patch!

Thank you for the explanation!

In D89768#2355576, @junparser wrote:

In D89768#2354119, @lxfind wrote:

In D89768#2353488, @junparser wrote:

Thanks for the patch! I think I generally agree with this patch.
One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.

What do you mean by "leave aliases on frame"?

Just setEscaped for pointer cast used before coro.begin, or can we do something like llvm::PointerMayBeCapturedBefore to check the pointer directly? I'm not sure about this.

In D89768#2354119, @lxfind wrote:

In D89768#2353488, @junparser wrote:

Thanks for the patch! I think I generally agree with this patch.
One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.

What do you mean by "leave aliases on frame"?

Thanks for the patch! I think I generally agree with this patch.
One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.

In D89066#2325358, @lxfind wrote:

In D89066#2324291, @junparser wrote:

In D89066#2324151, @lxfind wrote:

In D89066#2324115, @junparser wrote:

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

Unfortunately I don't. But this is not related to ASAN. Basically, this is causing destructing of objects that should still be alive. I suspect that it's because ExprWithCleanups always clean up temps that belongs to the full expression, not just the sub-expression in it.

In D89066#2324151, @lxfind wrote:

In D89066#2324115, @junparser wrote:

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

why we should not do this with normal await call?

@efriedma Consider that temporary fix maybe not the best solution for such issue. We would try to find other general solution. Before that, I'll keep this patch as plan changes.

In D87817#2294244, @lxfind wrote:

In D87817#2294122, @junparser wrote:

In D87817#2291684, @lxfind wrote:

In D87817#2289365, @junparser wrote:

In D87817#2278816, @junparser wrote:

In D87817#2278717, @junparser wrote:

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

one of the idea is that we do not generate default case for switch instruction of coro.suspend intrinsic. Instead, we find fall through coro.end for coro.suspend directly in corosplit pass. The only uncertain thing is whether fall through coro.end is moved by passes which may change the semantic.

@lxfind, what do you think about this idea?

Are you suggesting to not have an edge from the coro.suspend switch to the coro.end block? Would that make the coro.end BB a dead BB and could potentially get removed?

No, There is always path from entry to coro.end BB.

This is likely the right solution, but we do need to make it more general. In order for this to always work, I wonder if the proper place to insert this transformation is in LoopSimplify, which is a pass always executed before any Loop pass. That might guarantee that any loop pass will be able to handle this properly. I am not too familiar with loop passes, but maybe someone else could comment on that.

Loop exit-block insertion in LoopSimplify pass guarantees that all exit blocks from the loop only have predecessors from inside of the loop. It will break the dedicated form of exit blocks when we consider ignore coro.suspend path. Maybe there is other idea i haven't thought about.

I wasn't referring to ignoring the coro.suspend path, but to move this code in this patch into the LoopSimplify pass, so that all loop passes can benefit, not sure if that makes sense. Anyway, I don't have a better idea.. So don't let that block you here.

LGTM, thanks for the patch!

In D87817#2291684, @lxfind wrote:

In D87817#2289365, @junparser wrote:

In D87817#2278816, @junparser wrote:

In D87817#2278717, @junparser wrote:

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

one of the idea is that we do not generate default case for switch instruction of coro.suspend intrinsic. Instead, we find fall through coro.end for coro.suspend directly in corosplit pass. The only uncertain thing is whether fall through coro.end is moved by passes which may change the semantic.

@lxfind, what do you think about this idea?

Are you suggesting to not have an edge from the coro.suspend switch to the coro.end block? Would that make the coro.end BB a dead BB and could potentially get removed?

No, There is always path from entry to coro.end BB.

Please remove unnecessary headers include and declarations

In D87817#2278816, @junparser wrote:

In D87817#2278717, @junparser wrote:

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

one of the idea is that we do not generate default case for switch instruction of coro.suspend intrinsic. Instead, we find fall through coro.end for coro.suspend directly in corosplit pass. The only uncertain thing is whether fall through coro.end is moved by passes which may change the semantic.

In D87817#2288579, @asbirlea wrote:

Can you please extend the testing to cover at least some of these cases: multiple predecesssors, multiple exit blocks, multiple predecessors with a coroutine switch?

Update the doc. @efriedma is this ok?

sorry for the late reply.

In D87817#2280618, @efriedma wrote:

If there are transformations that are specifically illegal with corouties, I'd like to see a section in https://llvm.org/docs/Coroutines.html describing what transforms are and are not legal. Just saying that the current version of the coroutine lowering code can't handle it isn't sufficient.

LGTM, Thanks for the fix.

In D87817#2278717, @junparser wrote:

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

what does this intrinsic use for?

@lxfind , could you backport this to branch 11?

LGTM， thanks！

Thanks for the change. LGTM, and testcase？

In D86190#2227340, @efriedma wrote:

Are you sure this is the right fix? At first glance, ensuring %p lives long enough seems like the sort of thing coroutine lowering should be able to deal with. If it isn't, at the very least we need documentation describing what optimizations are legal.