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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to