[PATCH] D70927: Introduce a CallGraph updater helper class

[Brian Gesiak via Phabricator via llvm-commits]

modocache added a comment.

Thanks for this! I'm depending on this patch in D71899 <https://reviews.llvm.org/D71899>.

I noticed an edge case when using the CallGraphUpdater to introduce coroutine funclets for recursive coroutine functions, specifically when compiling this: https://github.com/lewissbaker/cppcoro/blob/d83ee9352ea2652fbc4f02885f9d19770f5e9608/test/recursive_generator_tests.cpp#L164. I reduced this to a small unit test, and what I think may be a reasonable fix, in D72226 <https://reviews.llvm.org/D72226>. Please feel free to review my patch, merge it into this one, or to address the issue however you like. And please let me know if it's actually just a user error on my part.

I had a question about where `registerOutlinedFunction` belongs, but other than that this LGTM! I'm not an expert, though, so maybe @chandlerc or someone could review this.



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


================
Comment at: llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h:43
+
+  /// After an CG-SCC pass changes a function in ways that affect the call
+  /// graph, this method can be called to update it.
----------------
Just echoing my nit-pick comment on D72025, but I think "CGSCC" is the canonical abbreviation. "CG-SCC" only appears in this diff and D72025.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70927