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

[Chandler Carruth via llvm-commits]

chandlerc 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) &&
----------------
sanjoy wrote:
> I'd say lets wait till we have a user?
Fine fine. ;] And I was so happy to have figured out a nice pattern here. Will try to remember it.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:725
@@ -219,2 +724,3 @@
+#endif
 }
 
----------------
sanjoy wrote:
> > 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.
I've tried to expand the comments about this in both algorithms, and reference those comments from the place where we do the short-circuit. Let me know if this is helping.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1010
@@ +1009,3 @@
+    do {
+      Node *N;
+      edge_iterator I;
----------------
sanjoy wrote:
> > 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.
I'm really not sure this is the right tradeoff... The iterator itself has to carry 2x the state in order to remember where we "paused" in our walk. But we do get to only have 1 frame in the DFS stack for each SCC.

My expectation is that SCCs with >1 node are *quite* rare in practice, and RefSCCs with >1 SCC are somewhat rare (probably under 50%, maybe under 20% in most code). Given that, I suspect that the state would almost always be a bunch of zeros and we wouldn't save a lot of depth on the stack.

But I think this is still something to potentially revisit as we go along. But I'd like to keep the algorithm as-is for now. It's a complex change and there is already too much of that here.

I'd be interested in re-visiting this and trying to see if there is a way to get the best of both worlds -- simple DFS stack and walk over edges, but handle SCCs at once so that we actually skip redundant work in the face of large SCCs.


http://reviews.llvm.org/D16802