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

[Brian Gesiak via Phabricator via llvm-commits]

modocache marked 3 inline comments as done.
modocache added a comment.

Thanks for the quick review! I'll send an update in a sec.



================
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:
> 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.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:1575
+  if (Coroutines.empty())
+    llvm_unreachable("experimental pass manager cannot yet handle "
+                     "'llvm.coro.prepare.retcon'");
----------------
wenlei wrote:
> nit: I would say "new pass manager" instead of "experimental pass manager".. it's not that experimental at this moment. 
Oh, good catch! I agree, will do.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:1597
+      // worklist.
+      UR.RCWorklist.insert(&C.getOuterRefSCC());
+      F.addFnAttr(CORO_PRESPLIT_ATTR, PREPARED_FOR_SPLIT);
----------------
wenlei wrote:
> Is it necessary to request RefSCC to be reprocessed? I thought `UR.CWorklist.insert(&C)` should be enough..
Will do! I was trying to do whatever seemed most similar to the legacy pass manager's repeater, I wasn't sure whether that was the outer repeating loop over RefSCCs, or the inner SCC loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71899