[PATCH] D72146: [DSE] Add first version of MemorySSA-backed DSE.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Thank you for the other patch! I added a few comments on the bottom up approach. I think overall it's a better high-level approach.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1604
+// 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 MemoryDef's/aren't MemoryDefs
s/MemoryDef that/MemoryDefs that


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1722
+      MemoryDef *ND = dyn_cast<MemoryDef>(NextAccess);
+      Instruction *NI = ND->getMemoryInst();
+      LLVM_DEBUG(dbgs() << "  def " << *NI << "\n");
----------------
There's a contradiction here. Either NextAccess is always a MemoryDef, in which case you can make it a MemoryDef inside the struct and remove the cast, or it can be a MemoryPhi, in which case ND is null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146