[PATCH] D28549: Global DCE performance improvement

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: include/llvm/Transforms/IPO/GlobalDCE.h:36
+  SmallPtrSet<GlobalValue*, 16> LiveGVs_;
+  DenseMap<GlobalValue*, SmallPtrSet<GlobalValue*, 8>> GVDependencies_;
+  std::unordered_map<Constant*, SmallPtrSet<GlobalValue*, 8>> DependenciesCache_;
----------------
mehdi_amini wrote:
> serge-sans-paille wrote:
> > mehdi_amini wrote:
> > > `DenseMap` is intended to contains small value, `SmallPtrSet` seems a bit `fat` here?
> > > 
> > > Also, after reading the algorithm, this could be a vector, you should not need dedup by construction.
> > According to my tests, DenseMap performs better on large test case than std::unorderd_map (by a factor of two)
> > 
> > Correct for Set->Vector
> I'm not sure what you mean by "large test case"? Do you mean lots of entries? My point was about `sizeof(Map[key])` and memory consumption.
Yeah I meant lots of entries. I've done the tests with an std::vector instead and it does not impact the execution time, and it's indeeded better on the memory usage, switching to this.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:126
+      for (auto &&CM : make_range(ComdatMembers_.equal_range(C)))
+        MarkLive(*CM.second, Updates);
+    }
----------------
mehdi_amini wrote:
> serge-sans-paille wrote:
> > mehdi_amini wrote:
> > > What's the bound on the recursion? How do we guarantee that we won't run out of stack? Worth a comment. 
> > There's a finite number of globals and they can only be visited once because of the set check.
> I'm not worried about *infinite* recursion, but *too large* recursion for the limit stack size in some environment. If the recursion is in `O(global)` that's a potential stack overflow.
Previous algorithm worst case recursion depth was in ``O(|global|)`` and that happened, for instance, when a global function references a static function that references a static function that ... etc.

I'm not 100% sure but this function has a worst case recursion depth of two, has it only propagates the liveness to the whole comdat section. If you agree with my analyse, I'll update the comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D28549