[PATCH] D70927: Introduce a CallGraph updater helper class

[Brian Gesiak via Phabricator via llvm-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

> I mentioned in my last response that you moved the "problematic" logic into LazyCallGraph in your patch (D72226 <https://reviews.llvm.org/D72226>), or did you do something different? Please clarify what you want me to do that is different, or the same, to your patch. I'm happy to move logic into the LazyCallGraph class for example.

Ah, yes, I see the confusion! In D72226 <https://reviews.llvm.org/D72226> I still have the logic in `CallGraphUpdater`, I didn't move it into `LazyCallGraph`. In D72226 <https://reviews.llvm.org/D72226>, I moved the "problematic" logic into new member functions on `CallGraphUpdater`:

  LazyCallGraph::Node &CallGraphUpdater::createNode(LazyCallGraph &LCG,
                                                    Function &Fn) {
    assert(!LCG.lookup(Fn) && "node already exists");
  
    LazyCallGraph::Node &CGNode = LCG.get(Fn);
    LCG.NodeMap[&Fn] = &CGNode;
    CGNode.DFSNumber = CGNode.LowLink = -1;  // <- HERE, I think this implementation detail ought not be exposed to CallGraphUpdater
    CGNode.populate();
    return CGNode;
  }
  
  void CallGraphUpdater::addNodeToSCC(LazyCallGraph &LCG, LazyCallGraph::SCC &SCC,
                                      LazyCallGraph::Node &N) {
    SCC.Nodes.push_back(&N);
    LCG.SCCMap[&N] = &SCC;  // <- HERE, I think this implementation detail ought not be exposed to CallGraphUpdater
  }

So my specific suggestion would be to move the lines marked "HERE" into a member function on `LazyCallGraph`, thus removing the need for `CallGraphUpdater` to be marked as a `friend`.

> EDIT: I moved the logic into `LazyCallGraph::addNewFunctionIntoSCC`. Does that address your concerns? (It also allowed me to remove the updater as a friend)

Yes, that's exactly what I meant! Sorry I wasn't clearer!

Your latest update addressed all of my comments -- thank you! I do wish someone who's authored more of `CallGraph` or `LazyCallGraph` could chime in here, but this at least LGTM. Thanks for this work! It was a tremendous help in my own work to adapt coroutines to the new pass manager.



================
Comment at: llvm/include/llvm/Transforms/Utils/CallGraphUpdater.h:31
+  /// `finalize` is called. This can happen explicitly or as part of the
+  /// destructor. Dead functions in comdat sections are tracked seperatly
+  /// because a function with discardable linakage in a COMDAT should only
----------------
Nit-pick typo: `s/seperatly/separately/g`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70927