[PATCH] D29149: NewGVN: Add basic dead store elimination

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

The logic is sound, I left some comments inline while I try on my tests and see how it performs =)



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:90
           "Number of times a member dominated it's new classes' leader");
+STATISTIC(NumGVNDeadStores, "Number of dead stores eliminated");
 
----------------
I would use "dead/redundant stores eliminated" here, but up to you.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:351-352
   struct ValueDFS;
-  void convertDenseToDFSOrdered(CongruenceClass::MemberSet &,
+  void convertDenseToDFSOrdered(const CongruenceClass::MemberSet &,
                                 SmallVectorImpl<ValueDFS> &);
+  void convertDenseToLoadsAndStores(const CongruenceClass::MemberSet &,
----------------
This `constness` is independent? Probably it's not worth changing it separately, just checking.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1496-1503
+  // If all that is left is nothing, our memoryphi is undef.
+  // Note: The only case this should happen is if we have at least one
+  // self-argument.
+  if (Filtered.begin() == Filtered.end()) {
+    if (setMemoryAccessEquivTo(MP, MSSA->getLiveOnEntryDef()))
+      markMemoryUsersTouched(MP);
+    return;
----------------
Is there a way to test this behaviour? (self-referencing phis)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1972-1979
+void NewGVN::convertDenseToLoadsAndStores(
+    const CongruenceClass::MemberSet &Dense,
+    SmallVectorImpl<ValueDFS> &LoadsAndStores) {
+  for (auto D : Dense) {
+    if (!isa<LoadInst>(D) && !isa<StoreInst>(D))
+      continue;
+
----------------
Add a comment (at the top of the function) to explain what this function is doing?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1986
+
+    // If it's an instruction, use the real local dfs number.
+    if (auto *I = dyn_cast<Instruction>(D))
----------------
This comment can be probably removed, as it *must* be an instruction =)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2126
       for (auto M : CC->Members) {
-
         Value *Member = M;
----------------
Spurious newline? Here and the next two)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2154-2156
+  for (auto *CC : CongruenceClasses) {
+    // Track the equivalent store info so we can decide whether to try
+    // dead store elimination.
----------------
I still prefer the explicit type here as it's not obvious which is (well the name implies, but still). Anyway, up to you.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2273
+            PossibleDeadStores.push_back(VD);
+
           // Skip the Value's, we only want to eliminate on their uses.
----------------
dberlin wrote:
> This is leftover from when i was trying to only walk the members once, but harmless.
> removing it.
Thanks.


https://reviews.llvm.org/D29149