[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:35
 private:
-  SmallPtrSet<GlobalValue*, 32> AliveGlobals;
-  SmallPtrSet<Constant *, 8> SeenConstants;
-  std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
-
-  /// Mark the specific global value as needed, and
-  /// recursively mark anything that it uses as also needed.
-  void GlobalIsNeeded(GlobalValue *GV);
-  void MarkUsedGlobalsAsNeeded(Constant *C);
+  SmallPtrSet<GlobalValue*, 16> LiveGVs_;
+  DenseMap<GlobalValue*, SmallPtrSet<GlobalValue*, 8>> GVDependencies_;
----------------
Why did you rename this? Some part of this diff could disappear without this change.


================
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_;
----------------
`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.


================
Comment at: include/llvm/Transforms/IPO/GlobalDCE.h:38
+  std::unordered_map<Constant*, SmallPtrSet<GlobalValue*, 8>> DependenciesCache_;
+  std::unordered_multimap<Comdat*, GlobalValue *> ComdatMembers_;
+
----------------
We don't suffix member names.
Also it'd be nice to document these.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:101
+      for(auto* CEUser : CE->users())
+        ComputeDependencies(CEUser, LocalDeps);
+      Deps.insert(LocalDeps.begin(), LocalDeps.end());
----------------
The recursion is bound by the fact that we only recurse on constant and stop at the first GV reached. I'd rather have this as a comment in the code.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:103
+      Deps.insert(LocalDeps.begin(), LocalDeps.end());
+    }
+  }
----------------
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.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:113
+  for(auto* GVU : Deps) {
+    GVDependencies_[GVU].insert(&GV);
+  }
----------------
Why not passing GV to ComputeDependencies instead of Deps and populate directly `GVDependencies_` ?


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:114
+    GVDependencies_[GVU].insert(&GV);
+  }
+}
----------------
No braces


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:118
+// Mark Global value as Live
+void GlobalDCEPass::MarkLive(GlobalValue & GV, SmallVectorImpl<GlobalValue*>* Updates) {
+  auto const Ret = LiveGVs_.insert(&GV);
----------------
`GlobalValue & GV` does not seems formatted, I'd suggest running `git clang-format` on this patch.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:120
+  auto const Ret = LiveGVs_.insert(&GV);
+  if(Ret.second) {
+    LiveGVs_.insert(&GV);
----------------
Early return.


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:121
+  if(Ret.second) {
+    LiveGVs_.insert(&GV);
+    if(Updates)
----------------
What does this do? Isn't it redundant with the one two lines above?


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:126
+      for (auto &&CM : make_range(ComdatMembers_.equal_range(C)))
+        MarkLive(*CM.second, Updates);
+    }
----------------
What's the bound on the recursion? How do we guarantee that we won't run out of stack? Worth a comment. 


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:187
+    auto * LGV = NewLiveGVs.back();
+    NewLiveGVs.pop_back();
+    for(auto* GV : GVDependencies_[LGV]) {
----------------
`pop_back_val()`


================
Comment at: lib/Transforms/IPO/GlobalDCE.cpp:190
+      MarkLive(*GV, &NewLiveGVs);
+    }
   }
----------------
No brace


Repository:
  rL LLVM

https://reviews.llvm.org/D28549