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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:625
-  if (PTO.Coroutines)
-    FPM.addPass(CoroElidePass());
-
----------------
lxfind wrote:
> aeubanks wrote:
> > we can still keep this here right? since we'll run the function simplification pipeline on the split coroutine
> > also, this should be duplicated in `buildFunctionSimplificationPipeline()`
> We could. It does create a logically strange situation, that CoroElide will run once before CoroSplit.
> How to decide whether CoroElide should go into simplification pipeline or optimization pipeline?
I agree with that it is strange that CoroElide would run once before CoroSplit. But if we only run CoroElide after CoroSplit, we may miss many optimization point. Given that a coroutine A was inlined into a coroutine B, and CoroElide run on B before the CoroSplit for B.
```
define B(...) {
    entry:
         %A.handle = ...; The handle for A
         ;... ; Some operations with A
        call @llvm.coro.suspend(%B.handle, false); which is possible if the implementation of initial_suspend of the promise type of B isn't std::experimental::suspend_always
        ;... Some other operations with A
        call @llvm.subfn.addr(%A.handle, 1) ; means the end for the lifetime of A
}
```
In this example, CoroElide would find that every path from the definition of %A.handle to the exit point of B would pass `llvm.subfn.addr(%A.handle, 1) ` which means the end for the lifetime of `%A.handle` so that CoroElide may find that `%A.handle` isn't escaped in B. So it is possible that A maybe elided in B.

However if we run CoroElide only if B have been split,  we may miss the opportunity to elide A in B. I would give the example if needed.

I agree with that the design of CoroElide seems to be a little strange and need to be refactored likely. But I don't get a clear and feasible solution now.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95807