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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to