[PATCH] D66230: [coroutine] Fixes "cannot move instruction since its users are not dominated by CoroBegin" problem.

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In D66230#1633348 <https://reviews.llvm.org/D66230#1633348>, @GorNishanov wrote:

> In D66230#1632625 <https://reviews.llvm.org/D66230#1632625>, @rjmccall wrote:
>
> > aspects of frame layout being exposed in the ABI is a property of the switch lowering, not coroutines in general.
>
>
> You may be right. Given that now we have two frontends targeting LLVM Coroutines, some refactoring may be in order. I need to study more the Swift approach before I can form an opinion.
>
> >> Marking it as NoDuplicate in CoroEarly helps simplify the CoroSplit logic.
> > 
> > Does it?  Why?  There already has to be a single `coro.id` in the coroutine, and that's the intrinsic that takes useful information.  `coro.begin` just has a bunch of requirements and no clear purpose except to return the frame, which could just as well be done with a duplicable intrinsic.
>
> Here is how it looks to me: C++ frontends emits the following structure (simplified):
>
>   %id = coro.id(stuff)
>   %mem = SomeAllocCodeCouldBeAnything(stuff)
>   %frame = coro.begin(%mem)
>
>
> coro.begin "blesses" the memory as the one to be used in the coroutine and therefore should dominate any possible uses of data that go into the coroutine frame and gives a convenient place to dump spills, copies, etc.
>
> If we did not allow arbitrary allocation logic in c++, there would be no need for coro.begin at all.


Ah, okay, I see.  Yes, if there has to be inlined code to do the allocation, then something like `coro.begin` does seem necessary; although of course the danger is that that arbitrary code — if nothing else, after inlining — might do something that we naively think needs the coroutine frame.  For example, if the allocation called a user-defined allocation function, and we inlined that call before splitting the coroutine, and the inlined code contained an `alloca` (scoped to the call, of course, but we haven't taught CoroFrame to optimize based on `alloca` lifetimes yet), that would presumably cause serious problems for lowering because the alloca would have uses prior to `coro.begin`.

How arbitrary can allocation really be?  If it can be emitted in a separate function call that can be provided abstractly to `coro.id`, then coroutine lowering can just insert a call to that function (or whatever more complicated pattern is necessary to enable static elimination of the allocation) and then trigger further optimization/inlining as necessary to optimize that new call.  If we need to handle exceptions out of it then maybe we can make `coro.id` non-`nounwind`.  (We can almost get away with just saying "allocation is the first thing the function does, so it never happens in an interesting EH context.  Unfortunately, there are some features/ABIs that require parameters to be destroyed in the callee, which can mean that everything in the function is in an interesting EH context, even generated code in the prologue.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66230