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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Some small 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.
----------------
s/modeled/modelled
s/MemroySSA/MemorySSA


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1465
+    // alloc-like objects.
+    for (Instruction &I : instructions(F)) {
+      if (I.mayThrow() && !MSSA.getMemoryAccess(&I))
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1587
+    LLVM_DEBUG(dbgs() << "  Checking for reads of " << *DomAccess);
+    if (isa<MemoryDef>(DomAccess))
+      LLVM_DEBUG(dbgs() << " (" << *cast<MemoryDef>(DomAccess)->getMemoryInst()
----------------
Reduce scope of DomAccess to inside the do-while loop and use DomDef here, hence no need for the `if (isa<MemoryDef>(DomAccess))` check - see previous comment (please mark them as done when updating).


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1592
+
+    SmallVector<MemoryAccess *, 4> WorkList;
+    auto PushMemUses = [&WorkList](MemoryAccess *Acc) {
----------------
SmallSetVector to avoid processing duplicates?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1643
+        PushMemUses(UseDef);
+        if (isWriteClobber(DefLoc, UseInst)) {
+          LLVM_DEBUG(dbgs() << "  ... may-write clobber\n");
----------------
Can this happen? Wouldn't the getClobbering above have found UseAccess /UseInst instead of DomDef?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1686
+  // Check for any extra throws between SI and NI that block DSE.  This only
+  // checks extra maythrows (those that arn't MemoryDef's). MemoryDef that may
+  // throw are handled during the walk from one def to the next.
----------------
s/arn't/aren't


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72700