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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 7 inline comments as done.
dberlin added a comment.

This now depends on a fix for 31761 to be committed, but otherwise shoudl be in good shape once i added the self-reference testcase.



================
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");
 
----------------
davide wrote:
> I would use "dead/redundant stores eliminated" here, but up to you.
fixed


================
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 &,
----------------
davide wrote:
> This `constness` is independent? Probably it's not worth changing it separately, just checking.
yes


================
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;
----------------
davide wrote:
> Is there a way to test this behaviour? (self-referencing phis)
It's hard to get it to occur naturall
I believe i can add an irreducible testcase where the phis are in a cycle that resolves to themselves.



================
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;
+
----------------
davide wrote:
> Add a comment (at the top of the function) to explain what this function is doing?
Done


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


================
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.
----------------
davide wrote:
> I still prefer the explicit type here as it's not obvious which is (well the name implies, but still). Anyway, up to you.
I put it back


https://reviews.llvm.org/D29149