[PATCH] D37093: [coroutines] Promote cleanup.dest.slot-like allocas to registers to avoid storing them in the coroutine frame

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D37093#918717, @GorNishanov wrote:

> In https://reviews.llvm.org/D37093#910964, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D37093#910951, @rjmccall wrote:
> >
> > > In https://reviews.llvm.org/D37093#910909, @GorNishanov wrote:
> > >
> > > > @rjmccall suggested to do the cleanup dest slot elimination wholesale for all functions in clang. (Not specific for coroutines).
> > > >  That is a sweeping change :-), so, I don't mind still having this here to unblock Facebook and when the clang change lands, remove this part from llvm.
> > >
> > >
> > > Well, I said that I wouldn't be opposed to cleaning up the cleanup dest slot unconditionally.  I expressed no opinion about whether that was a reasonable way to fix this problem in your representation.  (This doesn't affect my lowering as coro.end is always a terminator.)
> >
> >
> > I think you probably do need to solve this problem more generally, as it sounds like any use of an alloca in the post-coro.end section of the function will lead to miscompiles.  It sounds like you have some control of the post-coro.end generated code, so maybe allocas don't normally affect you there — but from the fact that you have uses of the cleanup dest, I assume you're branching through cleanups, which means you have calls, which means you can have arbitrary code there if the callee is marked always_inline.
>
>
> coro-early pass is run as early pass during "cleanup after a frontend" phase of the optimizer. Thus, arbitrarly inlined code cannot happen.


Hmm.  After digging through source code for a while, it looks like this does precede the always-inliner, chiefly by virtue of being added to the function pass manager which seems to be run before the module passes.  Still, this really feels like a hack that's going to come back to bite us.  What if there's some weird control-flow feature elsewhere in the function that prevents mem2reg from working completely?  I assume the direct bug you're trying to fix is that the frame is destroyed before your last use of the cleanup-dest alloca; is there a strong argument why this problem is specific to just the cleanup-dest alloca?  Are we going to end up tightly integrating coroutine emission (which, with all due respect, is going to be very lightly tested in terms of the language features it interacts with) with of the cleanup and cleanup-emission logic across IRGen?  I'm sorry if some of these points have already been hashed out elsewhere; if you want to just refer me to that discussion, please do.

If this really is special to cleanup dests, and we have a compelling argument that mem2reg will always succeed, then I think we can accept this patch.  The other option is to find an IR-generaton pattern that avoids needing to access the cleanup-dest alloca after the destruction of the frame at all.


https://reviews.llvm.org/D37093