[PATCH] D16802: [LCG] Construct an actual call graph with call-edge SCCs nested inside reference-edge SCCs.

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:452
@@ +451,3 @@
+
+    range slice(SCC &BeginC, SCC &EndC) const {
+      assert(find(BeginC) <= find(EndC) &&
----------------
I'd say lets wait till we have a user?

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:477
@@ -348,1 +476,3 @@
+    bool isChildOf(const RefSCC &C) const {
+      return Parents.count(const_cast<RefSCC *>(&C));
     }
----------------
If having the parents be a container of `const RefSCC *` increases the number of `const_cast`s then what you have here is fine.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:582
@@ -391,3 +581,3 @@
     ///
     /// FIXME: We could possibly optimize this quite a bit for cases where the
     /// caller and callee are very nearby in the graph. See comments in the
----------------
Ah, I misread it as inserting an incoming call edge.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:724
@@ -522,3 +723,3 @@
   /// iterator walk.
   SCC *lookupSCC(Node &N) const { return SCCMap.lookup(&N); }
 
----------------
> Either way, moving completely away from the map seems like it could be usefully separated into a follow-up change.

SGTM.  Might save a few hashtable lookups.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:336
@@ +335,3 @@
+      for (Edge &E : N.calls()) {
+        assert(E.getNode() && "Must have formed a node within an SCC!");
+        if (ConnectedSet.count(G->lookupSCC(*E.getNode())))
----------------
As discussed on IRC, the for loop is fine.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:360
@@ +359,3 @@
+  // post-order and there are no cycles formed.
+  if (!ConnectedSet.count(&TargetSCC)) {
+    assert(SourceI > (SCCs.begin() + SourceIdx) &&
----------------
SGTM

================
Comment at: lib/Analysis/LazyCallGraph.cpp:725
@@ -219,2 +724,3 @@
+#endif
 }
 
----------------
> Is there anything that would make you more comfortable with it? We do this optimization in two places so I'd like to get it right.

Might be helpful to explicitly document that this is a performance optimization then -- I couldn't easily tell if there's something fundamentally different going on here.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:840
@@ +839,3 @@
+                 "Cannot have a null node within a visited SCC!");
+          RefSCC &ChildRC = *G->lookupRefSCC(*E.getNode());
+          if (ConnectedSet.count(&ChildRC))
----------------
SGTM

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1010
@@ +1009,3 @@
+    do {
+      Node *N;
+      edge_iterator I;
----------------
> we'd still visit exactly the same number of edges.

Wouldn't you be able to skip pushing intra-SCC edges, if you're considering an SCC as a node?

> we get a significantly simpler edge iterator model.

This I agree with: for the code to be readable, we'll need to add an `outgoing_edges` iterator to `SCC`, that skips intra-SCC edges.


http://reviews.llvm.org/D16802