[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 18 inline comments as done.
chandlerc added a comment.

Thanks for all the comments Sanjoy and Easwaran!


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

================
Comment at: lib/Analysis/LazyCallGraph.cpp:827
@@ +826,3 @@
+    Set.insert(&SourceC);
+    auto IsConnected = [&](RefSCC &RC) {
+      for (SCC &C : RC)
----------------
sanjoy wrote:
> eraman wrote:
> > chandlerc wrote:
> > > I mean, I could. Do you think that would be simpler? It seems like it would be strictly more complicated than this, and still O(edges), so it didn't seem like it was worth pursuing. But I'm happy to try it out if you think it'd be an improvement.
> > Isn't what Sanjoy proposing O(|edges in RefSCC DAG|)  which is a tighter bound than O(|edges in the callgraph|) of the current implementation? 
> > Do you think that would be simpler?
> 
> Not in a one-to-one comparison, but perhaps you could write it in a way that can be shared with `ComputeTargetConnectedSet` ?
> 
> + What @eraman said.
Sanjoy and I chatted about this on IRC and now I understand *much* better what your idea is here Sanjoy.

The idea is just to walk the parent edges to build connected sets. It's not the full complexity (in code) of Tarjan's, its just walking like this but with a nicer complexity bound.

However, thinking about this I think I've made this entire problem much more complex than is necessary. But the simplification I have in mind will impact both this code and the SCC caller, so I'd rather do that in a follow-up if its OK.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:870-876
@@ -869,9 +869,9 @@
 
-      // 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
+  // routine will also take care of updating the indices into the postorder
+  // sequence.
+  auto MergeRange = updatePostorderSequenceForEdgeInsertion(
+      SourceC, *this, G->PostOrderRefSCCs, G->RefSCCIndices,
+      ComputeSourceConnectedSet, ComputeTargetConnectedSet);
 
----------------
Done on the first count.

The type seems really long, opaque, and to add little value to me? It's: `iterator_range<SmallVectorImpl<RefSCC *>::iterator>`. That said, if it makes it easier to read, no problem. I've put it in here, and I can update the other caller.

To help me understand this better, what about seeing this type helps you read the code?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:892
@@ +891,3 @@
+      if (!MergeSet.count(ParentRC))
+        Parents.insert(ParentRC);
+    RC->Parents.clear();
----------------
sanjoy wrote:
> `copy_if` ?
It's quite awkward to use. std::inserter doesn't help because the insert routine doesn't accept an iterator position (and even if it did, there is no meaningful iterator to provide).

We could use the range insert routine and a filtered iterator, but that seems also overkill.

================
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 &&
----------------
sanjoy wrote:
> Use `llvm::seq` ?
Hah, good idea!


https://reviews.llvm.org/D24219