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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Logic looks correct now, and new compile-time impact (https://llvm-compile-time-tracker.com/compare.php?from=e42636d3c1a41a9b7c5d8095ae5ef6682e26d4a2&to=502b1e5a2bd94706da3df367ddf558fdef7e2a2e&stat=instructions) also looks fine to me.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:942
+    // memory location and not located in different loops.
+    if (!IsGuaranteedLoopInvariant(EarlierI, LaterI, Later))
+      return OW_Unknown;
----------------
While it ultimately shouldn't make a difference, I think this should be passing `Earlier` as the memory location.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1273
   /// MemoryLocation during execution of the containing function.
-  bool IsGuaranteedLoopInvariant(Value *Ptr) {
-    auto IsGuaranteedLoopInvariantBase = [this](Value *Ptr) {
+  bool IsGuaranteedLoopInvariant(const Instruction *Current,
+                                 const Instruction *KillingDef,
----------------
Doc comment is outdated now -- and the function should probably also get renamed as well.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1275
+                                 const Instruction *KillingDef,
+                                 MemoryLocation CurrentLoc) {
+    // Limit elimination to candidates for which we can guarantee they always
----------------
`const MemoryLocation &`


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

https://reviews.llvm.org/D100464