[PATCH] D111727: [DSE] Eliminates redundant store of an exisiting value (PR16520)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the updates. I left 2 more additional suggestions, please feel free to address them directly in the commit version.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1965
+      deleteDeadInstruction(DefInst);
+      NumRedundantStores++;
+      MadeChange = true;
----------------
for debugging purposes it would be helpful if we had a debug message for removed stores here, like we have in other places.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2131
 
+  MadeChange |= State.eliminateRedundantStoresOfExistingValues();
   if (EnablePartialOverwriteTracking)
----------------
It might be better to eliminate the redundant stores after `removePartiallyOverlappedStores`, as this might shorten memsets based on state collected earlier. This will become an issue once we support eliminating stores made redundant by a memset, as in D112321


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111727