[PATCH] D29381: [PM/LCG] Remove the lazy RefSCC formation from the LazyCallGraph during iteration.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

I personally like this solution as it strips away a lot of complexity. Some comments while I do some testing on this patch.



================
Comment at: include/llvm/Analysis/LazyCallGraph.h:1086-1090
+  template <typename RootsT, typename GetBeginT, typename GetEndT,
+            typename GetNodeT, typename FormSCCCallbackT>
+  static void buildGenericSCCs(RootsT &&Roots, GetBeginT &&GetBegin,
+                               GetEndT &&GetEnd, GetNodeT &&GetNode,
+                               FormSCCCallbackT &&FormSCC);
----------------
I think this API deserves a comment, also for consistency with everything else in the file.


================
Comment at: lib/Analysis/LazyCallGraph.cpp:1692-1693
 
-          assert(!lookupSCC(ChildN) &&
-                 "Found a node with 0 DFS number but already in an SCC!");
+          //assert(!lookupSCC(ChildN) &&
+          //       "Found a node with 0 DFS number but already in an SCC!");
           ChildN.DFSNumber = ChildN.LowLink = NextDFSNumber++;
----------------
Commented code?


================
Comment at: lib/Analysis/LazyCallGraph.cpp:1788
+  if (EntryEdges.empty() || !PostOrderRefSCCs.empty())
+    // RefSCCs are either non-existent or already bulit!
+    return;
----------------
s/bulit/built/


================
Comment at: lib/Analysis/LazyCallGraph.cpp:1797-1798
+
+  std::reverse(Roots.begin(), Roots.end());
+
+  buildGenericSCCs(
----------------
Comment on why you need this reverse, maybe?


================
Comment at: lib/Analysis/LazyCallGraph.cpp:1810-1811
+
+        // Push the new node into the postorder list.
+        bool Inserted =
+            RefSCCIndices.insert({NewRC, PostOrderRefSCCs.size()}).second;
----------------
"Push the new node into the postorder list " ... 
I would also add the fact you're inserting in the indices map (or move this comment down, up to you).


https://reviews.llvm.org/D29381