[PATCH] D86859: [Coroutine] Make dealing with alloca spills more robust

[Xun Li via Phabricator via llvm-commits]

lxfind added a comment.

@ChuanqiXu Thank you for taking a look!



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:643
+// If %b is an alias of %a and will be used after CoroBegin, this will be broken
+// and there is nothing we can do about it.
 namespace {
----------------
ChuanqiXu wrote:
> Would there be cast* or GEP instruction of `Alloca` before Coro.Begin? If it is, it seems like we need to visit all the use of Alloca in AllocaVisitor
Yes that's exactly what we are doing in the visitor.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1002
   // the spill slot in the coroutine frame.
   if (MightNeedToCopy) {
     Builder.SetInsertPoint(FramePtr->getNextNode());
----------------
ChuanqiXu wrote:
> If there is a case:
> ```
> %a = alloca..
> %b = bitcast  %a to ...
> @coro.begin
> // some use of b but no use of a
> ```
> in this case, all the use of `%a` is dominated by Coro.begin, so the variable `MightNeedToCopy` may not be true, then the codes before won't be executed.
> 
> 
MightNeedToCopy will be true if any use if the AllocaInst is not dominated by coro.begin. So in your example, since %b uses %a and happens before coro.begin, it's not dominated by coro.begin, and hence MightNeedToCopy will be true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86859