[PATCH] D28549: Global DCE performance improvement

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

I wonder if your diff is correct, it seems that you updated the diff compare to the previous diff state (like if you didn't squash commits).



================
Comment at: include/llvm/Transforms/IPO/GlobalDCE.h:42
+  std::unordered_multimap<Comdat *, GlobalValue *>
+      ComdatMembers; // Comdat -> Globals in that Comdat section.
 
----------------
Put the comments on the line before the variables, and use `///` (doxygen-like prefix).


================
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:
> > 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.
Yes I guess a GV can belong only to a single comdat so it should be fine!


Repository:
  rL LLVM

https://reviews.llvm.org/D28549