[PATCH] D71899: [Coroutines][2/6] New pass manager: coro-split

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:1411
+  // pass would be misattributed to that unrelated function pass.
+  updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR);
+}
----------------
modocache wrote:
> wenlei wrote:
> > Now that we have CallGraphUpdater, can we use `CGUpdater.reanalyzeFunction(N.getFunction())` instead?
> Yeah, I considered that as well. I opted not to because it's just extra steps:
> 
> 1. We grab node's function via `LazyCallGraph::Node::getFunction`, just to have `CallGraphUpdater::reanalyzeFunction` call `LazyCallGraph::get` to lookup the node in the graph, or create one if it's not in the graph. But we know this node in the graph, and we already have a reference to it, so why look it up?
> 2. `CallGraphUpdater::reanalyzeFunction` then looks up the SCC, which we also already have a reference to. As far as I can tell there's no way `CallGraphUpdater::registerOutlinedFunction` above would have split the SCC, so there's no reason to re-lookup an SCC we already have a reference to.
> 3. And finally, the call to `updateCGAndAnalysisManagerForCGSCCPass`.
> 
> I figured, why not just do (3)?
> 
> That being said, I do think in future patches there's room to consolidate the legacy pass and the new pass's interactions with the call graph. My personal preference would be to wait until D70927 is merged to do that, though.
yeah, there's a bit of abstraction overhead for the CallGraphUpdater layer over calling `updateCGAndAnalysisManagerForCGSCCPass`. Technically `CallGraphUpdater::reanalyzeFunction` is not really needed anywhere as we can almost always call `updateCGAndAnalysisManagerForCGSCCPass` directly, but I thought communicating through `CallGraphUpdater` is cleaner. I don't have a strong opinion on this though - @jdoerfert may want to weigh in on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71899