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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
