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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Okay, this is a lot of code to review at once. :)

I'm okay with the general structure (SCC's nested in RefSCCs).  I've only skimmed through the code so far, and have added some minor comments inline based on that.


================
Comment at: include/llvm/Analysis/LazyCallGraph.h:473
@@ -367,2 +472,3 @@
 
-    /// Insert an edge from one node in this SCC to another in this SCC.
+    /// Make an exsiting internal ref edge into a call edge.
+    ///
----------------
Nit: "existing"

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:487
@@ +486,3 @@
+
+    /// Make an exsiting internal call edge into a reF edge.
+    ///
----------------
Nit: "existing"

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:499
@@ +498,3 @@
+
+    /// Make an exsiting outgoing ref edge into a call edge.
+    ///
----------------
Nit: "existing"

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:765
@@ -576,3 +764,3 @@
   /// Map of the entry nodes in the graph to their indices in \c EntryEdges.
-  DenseMap<Function *, size_t> EntryIndexMap;
+  DenseMap<Function *, int> EntryIndexMap;
 
----------------
Why not map these to `unsigned`?  Is making the integer type signed a semantic change?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:349
@@ +348,3 @@
+  // postorder sequence and be done with it.
+  if (ConnectedSet.count(&TargetSCC)) {
+    std::stable_partition(SCCs.begin() + SourceIdx,
----------------
Should this be `!ConnectedSet.count(&TargetSCC)`?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:352
@@ +351,3 @@
+                          SCCs.begin() + TargetIdx + 1,
+                          [&](SCC *C) { return !ConnectedSet.count(C); });
+    SourceN.setEdgeKind(TargetN.getFunction(), Edge::Call);
----------------
Here and below in the later `std::stable_partition`, do you need to update `SCCIndices`?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:408
@@ +407,3 @@
+        SCCs.begin() + SourceIdx + 1, SCCs.begin() + TargetIdx + 1,
+        [&ConnectedSet](SCC *C) { return !ConnectedSet.count(C); });
+    TargetIdx = TargetI - SCCs.begin();
----------------
Should this be `return ConnectedSet.count(C);`?  Since I understand you want `{ nodes reachable from Target } Target { nodes not reachable from Target }` ?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:424
@@ +423,3 @@
+  // NB: We merge into the target because all of these functions were already
+  // reachable from the target, meaning any SCC-wide properties deduced about it
+  // other than the set of functions within it will not have changed.
----------------
any SCC-wide properties except `norecurse`?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:456
@@ +455,3 @@
+                                                    Node &TargetN) {
+  assert(SourceN[TargetN].isCall() && "Must start with a ref edge!");
+
----------------
Minor: assert message is wrong.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:713
@@ -226,1 +712,3 @@
+LazyCallGraph::RefSCC::insertIncomingRefEdge(Node &SourceN, Node &TargetN) {
+  assert(G->lookupRefSCC(TargetN) == this && "Target must be in this SCC.");
 
----------------
This may be naive of me, but why can't this do exactly what `switchInternalEdgeToCall` does when it discovers that it needs to merge a set of SCC's into a new SCC (perhaps even share code with some suitable abstraction)?  If it is expensive to keep a postorder of RefSCCs due to lazy generation (since you'll have to prepend onto a vector), can we apply essentially the same algorithm to the reverse postorder (that is always up to date) of RefSCCs?


http://reviews.llvm.org/D16802