[PATCH] D24219: [LCG] Redesign the lazy post-order iteration mechanism for the LazyCallGraph to support repeated, stable iterations, even in the face of graph updates.

[Chandler Carruth via llvm-commits]

chandlerc marked 2 inline comments as done.
chandlerc added a comment.

Thanks so much for the review! See responses below.


================
Comment at: lib/Analysis/LazyCallGraph.cpp:804
@@ -803,2 +803,3 @@
 LazyCallGraph::RefSCC::insertIncomingRefEdge(Node &SourceN, Node &TargetN) {
   assert(G->lookupRefSCC(TargetN) == this && "Target must be in this SCC.");
+  RefSCC &SourceC = *G->lookupRefSCC(SourceN);
----------------
eraman wrote:
> Nit: RefSCC in place of SCC in assert messages here and below.
Great catch! Thanks, and fixed. I think I have several more of these that I'm gonna do a pass over this entire code to clean up afterward...

================
Comment at: lib/Analysis/LazyCallGraph.cpp:830
@@ +829,3 @@
+        for (Node &N : C)
+          for (Edge &E : N.calls()) {
+            assert(E.getNode() && "Must have formed a node within an SCC!");
----------------
eraman wrote:
> Why are ref edges excluded here?
Yikes!

Because this code is based originally on the code for call edges, and I didn't update this.

I've updated the tests to actually check that this works in graphs composed of reference cycles rather than call cycles.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:906
@@ -919,3 +905,3 @@
       for (Node &N : InnerC) {
         G->SCCMap[&N] = &InnerC;
         for (Edge &E : N) {
----------------
eraman wrote:
> The node to SCC mapping doesn't change right?
Correct, this should be a no-op. But I'd prefer to remove it in a separate commit as this was already here and I'm not changing the fact that it isn't needed. =] Good catch though.

Sadly, I can't really test for this as it is in fact a no-op. Even an assert doesn't really seem valuable here.


https://reviews.llvm.org/D24219