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

[Easwaran Raman via llvm-commits]

eraman added inline comments.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:883
@@ -896,3 +882,3 @@
   // a connected set with the inserted edge, merge all of them into this SCC.
   // We walk the newly connected RefSCCs in the reverse postorder of the parent
   // DAG walk above and merge in each of their SCC postorder lists. This
----------------
The comment is stale now since there is no DAG walk above anymore.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:888
@@ -901,3 +887,3 @@
   int SCCIndex = 0;
-  for (RefSCC *C : reverse(Connected)) {
-    assert(C != this &&
+  for (RefSCC *RC : reverse(MergeRange)) {
+    assert(RC != this &&
----------------
I don't think you should be reversing the MergeRange here. Consider this example where you have three RefSCCs R0, R1 and R2 with the numbers being index to the PostOrderRefSCCs. Assume R2 is this RefSCC and the edge being added is from R0 -> R2. Assume each Ri has one Si (call SCC). MergeRange is [R0, R1].  At the end of this loop, MergedSCCs will have [S1, S0] and outside the loop S2 gets added to MergedSCCs resulting in [S1, S0, S2]. I believe you want it to be [S0, S1, S2] to keep it in post order. 

The unit test below doesn't test that call SCC's are in post-order. Ensuring verify on the merged RefSCC doesn't assert helps in general (but still won't catch this)



https://reviews.llvm.org/D24219