[clang] [llvm] [Coroutines] Mark parameter allocas with coro.outside.frame metadata (PR #127653)

[Chuanqi Xu via llvm-commits]

ChuanqiXu9 wrote:

> > Adding metadata to an instruction should never be required for correctness
> 
> I figured the existing use and the fact that it only needs to survive until CoroSplit made it good enough. But you're right, we should do better.
> 
> I do think we need an explicit way to tell CoroSplit whether an alloca should go in the frame or not. I was thinking of adding an intrinsic when I found this metadata.
> 
> How about turning `coro.outside.frame` into an intrinsic instead?

My instinct reaction is, it may block some optimizations to optimize the alloca out. I don't feel very good about it. Is it possible to introduce the intrinsics without blocking optimizations?

> 
> > I don't feel the previous approach is too problematic.
> 
> I was concerned that it relied on semantics which aren't really defined. There is nothing in the langref which says that the coro frame cannot be accessed after coro.end. As far as I can tell, doing so might be fine as long as the frame hasn't been deallocated, which we can't infer statically.

I believe we shouldn't access the coroutine frame after coro.end. If the doc doesn't make it clear, let's try to make it.

It is true that this may be fine with an optimization, but we don't need to care about that.

> 
> In fact, my change broke a test which seems to be doing exactly that. Just because it's old doesn't mean it was wrong. So the patch didn't feel solid.

I do feel it is wrong since it accesses the frame after it ends. It makes no sense.

> 
> On the other hand, the frontend knows exactly what's going on with these parameter allocas: they should be outside the frame, so having a way to communicate that seems like a solid fix.


https://github.com/llvm/llvm-project/pull/127653