[PATCH] D100464: [DSE] Remove stores in the same loop iteration

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, thanks! I think it would be good to update some of the naming/comments slightly (as per the comments) before committing.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:940
+    // AliasAnalysis does not account for loops. Limit overwrite checks to
+    // candidates for which we can guarantee they always store to the same
+    // memory location and not located in different loops.
----------------
nit: 'the same location' may be a bit confusing here. I guess it's meant as the same `MemoryLocation` value, but we can store to different concrete locations for locations that depend on a loop induction for example.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1278
+                                 const MemoryLocation &CurrentLoc) {
+    // Limit elimination to candidates for which we can guarantee they always
+    // store to the same memory location and not located in different loops. But
----------------
Referring to candidates & elimination here seems a bit out-of-place given the name & doc-comment of the function. It would be good to update the function name (doesn't check for invariance any longer). It now more accurately tries to rule out cross-iteration dependencies?


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

https://reviews.llvm.org/D100464