[PATCH] D79391: [DSE] Remove noop stores in MSSA.

[Zoe Carver via Phabricator via llvm-commits]

zoecarver marked 2 inline comments as done.
zoecarver added a comment.

Thanks for the review.

> I would suggest to focus on the statistics about removed stores and how the patch impacts them.

What do you think about adding a statistic for noop stores? Or should I increment `NumRedundantStores ` or a different statistic? I think as-is this patch doesn't change any statistics.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1846
+  // access may point to it. So, pretend like we've already checked it.
+  ToCheck.insert(KillingDef);
+  ToCheck.insert(Current);
----------------
fhahn wrote:
> IIUC the MemoryPHI handling should be optional, right? I would recommend splitting it off in a separate patch, as smaller patches make it generally easier to review/submit.
Yes, it's optional. I'll make this a separate patch. 


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1897
+                          << '\n');
+        break;
+      }
----------------
fhahn wrote:
> shouldn't that be `continue`? (Test case would be a scenario where we first remove a no-op store and then some other stores by another def in State.MemDefs.
Yes, you're right. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79391/new/

https://reviews.llvm.org/D79391