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

[lxfind]

Summary

Relevant discussion can be found at: https://lists.llvm.org/pipermail/llvm-dev/2021-January/148197.html
In the existing design, An SCC that contains a coroutine will go through the folloing passes:
Inliner -> CoroSplitPass (fake) -> FunctionSimplificationPipeline -> Inliner -> CoroSplitPass (real) -> FunctionSimplificationPipeline

The first CoroSplitPass doesn't do anything other than putting the SCC back to the queue so that the entire pipeline can repeat.
As you can see, we run Inliner twice on the SCC consecutively without doing any real split, which is unnecessary and likely unintended.
What we really wanted is this:
Inliner -> FunctionSimplificationPipeline -> CoroSplitPass -> Inliner -> FunctionSimplificationPipeline

Hence the way we do it here is to move CoroSplitPass to the end of the CGSCC pipeline, make it once for real, and then insert the newly generated SCCs back to the pipeline so that they can be optimized again.
From here we can then reason about whether we actually want to run it a second time (maybe add an option to turn it off if the cost is over benefit).

This approach also conforms to how the new pass manager works instead of relying on an adhoc post split cleanup, making it ready for full switch to new pass manager eventually.

A lot of tests need to be updated because we now no longer clean things up in O0 after CoroSplit.
I will clean up the tests latter but for now this is RFC to see if people have any objects on doing this.

[aeubanks]

llvm/lib/Passes/PassBuilder.cpp

625

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()

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1710

this could regress coroutines under the legacy PM behavior since we don't run postSplitCleanup() here? but maybe we don't care about the legacy PM so much anymore.

2004

are all Clones guaranteed to be in different SCC?

[lxfind]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1710

Not really. CoroSplit for legacy PM works in a similar way. Function simplifications happen after CoroSplit (if not O0) as well.

2004

No. But CWorklist is a map so redundant insertions won't happen.

[rjmccall]

I can't really review the subtle pass-management stuff here, but if you're going to be able to eliminate the hacky post-pass cleanup pipeline, I'm overjoyed.

[lxfind]

llvm/lib/Passes/PassBuilder.cpp

625

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?

[ChuanqiXu]

llvm/lib/Passes/PassBuilder.cpp

625

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.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

2001

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.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1032

Is it necessary to remain the verify process?

[rjmccall]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1032

It's probably okay for it to go away now.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1032

I prefer to remain the verification *personally* since it had reported many errors for me :).

[lxfind]

By the way, is everyone comfortable with dropping the test lines using the legacy pass manager for Coroutine?
It's really painful to update the tests to keep both legacy pass manager and new pass manager, now that the pass sequence becomes much different. (CoroSplit runs late in CGSCC pipeline for NewPM, but early in legacy pm).
If we want to keep the legacy PM tests for a while for coroutine, I could wait until everyone is comfortable before updating this patch again.

[rjmccall]

In D95807#2537105, @lxfind wrote:

By the way, is everyone comfortable with dropping the test lines using the legacy pass manager for Coroutine?
It's really painful to update the tests to keep both legacy pass manager and new pass manager, now that the pass sequence becomes much different. (CoroSplit runs late in CGSCC pipeline for NewPM, but early in legacy pm).
If we want to keep the legacy PM tests for a while for coroutine, I could wait until everyone is comfortable before updating this patch again.

What's most important is to test the pass. If you want to port most of the tests to unconditionally test under the new PM, that's fine with me. We should keep some tests that validate basic full-coroutine-pipeline behavior under the old PM until we actually retire the old PM, though.

[ChuanqiXu]

In D95807#2537105, @lxfind wrote:

By the way, is everyone comfortable with dropping the test lines using the legacy pass manager for Coroutine?
It's really painful to update the tests to keep both legacy pass manager and new pass manager, now that the pass sequence becomes much different. (CoroSplit runs late in CGSCC pipeline for NewPM, but early in legacy pm).
If we want to keep the legacy PM tests for a while for coroutine, I could wait until everyone is comfortable before updating this patch again.

I think testing is more important than the develop experience. It is more suitable to remove the tests for legacy pass manager when the old pass manager got replaced entirely.

[aeubanks]

llvm/lib/Passes/PassBuilder.cpp

625

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.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

2001

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.

2004

Oh I didn't know that, thanks!

[ChuanqiXu]

llvm/lib/Passes/PassBuilder.cpp

625

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.

[aeubanks]

llvm/lib/Passes/PassBuilder.cpp

625

Your description of the pass sounds like simplification/canonicalization to me.

[lxfind]

In D95807#2538160, @ChuanqiXu wrote:

In D95807#2537105, @lxfind wrote:

By the way, is everyone comfortable with dropping the test lines using the legacy pass manager for Coroutine?
It's really painful to update the tests to keep both legacy pass manager and new pass manager, now that the pass sequence becomes much different. (CoroSplit runs late in CGSCC pipeline for NewPM, but early in legacy pm).
If we want to keep the legacy PM tests for a while for coroutine, I could wait until everyone is comfortable before updating this patch again.

I think testing is more important than the develop experience. It is more suitable to remove the tests for legacy pass manager when the old pass manager got replaced entirely.

By the way, since the new PM has been enabled by default, all of the tests are no longer testing Legacy PM behavior anymore (unless explicitly specified --enable-new-pm=0)

[ChuanqiXu]

In D95807#2554905, @lxfind wrote:

In D95807#2538160, @ChuanqiXu wrote:

In D95807#2537105, @lxfind wrote:

By the way, is everyone comfortable with dropping the test lines using the legacy pass manager for Coroutine?
It's really painful to update the tests to keep both legacy pass manager and new pass manager, now that the pass sequence becomes much different. (CoroSplit runs late in CGSCC pipeline for NewPM, but early in legacy pm).
If we want to keep the legacy PM tests for a while for coroutine, I could wait until everyone is comfortable before updating this patch again.

I think testing is more important than the develop experience. It is more suitable to remove the tests for legacy pass manager when the old pass manager got replaced entirely.

By the way, since the new PM has been enabled by default, all of the tests are no longer testing Legacy PM behavior anymore (unless explicitly specified --enable-new-pm=0)

To my understand, this means that it is OK to remain the Legacy PM test lines? Since the legacy PM test line would be ignored?

[rjmccall]

Since we seem to be nearly done with the legacy PM, I think it's acceptable to no longer test it specifically for the coro passes.