[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 added a comment.

Thanks all! Adjustments made, landed.

Easwaran, if you have more comments, don't hesitate to post them and I'll keep track here. I've still got a few things to clean up post-submit anyways.


================
Comment at: lib/Analysis/LazyCallGraph.cpp:852-876
@@ -869,9 +851,27 @@
 
-      // We fully explore the depth-first space, adding nodes to the connected
-      // set only as we pop them off, so "recurse" by rotating to the parent.
-      DFSStack.push_back({C, I});
-      C = &Parent;
-      I = C->parent_begin();
-      E = C->parent_end();
-    }
+  // Use a normal worklist to find which SCCs the target connects to. We still
+  // bound the search based on the range in the postorder list we care about,
+  // but because this is forward connectivity we just "recurse" through the
+  // edges.
+  auto ComputeTargetConnectedSet = [&](SmallPtrSetImpl<RefSCC *> &Set) {
+    Set.insert(this);
+    SmallVector<RefSCC *, 4> Worklist;
+    Worklist.push_back(this);
+    do {
+      RefSCC &RC = *Worklist.pop_back_val();
+      for (SCC &C : RC)
+        for (Node &N : C)
+          for (Edge &E : N) {
+            assert(E.getNode() && "Must have formed a node!");
+            RefSCC &EdgeRC = *G->lookupRefSCC(*E.getNode());
+            if (G->getRefSCCIndex(EdgeRC) <= SourceIdx)
+              // Not in the postorder sequence between source and target.
+              continue;
+
+            if (Set.insert(&EdgeRC).second)
+              Worklist.push_back(&EdgeRC);
+          }
+    } while (!Worklist.empty());
+  };
 
+  // Use a generic helper to update the postorder sequence of RefSCCs and return
----------------
sanjoy wrote:
> Okay, I agree that `iterator_range<SmallVectorImpl<RefSCC *>::iterator>` isn't great. :)
> 
> However, I had made my suggestion under the assumption that `updatePostorderSequenceForEdgeInsertion` returns an `ArrayRef<RefSCC *>`.  Does changing `updatePostorderSequenceForEdgeInsertion` to return `ArrayRef<RefSCC *>` sound generally cleaner?
> 
> In any case, there's no reason to hold checking this on this specific point, it's very minor.
Doesn't really work ... we hand the range to erase, etc. We could hack it with addresses into the ArrayRef, but that seems pretty gross. Happy to keep tweaking the API though...

================
Comment at: unittests/Analysis/LazyCallGraphTest.cpp:142
@@ +141,3 @@
+     "define void @a1() {\n"
+     "entry:\n"
+     "  %a = alloca void ()*\n"
----------------
sanjoy wrote:
> Are the trailing `\n` 's needed?  Seems like we should be able to replace them with spaces (which may look less distracting).
I think so. But can try some stuff with this whole file in a follow-up commit.

================
Comment at: unittests/Analysis/LazyCallGraphTest.cpp:581
@@ -480,3 +580,3 @@
   for (LazyCallGraph::RefSCC &RC : CG.postorder_ref_sccs())
-    (void)RC;
+    dbgs() << "Formed RefSCC: " << RC << "\n";
 
----------------
sanjoy wrote:
> Writing to `dbgs()` seems fairly uncommon in `unittests/` (I only see one other instance).  I 
> can't see any obvious reason why this will be a problem, but perhaps there is a reason why it is usually avoided?
> 
No idea, it made debugging this stuff easier and it seems harmless...

================
Comment at: unittests/Analysis/LazyCallGraphTest.cpp:895
@@ +894,3 @@
+  LazyCallGraph::Node &B1 = *CG.lookup(lookupFunction(*M, "b1"));
+  LazyCallGraph::Node &B2 = *CG.lookup(lookupFunction(*M, "b2"));
+  LazyCallGraph::Node &B3 = *CG.lookup(lookupFunction(*M, "b3"));
----------------
sanjoy wrote:
> Can this sequence be a C macro or a C++ class with `A1`, `A2` etc. as fields (in the latter case all of the lookup logic could be in a constructor or some helper function)?  Right now this is repeated a few times.
> 
Yea, I'd like to refactor a bunch of this, probably in a follow-up. The repeated pattern got a lot more frequent somewhat organically.

================
Comment at: unittests/Analysis/LazyCallGraphTest.cpp:1082
@@ +1081,3 @@
+
+  // Connect the top to the bottom forming a large RefSCC made up mostly of calls.
+  auto MergedRCs = ARC.insertIncomingRefEdge(D, A);
----------------
sanjoy wrote:
> @eraman 's comment has not been addressed.
Doh, sorry I missed this. Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D24219