[PATCH] D48939: CallGraph passes: iterate over all functions rather than just externally visible ones

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a reviewer: dexonsmith.
dexonsmith added a comment.

Awesome.  I wonder, would it be worth measuring compile-time impact for optimized builds?



================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:102-105
+  static scc_iterator begin(NodeRef N) {
+    return scc_iterator(N);
   }
   static scc_iterator end(const GraphT &) { return scc_iterator(); }
----------------
It's weird to have `begin()` take a `NodeRef` and `end()` take a `GraphT`, but I wonder: do we need these at all?


================
Comment at: llvm/lib/Analysis/CallGraphSCCPass.cpp:456
+
+  std::set<const Function *> Visited;
+  for (auto I = CG.begin(), E = CG.end(); I != E; ++I) {
----------------
I would have expected `DenseSet` or `SmallDenseSet`.  Is there a reason you're preferring `std::set` here?


================
Comment at: llvm/lib/Analysis/CallGraphSCCPass.cpp:458
+  for (auto I = CG.begin(), E = CG.end(); I != E; ++I) {
+    if (Visited.find(I->first) == Visited.end())
+      Changed |= runOnNode(I->second.get(), CG, Visited);
----------------
I think `if (!Visited.count(I->first))` might be more clear.


================
Comment at: llvm/lib/Analysis/CallGraphSCCPass.cpp:470
   // Walk the callgraph in bottom-up SCC order.
-  scc_iterator<CallGraph*> CGI = scc_begin(&CG);
+  scc_iterator<CallGraph*> CGI = scc_begin<CallGraph*>(N);
 
----------------
Might as well add a space to `CallGraph *` (or have clang-format do it) since you're touching this line.  Also, I wonder if `auto` is just as clear as `scc_iterator<...>` here, since we're calling a begin function?


================
Comment at: llvm/lib/Analysis/CallGraphSCCPass.cpp:480
+    // twice.
+    for (auto Elt : NodeVec)
+      Visited.insert(Elt->getFunction());
----------------
Can you make this `auto *Elt` so it's clear that it's a raw pointer?


Repository:
  rL LLVM

https://reviews.llvm.org/D48939