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

[Xun Li via Phabricator via cfe-commits]

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 point it doesn't yet know if this function is a coroutine.
I could turn ShouldEmitLifetimeMarkers to non-const, and then modify it once it realizes it's a coroutine though, if that's better than the current approach.

> Sorry, I re-read this after posting, and it's not exactly clear what I was saying.  There are a lot of situations where Clang doesn't emit lifetime intrinsics for every `alloca` it emits, or emits unnecessarily weak bounds.  Certain LLVM transforms can also introduce `alloca`s that don't have corresponding lifetime intrinsics.  So I think it's problematic to consider it a correctness condition that we're emitting optimally-tight lifetimes.

I tend to agree. Relying on lifetime for correctness seems fragile.
I wonder if there is a better way to inform optimizer that a "variable" is really a temporary value that should die at the end of an expression?
For instance, whenever we do something simple like:

  foo().bar();
  co_await ...

If we compile it under -O0 without lifetime intrinsics, the return value of `foo()` will always be put on the coroutine frame, unless the compiler knows in advance that `bar()` does not capture.
This becomes a problem if this code appears at a location where the current coroutine frame may be destroyed (but the code itself isn't wrong, it simply doesn't access the frame).
The case for symmetric transfer is exactly this situation.

An alternative to solve the problem for the case of symmetric transfer, is to change the design of symmetric transfer. For example, if we let `await_suspend` to return `void*` instead of `coroutine_handle`, we won't have this problem in the first place, because we no longer need to call `address()`. Maybe @lewissbaker can comment on the viability of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99227