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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Thanks so much for the review Sanjoy.

Now with much better testing (still checking on the last bit of coverage) and many, many bugs fixed. I've also replied inline to some of your questions and fixed all the issues you pointed out (that I could).


================
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;
 
----------------
sanjoy wrote:
> Why not map these to `unsigned`?  Is making the integer type signed a semantic change?
Because I don't want 2^32 modular arithmetic behavior.

I use signed integers unless I *need* an unsigned integer. It makes me much more comfortable writing relational comparisons, etc.

================
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,
----------------
sanjoy wrote:
> Should this be `!ConnectedSet.count(&TargetSCC)`?
Yep.

New unit tests catch this as well.

================
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);
----------------
sanjoy wrote:
> Here and below in the later `std::stable_partition`, do you need to update `SCCIndices`?
Yep. Unit tests also catch this now.

I've also been able to merge some of the calls to this to simplify things.

================
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();
----------------
sanjoy wrote:
> Should this be `return ConnectedSet.count(C);`?  Since I understand you want `{ nodes reachable from Target } Target { nodes not reachable from Target }` ?
Yep. Again, unit tests now catch this and fixed.

================
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.
----------------
sanjoy wrote:
> any SCC-wide properties except `norecurse`?
readnone? most of them are...

================
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.");
 
----------------
sanjoy wrote:
> 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?
So, I've not thought of a good way to retain the postorder of RefSCCs and use them here. It's made tricky because one of the goals of RefSCCS is for updates to one RefSCC to not impact other ones (for parallelism etc) and mutating a postorder list would likely do just that.

But it is a delightful optimization so I'm going to keep thinking about this. Maybe something will present itself once we have the users in hand and know what their usage looks like?


http://reviews.llvm.org/D16802