[PATCH] D28549: Global DCE performance improvement

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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_;
----------------
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.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:103
+      Deps.insert(LocalDeps.begin(), LocalDeps.end());
+    }
+  }
----------------
serge-sans-paille wrote:
> mehdi_amini wrote:
> > So the memorization is only used for constants (GV initializers, GEP, what else?), I'd rename it to make it explicit that this cache only applies to constants.
> > 
> > Also can such constant have multiple uses? I'm failing to remember in which case right now. If they only have a single use we could just traverse and remove the memorization entirely.
> See r179458
Thanks for the pointer!


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:126
+      for (auto &&CM : make_range(ComdatMembers_.equal_range(C)))
+        MarkLive(*CM.second, Updates);
+    }
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D28549