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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

I find that the style of suspend block for async and retcon is a little different with switch. So I choose to handle switch style suspend specially. It should be OK since the intuition of the patch is to make `sinkLifetimeStartMarkers` avoid preventing symmetric transfer. And it seems like that only the switch style coroutine (c++ coroutine) has the definition about symmetric transfer.



================
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))
----------------
rjmccall wrote:
> 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.
> Do we ever not emit a block that does this kind of branch/switch?

We always emit a switch in the frontend. But if there are identical destination basic block in the switch instruction, it may be converted into a conditional branch instruction. And if all the destination blocks are the same (happens in the test case), it would be converted to an unconditional branch instruction.

 > What if there's only one suspend point?

>From my point of view, it should be OK.

>  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.

Agreed. However, it seems hard to code the pattern in a one assert statement. So I tried to add codes to check the pattern. The codes look a little bit ugly to me. But I find it isn't easy to make it in a simple way. One alternative was to add function to verify the IR in the beginning of coro split pass.

By the way, there are other side effects for these guard codes. It breaks some tests whose pattern are unusual for IR produced by the frontend. But from the documentation (https://llvm.org/docs/Coroutines.html#llvm-coro-suspend-retcon-intrinsic), I think these tests are legal. After all, LLVM IR are independent language. So if we decide to continue in this way. We need to modify the document about coroutine API to make it consistent.


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

https://reviews.llvm.org/D103593