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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1886
 
     MemoryAccess *Current = KillingDef;
     LLVM_DEBUG(dbgs() << "Trying to eliminate MemoryDefs killed by "
----------------
asbirlea wrote:
> `Current` unused in release builds.
thanks, I removed the variable.


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


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1908
+      std::tie(Current, CurrentLoc) = ToCheck[I];
+      if (!SeenAccesses.insert(Current).second)
+        continue;
----------------
ebrevnov wrote:
> Why do you need that?
It's not needed in the latest version. I removed it. Thanks!


================
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);
----------------
ebrevnov wrote:
> My understanding is the third parameter of getDomMemoryDef should be location of killing store. Changing it to phi translated one seems problematic.
The location is used to find a potentially dead store.  We should already have checked all candidates after the phi, so I don't think we would miss any candidates after translating. And when scanning for reads, AA should be conservative when dealing with the translated location. Is there anything in particular you think could be problematic?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98288