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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 4 inline comments as done.
t.p.northover added a comment.

Thanks for looking at the patch.

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

Very worthwhile because it revealed a problem with the iteration scheme. CallGraph nodes can become stale when the passes are run so I changed it to find the next node we're going to start from before running them.

As for actual compile time, across the test-suite it actually seemed to improve things by a tiny amount (0.1%). I don't actually trust that result, but it probably means any degradation is negligible.



================
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(); }
----------------
dexonsmith wrote:
> It's weird to have `begin()` take a `NodeRef` and `end()` take a `GraphT`, but I wonder: do we need these at all?
Yep, they are pretty weird. I'll make the constructors public and get rid of them.


================
Comment at: llvm/lib/Analysis/CallGraphSCCPass.cpp:456
+
+  std::set<const Function *> Visited;
+  for (auto I = CG.begin(), E = CG.end(); I != E; ++I) {
----------------
dexonsmith wrote:
> I would have expected `DenseSet` or `SmallDenseSet`.  Is there a reason you're preferring `std::set` here?
Yes, I somehow forgot about `DenseSet`. I deliberately avoided a Small variant because I can't imagine there are many translation units with a usefully small number of functions. So I'll update it do `DenseSet`.


Repository:
  rL LLVM

https://reviews.llvm.org/D48939