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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:625
-  if (PTO.Coroutines)
-    FPM.addPass(CoroElidePass());
-
----------------
ChuanqiXu wrote:
> aeubanks wrote:
> > ChuanqiXu wrote:
> > > 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.
> > The simplification pipeline is anything that simplifies/canonicalizes IR. Passes in the optimization may add complexity. If CoroElide mostly ends up cleaning things, it should definitely be in the simplification pipeline.
> > 
> > I'm not super familiar with the coroutine passes, but given that we now unconditionally re-add the current SCC to the queue, CoroElide should run before and after CoroSplit on any given function that is split. If that's good enough then I'd say keep this here.
> CoroElide pass would try to transform heap allocation to stack allocation, which is a major optimization in Coroutine Passes. To my understanding, it isn't equal to simplification. I am not pretty sure about this.
> 
> I prefer to run CoroElide before and after CoroSplit for now even it is strange.
Your description of the pass sounds like simplification/canonicalization to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95807