[PATCH] D16802: [LCG] Construct an actual call graph with call-edge SCCs nested inside reference-edge SCCs.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Some more mostly minor comments:


================
Comment at: include/llvm/Analysis/LazyCallGraph.h:52
@@ -51,2 +51,3 @@
 #include <iterator>
+#include <utility>
 
----------------
Looks like this header is unused.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:485
@@ +484,3 @@
+    /// If that happens, the deleted SCC pointers are returned. These SCCs are
+    /// not in a valid state any longer but the pointers will remain valid for
+    /// the purpose of clearing cached information.
----------------
Minor:  I'd be specific about "remain valid till ... (destruction of the parent LCG?)"

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:497
@@ +496,3 @@
+    ///
+    /// If SourceN ant may be split up due to breaking a cycle in the call
+    /// edges that formed it. If that happens, then this will potentially
----------------
SourceN ant?  This first sentence sounds malformed.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:105
@@ +104,3 @@
+void LazyCallGraph::Node::insertEdgeInternal(Node &TargetN, Edge::Kind EK) {
+  EdgeIndexMap.insert(std::make_pair(&TargetN.getFunction(), Edges.size()));
+  Edges.emplace_back(TargetN, EK);
----------------
Here and elsewhere: why not `{&TargetN.getFunction(), Edges.size()}` instead of an explicit `std::make_pair`?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:110
@@ +109,3 @@
+void LazyCallGraph::Node::setEdgeKind(Function &TargetF, Edge::Kind EK) {
+  Edges[EdgeIndexMap.find(&TargetF)->second].setKind(EK);
+}
----------------
Why not `(*this)[&TargetF].setKind(EK)`?

================
Comment at: lib/Analysis/LazyCallGraph.cpp:733
@@ -219,2 +732,3 @@
+#endif
 }
 
----------------
The doc updates lgtm

================
Comment at: lib/Analysis/LazyCallGraph.cpp:788
@@ -268,1 +787,3 @@
+      if (ConnectedSet.count(&Parent)) {
+        ConnectedDepth = std::max<int>(ConnectedDepth, DFSStack.size());
         continue;
----------------
I think this can just be `ConnectedDepth = DFSStack.size()` (with an `assert(ConnectedDepth < (int)DFSStack.size())`).

================
Comment at: lib/Analysis/LazyCallGraph.cpp:1026
@@ +1025,3 @@
+    do {
+      Node *N;
+      edge_iterator I;
----------------
SGTM


http://reviews.llvm.org/D16802