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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 4 inline comments as done.
asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1463
+    DSEState State(F, AA, MSSA, DT, PDT, TLI);
+    // Collect blocks with throwing instructions not modeled in MemroySSA and
+    // alloc-like objects.
----------------
fhahn wrote:
> asbirlea wrote:
> > s/modeled/modelled
> > s/MemroySSA/MemorySSA
> I'm not a native speaker, but I think modeled is the US spelling and modelled is the UK spelling. I thought we prefer the US spelling, but I don't mind either way.
Yes, you're right, I'm not a native speaker either. Thank you for the correction!


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1465
+    // alloc-like objects.
+    for (Instruction &I : instructions(F)) {
+      if (I.mayThrow() && !MSSA.getMemoryAccess(&I))
----------------
fhahn wrote:
> asbirlea wrote:
> > Consider adding a cl::opt limit on the things you look to process in F. We've had this issue with generated code where a BB has order of thousands stores.
> I've added a rather generous limit for Defs per BB, but we can always adjust it later. We still have to check the whole basic block for throwing instructions.
sgtm


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1592
+
+    SmallVector<MemoryAccess *, 4> WorkList;
+    auto PushMemUses = [&WorkList](MemoryAccess *Acc) {
----------------
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!


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1643
+        PushMemUses(UseDef);
+        if (isWriteClobber(DefLoc, UseInst)) {
+          LLVM_DEBUG(dbgs() << "  ... may-write clobber\n");
----------------
fhahn wrote:
> asbirlea wrote:
> > Can this happen? Wouldn't the getClobbering above have found UseAccess /UseInst instead of DomDef?
> I think it can happen in loops where a store in a the header is killed by a store in the exit, but it is also stored to in the loop body. I've added a few additional test cases to llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-loops.ll loop_multiple_def* .
> 
> I've also moved the post dominance check in into the function and also updated the check here to skip PushMemUses for MemoryDefs that completely overwrite DefLoc. This helps with avoiding unnecessary checks.
I see, yes, I wasn't thinking with the MemoryPhi condition lifted.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72700