[coro] pre-split handling of the 'suspend' edge

[ChuanqiXu]

@mtrofin

if @llvm.coro.suspend() implied @llvm.coro.end(), that’d be fine, correct?

I think we need a new struct/intrinsic to refer %coro.end to avoid it gets optimized out. Then, it may be fine if we don’t worrying about the refactoring.

[jyknight]

I don’t understand: why do we need %coro.end to not be optimized out? What is the reason the block must preserved, even if nothing reaches it?

[ChuanqiXu]

Otherwise we’ll have a @lllvm.coro.begin() but not a @llvm.coro.end() explicitly… I feel bad about it… and if we want to do something in the ending of the coroutines someday, it may matter.

[mtrofin]

(for my understanding) llvm.coro.begin produces the handle - so logically it pairs with llvm.coro.free rather than llvm.coro.end?

[ChuanqiXu]

No, llvm.coro.free pairs with llvm.coro.alloc. llvm.coro.begin() marks the begin of the coroutine while the llvm.coro.end() marks the end of the coroutine.

[jyknight]

It makes sense to me to have a begin but no end, if the end is in fact unreachable, because the coroutine always aborts before reaching the end.

Are there concrete problems this is expected to cause, or are you only saying it feels weird?

[ChuanqiXu]

because the coroutine always aborts before reaching the end.

This is not true. The coro.ends are always reached. They are just lowered to non-op after compilations.

Are there concrete problems this is expected to cause

Currently, no. But it requires more work to refactor the existing code than adding a new explicit destination BB in an instruction personally.

[mtrofin]

@ChuanqiXu

Are there concrete problems this is expected to cause

Currently, no. But it requires more work to refactor the existing code than adding a new explicit destination BB in an instruction personally.

IIUC the callbr alternative with @llvm.coro.suspend implying @llvm.coro.end seems to address cleanly (meaning, maintainably) the problem the thread started with. Let’s worry about implementation effort separately - if we get general agreement this is the better, more sustainable design (for LLVM in total - coro & other scenarios), I’m happy to help, and I’m sure there are other folks in the community also interested in doing that.

[ChuanqiXu]

Except implementation effort, the only concern is the semantics of callbr. Since currently it is only used to implement goto. See LLVM Language Reference Manual — LLVM 18.0.0git documentation. The semantics of fallthrough label and indirect label sounds not good. But if we are able to extend/refactor that cleanly as @efriedma-quic mentioned. Then yes, no more concerns.

[hansw2000]

@mtrofin pointed out that [coro] Lower `llvm.coro.await.suspend.handle` to resume with tail call by zmodem · Pull Request #89751 · llvm/llvm-project · GitHub is related to this topic

[hansw2000]

I’ve been re-reading this thread since @jyknight asked if the problem has been addressed yet.

Going back to the original post:

@mtrofin

This came up in PR #71263 .

IIUC (I’m still ramping up on coroutines), the following pattern that appears in presplit coroutines:

%t = call i8 @llvm.coro.suspend(...)
switch i8 %t, label %suspend [
   i8 0, label %resume
   i8 1, label %destroy
]

really means that in the “default” (@llvm.coro.suspend “returns” -1) case, we must return immediately. The %suspend BB should have just a call to @llvm.coro.end and return. By “should” I mean that coro-aware passes like coro-split expect that.

The problem is that a naive (coro-unaware) pass or analysis could see the edge to %suspend, or the %suspend basic block, as a regular edge or basic block and do things like split the edge or insert more instructions in the basic block. In the PR example above, the edge could naively be seen as a loop exit edge (but it arguably isn’t. The loop is spread over multiple coro suspends and resumes).

I think the only part of coro-split which assumed that the %suspend block contains only @llvm.coro.end and ret was the code that turned symmetric transfers into tail calls.

That code no longer exists after [coro] Lower `llvm.coro.await.suspend.handle` to resume with tail cal… · llvm/llvm-project@3bb3969 · GitHub

If a pass were to split the edge to the suspend block, or even insert a printf("Coroutine suspending") call, I don’t believe that would be a problem anymore.

@ChuanqiXu pointed out there were other cases where passes incorrectly handled the suspend intrinsic.

I’m guessing those cases were also instances of instructions in the path from @llvm.coro.suspend to ret blocking the tail call transformation. At least from looking at the lit tests, I see that there were a number of such issues. I believe my patch fixed that category of problems altogether.

If I understand the original callbr proposal correctly, that wouldn’t change the control flow of the function — the callbr @llvm.coro.suspend would branch to the same %suspend, %resume, or %destroy blocks as before — it would just be more opaque to other passes. (But tail calls could previously still be broken if unexpected instructions ended up in the %suspend block.)

This seems like the real crux of the thread, and also orthogonal(?) to whether we use a call+switch or callbr for @llvm.coro.suspend:

@jyknight

I don’t understand why there are branches from the suspend points to the return block in the pre-split representation. Shouldn’t a pre-split coroutine be considered temporarily-paused at the llvm.coro.suspend, until it either continues execution (%resume) or gets destroyed (%destroy)? What is the third option of branching to %suspend for?

It seems the point of the %suspend block is to have control flow to the @llvm.coro.end intrinsic, which seems to act like a marker rather than have any effect in itself (in returned-continuation lowering it seems to do more).

I don’t fully understand @llvm.coro.end but it sounds like at least in the switched-resume lowering, maybe we don’t need it if @llvm.coro.suspend is also treated as an implicit @llvm.coro.end.

But since the original problem in the thread was resolved, is there any point in exploring that option further? Do we think it would bring more benefits?