[PATCH] D70927: Introduce a CallGraph updater helper class

Mon Feb 3 09:47:25 PST 2020

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

Sorry if it's presumptuous of me to request changes, but I guess @hfinkel may agree with me here that `registerOutlinedFunction` ought to be moved to `LazyCallGraph` or one of its inner classes.

================
Comment at: llvm/lib/Transforms/Utils/CallGraphUpdater.cpp:97
+    LazyCallGraph::Node &CGNode = LCG->get(NewFn);
+    CGNode.DFSNumber = CGNode.LowLink = -1;
+    CGNode.populate();
----------------
jdoerfert wrote:
> hfinkel wrote:
> > I agree with @modocache, this looks like an internal implementation detail of LCG. Can you move the logic into that class?
> @modocache Has a patch for this already (D72226) which I somehow missed for a month.
> Options are to merge them or to apply them in order. I don't care which we choose.
My revision D72226 builds on the idea of adding a `CallGraphUpdater` class, adding a specific function I need when outlining. I need something like this in order to make my particular CGSCC pass, coro-split, work with `LazyCallGraph`.

That being said, my (relatively new LLVM contributor) opinion is that `CallGraphUpdater` reaches a bit too deep into the `LazyCallGraph` internals -- it needs to be `friend`s with `LazyCallGraph`, `LazyCallGraph::SCC`, and `LazyCallGraph::Node` in order to accomplish what it's doing here: modifying the `DFSNumber` and `LowLink` members of the node, both of which seem to be like implementation details of the Tarjan DFS algorithm being used under the hood. It seems like @hfinkel agrees with me, that the logic to perform the specific SCC discovery algorithm should be encapsulated entirely within `LazyCallGraph`. And I would not mind modifying my patch D72226 to work with such an approach.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70927