[PATCH] D72700: [DSE] Add first version of MemorySSA-backed DSE (Bottom up walk).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1592
+
+    SmallVector<MemoryAccess *, 4> WorkList;
+    auto PushMemUses = [&WorkList](MemoryAccess *Acc) {
----------------
asbirlea wrote:
> fhahn wrote:
> > asbirlea wrote:
> > > SmallSetVector to avoid processing duplicates?
> > Done. I *think* in the initial version the only nodes we might add multiple times are MemoryPhis and we bail out on the first one we see. I've added such a test to llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-memoryphis.ll
> I think when you process uses, you may find 2 different operands for a Def, if that Def is optimized. Something like:
> ```
> 1 = Def(LoE)
> 2 = Def (1)
> 3 = Def (2) - Optimized field = 1
> ```
> So if adding uses of 1, one may add (2, 3), then when processing 2, (3) is a use and added again. This may be too convoluted to actually happen, but thought it's worth to have the safety net. Thank you for the update!
Thanks for the example! That should be handled now :)


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1684
+
+      // Remove it's operands
+      for (Use &O : DeadInst->operands())
----------------
asbirlea wrote:
> s/it's/its
Updated, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72700