[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

Wed Jan 1 19:31:48 PST 2020

modocache planned changes to this revision.
modocache marked 2 inline comments as done and an inline comment as not done.
modocache added inline comments.

================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1227
+          MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+          MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+          MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
----------------
junparser wrote:
> modocache wrote:
> > junparser wrote:
> > > Since coro elision depends on other optimization pass(inline and so on)  implicitly,  how can we adjust the pipeline to achieve this.
> > One option would be to use the new pass manager's registration callbacks, like `PassBuilder::registerPipelineStartEPCallback` or `PassBuilder::registerOptimizerLastEPCallback`. These work similarly to the old pass manager's `PassManagerBuilder::addExtension`. That's something that I think would be good to improve in a follow-up patch, but let me know if you'd rather see it in this one.
> yes,  please. It should be done in this patch sets. 
Will do!

================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1228
+          MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+          MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+          MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
----------------
wenlei wrote:
> Manually scheduling the 2nd coro-split passes in the same CGSCC pipeline would make the resume/suspend funclet ineligible for the bulk of CSGSS opts, given the split point is relatively early. The implication would be discouraging the use of coroutine in performance critical path. I would love to see this being addressed before we claim coroutine is ready for new PM.
> 
> As commented in the 2nd patch, we may not need the devirt trick used with legacy PM, instead for new PM, we could try to leverage `CGSCCUpdateResult` without involving artificial indirect call and devirt (or follow how outlining is handled by new PM).
> I would love to see this being addressed before we claim coroutine is ready for new PM.

I apologize, since I didn't intend to make such a claim. In http://lists.llvm.org/pipermail/llvm-dev/2019-December/137835.html I explained that these 6 patches were focused on lowering coroutine intrinsics. My goal was to resolve https://bugs.llvm.org/show_bug.cgi?id=42867, so that C++ coroutines code didn't trigger a fatal error in ISel.

That being said, I'm happy to make changes here. I'll send updates for D71899 and this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71903