[PATCH] D98288: [DSE] Translate killing locations through phis.

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1895
+    SmallPtrSet<MemoryAccess *, 32> SeenAccesses;
+    SmallVector<std::pair<MemoryAccess *, MemoryLocation>, 32> ToCheck;
 
----------------
Are you sure 32 is good default? Maybe just not to set it?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1908
+      std::tie(Current, CurrentLoc) = ToCheck[I];
+      if (!SeenAccesses.insert(Current).second)
+        continue;
----------------
Why do you need that?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1915
       Optional<MemoryAccess *> Next = State.getDomMemoryDef(
-          KillingDef, Current, SILoc, SILocUnd, ScanLimit, WalkerStepLimit,
+          KillingDef, Current, CurrentLoc, SILocUnd, ScanLimit, WalkerStepLimit,
           IsMemTerm, PartialLimit);
----------------
My understanding is the third parameter of getDomMemoryDef should be location of killing store. Changing it to phi translated one seems problematic.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1947
+                                          false) &&
+                  Addr.getAddr())
+                NewLoc = CurrentLoc.getWithNewPtr(Addr.getAddr());
----------------
Addr is expected to be non null if successfully translated. Turn into assert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98288