[PATCH] D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2343
            "should have split coro.suspend into its own block");
-    DomSet.insert(SuspendBlock->getSingleSuccessor());
+    BasicBlock *SwitchBlock = SuspendBlock->getSingleSuccessor();
+    for (auto *BB : llvm::successors(SwitchBlock))
----------------
ChuanqiXu wrote:
> rjmccall wrote:
> > Can we assert that this is a switch block somehow?  This is very dangerous if the code pattern isn't what we expect.
> The terminator of `SuspendBlock->getSingleSuccessor()` isn't necessarily  a `SwitchInst` actually. Precious name is misunderstanding, my bad. I add comments to clarify it.
Do we ever not emit a block that does this kind of branch/switch?  What if there's only one suspend point?  It's extremely important that this not accidentally walk into the user-provided parts of the function, so if there's any way for us to actually assert that the successor block has the form we expect (nothing but a switch or branch on the state value), that would make me feel a lot better.


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

https://reviews.llvm.org/D103593