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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I'm not sure I'm the right person to review this so do not wait for my OK. I left some drive-by comments below though.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:1411
+  // pass would be misattributed to that unrelated function pass.
+  updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR);
+}
----------------
wenlei wrote:
> 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.
If you are in a new PM only code region, and the LazyCall graph helpers are sufficient, I don't see any reason not to call them directly. The `CallGraphUpdater` comes in handy if you are in generic code that needs to work in both PMs, or if it has abstractions that make your live easier.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:19
-// coroutine.
-//===----------------------------------------------------------------------===//
 
----------------
Drive by: Isn't this accurate anymore? Should we replace it instead of removing it? I like file comments that explain what is going on.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:1345
 
-static void splitCoroutine(Function &F, coro::Shape &Shape,
-                           SmallVectorImpl<Function *> &Clones) {
----------------
Drive-by: Changes to these functions `splitCoroutine` seem NFC to me. If that is the case they can/should be committed beforehand as such (w/o review).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71899