[PATCH] D96922: [Coroutine] Check indirect uses of alloca when checking lifetime info

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

In D96922#2583920 <https://reviews.llvm.org/D96922#2583920>, @lxfind wrote:

> In D96922#2581141 <https://reviews.llvm.org/D96922#2581141>, @rjmccall wrote:
>
>> Is there any way we can simplify this problem for ourselves by expecting IR to be emitted differently within coroutines?  Because I think the true fundamental problem here is that LLVM's representation of stack allocations is not very good, and we're stuck trying to do an analysis that the representation isn't suited for at all.  There's no real chance that LLVM in general is going to move away from `alloca`, but if we can recognize when we're emitting one of these allocations that needs to overlap `coro.begin` or `coro.end` and just do it with a special intrinsic that we treat differently in lowering, that might substantially reduce the difficulty of the problem.  If necessary, we can even change the rules for inlining into unsplit coroutines.
>
> A special alloca for coroutines will certainly help but it's not sufficient. A special alloca and a set of special rules around data allocation in coroutines can help resolve the escape-before-coro.begin issue (though I haven't seen any new bugs around that).
> There are other problems that need to be solved. I need some more time to think about how to put them all together.
> I think we are doing OK identifying variables that need to be on the heap (a conservative approach will do just that).
> But it gets complex when we also need to be able to identify a set of variables that MUST be put on the stack. These are the variables that are being used after a coroutine is suspended (i.e. coro.suspend switch goes to the default destination), at which point the coroutine frame may have been destroyed.
> In the example I had above, the coroutine return object is allocated and returned in the very end. Since it's the return object, it has to be on the stack. But the compiler isn't powerful enough to tell that for two reasons:
>
> 1. It appears that it lives through coro.suspend, but it really only is used at the suspend path, not any other paths.
> 2. It's constructed by calling a (sret) struct pointer, which suggests that it may capture, further complicating the problem.
>
> I think a proper solution might require introducing a new IR instruction for coroutine suspension, instead of relying on intrinsics. I have similar conclusions in another bug that involves LICM and coroutine.
>
> To make this patch fully correct, we need to first check PI.isEscaped() before relying on lifetime information.
> Then for the return object case to work, we have two short-term solutions:
>
> 1. We can treat sret as nocapture. This isn't 100% accurate. But the only case where C++ can capture a sret pointer is the constructor captures "this". But even that happens, in order for the "this" to really escape the object needs to be used again. So I think we should be safe there.
> 2. We can special handle the return object variable, and make sure it's always on the stack. This should also work, I think.

Introducing new IR instruction and other special rules may be a long term target. And for the issue introduced by get_return_object, it looks better to emit special metadata for the alloca of `get_return_object` in the clang frontend.

> this patch incrementally improves the exisiting code by considering indirect use, so it should be a pure win

Yes, this Patch LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96922/new/

https://reviews.llvm.org/D96922