[PATCH] D71899: [Coroutines][2/6] New pass manager: coro-split

[JunMa via Phabricator via llvm-commits]

junparser added a comment.

In D71899#1799622 <https://reviews.llvm.org/D71899#1799622>, @wenlei wrote:

> > The  devirt trigger here is to restart CGSCC pipeline to run coro-split again to split the coroutine function. There is no need to introduce devirt trigger in NewPM  since we call coro-split pass manually.
>
> Manually schedule the 2nd coro-split pass is only a workaround before we can trigger CGSCC passes on the split funclet like we did for legacy PM. It does the split without restarting CGSCC passes so it works, but it also leaves performance on the table because the split funclets won't go through many opt passes of CGSCC pipeline. Yes, I agree don't need to introduce devirt trigger with new PM, but that's because I think we can request CGSCC passes on split funclet via other mechanism like `CGSCCUpdateResult`, not just because 2nd coro-split pass is manually scheduled.


There are two issues here: 1) coro-split needs run at least twice, we do not need CGSCC pipeline at pre-split stage which coro-split pass just works as a function pass  2)  request CGSCC passes on split funclet  after 2nd running of coro-split,  and coroutine optimization such as coro-elide pass also depends on these optimization.
Manually schedule coro-split twice is  a workaround for 1),  as for 2) the current pipeline of coroutine in this patch  set need be changed.

> That said, for new PM, this patch implemented devirt trigger insertion only, but not the devirt detection part. I would suggest we have all or nothing for a working mechanism of rerunning CGSCC passes for split funclet. Funclets like resume contains actual code, not just stubs, so IMHO fully optimizing these funclets is an essential part of coroutine support.

I agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71899