[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.

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Some comments inline.


================
Comment at: include/llvm/Analysis/LazyCallGraph.h:920
@@ +919,3 @@
+  ///
+  /// Only RefSCCs which have been created while walking the graph are in the
+  /// map.
----------------
Not sure what you mean by "Only RefSCCs which have been created while walking the graph" -- what else is there?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:827
@@ +826,3 @@
+    Set.insert(&SourceC);
+    auto IsConnected = [&](RefSCC &RC) {
+      for (SCC &C : RC)
----------------
Why can't you do a DFS on the parent edges starting from `SourceC`, and bound it by the range in the postorder list?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:870
@@ -869,8 +869,3 @@
 
-      // 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 generic helper to update the postorder sequence of SCCs and return
+  // a range of any SCCs connected into a cycle by inserting this edge. This
----------------
s/SCCs/RefSCCs/ right?

Also, I know this is "controversial", but I think using an explicit type for `MergeRange` here is actually going to be more readable.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:874
@@ +873,3 @@
+  // sequence.
+  auto MergeRange = updatePostorderSequenceForEdgeInsertion(
+      SourceC, *this, G->PostOrderRefSCCs, G->RefSCCIndices,
----------------
Nice!

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1215
@@ +1214,3 @@
+  // Insert the resulting postorder sequence into the global graph postorder
+  // sequence before the current RefSCC in that sequnece. The idea being that
+  // this RefSCC is the target of the reference edge removed, and thus has
----------------
s/sequnece/sequence/

================
Comment at: lib/Analysis/LazyCallGraph.cpp:892
@@ +891,3 @@
+      if (!MergeSet.count(ParentRC))
+        Parents.insert(ParentRC);
+    RC->Parents.clear();
----------------
`copy_if` ?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:937
@@ +936,3 @@
+  for (RefSCC *RC : make_range(EraseEnd, G->PostOrderRefSCCs.end()))
+    G->RefSCCIndices[RC] -= IndexOffset;
+
----------------
I know this isn't new, but does it make sense to erase the erased RefSCCs from `RefSCCIndices` too?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1221
@@ +1220,3 @@
+  if (!Result.empty()) {
+    int Idx = G->RefSCCIndices.find(this)->second;
+    G->PostOrderRefSCCs.insert(G->PostOrderRefSCCs.begin() + Idx,
----------------
How about adding an accessor to LCG like `getIndexForRefSCC` that verifies that the index into the postorder is correct?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1225
@@ +1224,3 @@
+    for (int i = Idx, Size = G->PostOrderRefSCCs.size(); i < Size; ++i)
+      G->RefSCCIndices[G->PostOrderRefSCCs[i]] = i;
+    assert(G->PostOrderRefSCCs[G->RefSCCIndices.find(this)->second] == this &&
----------------
Use `llvm::seq` ?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1588
@@ +1587,3 @@
+    // Push the new node into the post-order list and return true indicating we
+    // successfully grew the post-order sequece by one.
+    bool Inserted =
----------------
*sequence


https://reviews.llvm.org/D24219