[PATCH] D16038: [LCG] Build an edge abstraction for the LazyCallGraph and use it to differentiate between indirect references to functions an direct calls.

[Philip Reames via llvm-commits]

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

LGTM w/minor comments.  The added abstraction is entirely reasonable.


================
Comment at: include/llvm/Analysis/LazyCallGraph.h:109
@@ +108,3 @@
+
+  /// A class used to represent edges in the call graph.
+  ///
----------------
Having a class called edge which doesn't encode the source seems mildly confusing.  Possibly OutboundEdge?  Decidedly minor and optional.

If you make the change suggested, it might be good to pull enum out as a class enum Edge to keep the use sites descriptive.  Edge::Ref is more meaningful that OutgoingEdge::Ref to me.  Either way, nit, I'd be fine with either.

A bit more detail about the Function/Node and lazy creation might be useful for context on the accessors below.



================
Comment at: include/llvm/Analysis/LazyCallGraph.h:115
@@ +114,3 @@
+  public:
+    // We track both references to functions and actual direct calls as edges in
+    // the graph.
----------------
Minor: doxygen

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:137
@@ +136,3 @@
+    /// This requires that the edge is not null, but will succeed whether we
+    /// have bulit a call graph node for the function yet or not.
+    Function &getFunction() const;
----------------
built

Also, the last half of the comment doesn't parse when reading from the top of the diff?  Is this something which needs more explanation or is it obvious from something else in the code?

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:156
@@ +155,3 @@
+  private:
+    PointerIntPair<PointerUnion<Function *, Node *>, 1, Kind> Value;
+  };
----------------
I'm going to trust you got the double tagging right here.  I'm not bothering to look into the implementation to check.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:259
@@ -196,5 +258,3 @@
 
-      Function *F = I->get<Function *>();
-      Node &ChildN = G->get(*F);
-      *I = &ChildN;
-      return ChildN;
+    // Build the iterator for a specific position in the edge list.
+    call_edge_iterator(EdgeVectorImplT::iterator BaseI,
----------------
This iterator code is a bit confusing to read through - probably just because I'm not familiar with the base class.  

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:263
@@ +262,3 @@
+        : iterator_adaptor_base(BaseI), E(E) {
+      while (I != E && !*I && !I->isCall())
+        ++I;
----------------
Could this be separated into a helper "advance_to_next_call" or something?  Might make the code more obvious.

Also, this looks like this stops on a null edge?  Why?

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:608
@@ +607,3 @@
+inline Function &LazyCallGraph::Edge::getFunction() const {
+  assert(*this && "Queried a null edge!");
+  auto P = Value.getPointer();
----------------
Consistency: tested vs queried four lines apart.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:626
@@ +625,3 @@
+inline LazyCallGraph::Node &LazyCallGraph::Edge::getNode(LazyCallGraph &G) {
+  assert(*this && "Queried a null edge!");
+  auto P = Value.getPointer();
----------------
It looks like this can be:
if (auto *Existing = getNode())
  return Existing;
Node &M = ...

================
Comment at: lib/Analysis/LazyCallGraph.cpp:26
@@ +25,3 @@
+                    LazyCallGraph::Edge::Kind EK) {
+  // Note that we consider *any* function with a definition to be a viable
+  // edge. Even if the function's definition is subject to replacement by
----------------
Why are we excluding edges to declarations?  That seems odd to me.  (Not directly related to this review and should not block.)

================
Comment at: lib/Analysis/LazyCallGraph.cpp:318
@@ +317,3 @@
+      for (Edge &E : *N) {
+        assert(E.getNode() && "Cannot have a null node within a visited SCC!");
+        SCC &ChildC = *G->SCCMap.lookup(E.getNode());
----------------
Hm, time for a getNodeRequired()?


http://reviews.llvm.org/D16038