[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2001
 
-    if ((Shape.ABI == coro::ABI::Async || Shape.ABI == coro::ABI::Retcon ||
-         Shape.ABI == coro::ABI::RetconOnce) &&
-        !Shape.CoroSuspends.empty()) {
-      // Run the CGSCC pipeline on the newly split functions.
-      // All clones will be in the same RefSCC, so choose a random clone.
-      UR.RCWorklist.insert(CG.lookupRefSCC(CG.get(*Clones[0])));
+    if (!Shape.CoroSuspends.empty()) {
+      // Run the CGSCC pipeline on the original and newly split functions.
----------------
lxfind wrote:
> ychen wrote:
> > aeubanks wrote:
> > > ChuanqiXu wrote:
> > > > I am not familiar with the Shape.ABI other than coro::ABI:switch. But the diff line seems strange, it looks like that condition gets weaker.
> > > I believe that's intentional, and a big part of this patch. We want to re-add the current SCC (and the split SCCs) any time we split an SCC. Before we weren't properly doing that.
> > I got your point. So "// All clones will be in the same RefSCC ...." : this is not accurate I think?
> Note that previously this is done only for Async, Retcon and RetconOnce ABIs, not for the Switch ABI.
> I guess that's accurate for those ABIs? But for Switch ABI this is not true.
> And before we were not adding back the split functions to the pipeline to be properly optimized. Now we are dong that. This should help improve the performance of the post-split functions.
I missed the ABI condition. Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95807