[PATCH] D70927: Introduce a CallGraph updater helper class

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h:26
+/// simplifies the interface and the call sites, e.g., new and old pass manager
+/// passes can share the same code.
+class CallGraphUpdater {
----------------
modocache wrote:
> Because it's a `friend` of `LazyCallGraph` and so can modify its internal state, I'd say `CallGraphUpdater` is a lot more than just a wrapper to unify the two interfaces. I rely on CallGraphUpdater in my patch D71899 because it allows me to insert an outlined function node into the call graph via `CallGraphUpdater:: registerOutlinedFunction`, defined below.
> 
> This makes me wonder whether the functionality implemented in the body of `CallGraphUpdater::registerOutlinedFunction` should actually be moved, to a public member function on `LazyCallGraph` or one of its other helper classes. The body of that function reaches into internal mappings in `LazyCallGraph` and modifies them -- that seems like something only `LazyCallGraph` itself should be responsible for.
> 
> The other member functions of `CallGraphUpdater` only use `CallGraph` and `LazyCallGraph` API that are public, so those do seem like "wrappers" to me. But `CallGraphUpdater::registerOutlinedFunction` seems like something different. If that function is going to remain, I'd suggest updating this docblock to indicate that `CallGraphUpdater` provides an interface that isn't available on `LazyCallGraph` itself.
Resolved by making it part of the `LazyCallGraph` API.


================
Comment at: llvm/lib/Transforms/Utils/CallGraphUpdater.cpp:97
+    LazyCallGraph::Node &CGNode = LCG->get(NewFn);
+    CGNode.DFSNumber = CGNode.LowLink = -1;
+    CGNode.populate();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70927