[PATCH] D90328: Eliminates dead store of an exisiting value

[Florian Hahn via Phabricator via llvm-commits]

fhahn requested changes to this revision.
fhahn added a comment.
This revision now requires changes to proceed.

A few more comments inline. I think before checking the patch with Clang on C/C++ code, it would be good to make sure that all unit tests pass ( `check-llvm-transforms-deadstoreelimination` target)



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2393
+    for (int I = MemDefs.size() - 1; I >= 0; I--) {
+      MemoryDef *Def = MemDefs[I];
+      MemoryAccess *DefAccess = dyn_cast<MemoryAccess>(Def);
----------------
It is possible that memory defs in MemDefs get deleted. You need to skip defs in `SkipStores`.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2403
+      }
+      for (Use &U : DefAccess->uses()) {
+        MemoryAccess *UseAccess = cast<MemoryAccess>(U.getUser());
----------------
If we change below to remove the UseInst, we need to use iterators with early increment here, to avoid iterator invalidation.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2406
+        if (MemoryDef *UseDef = dyn_cast<MemoryDef>(UseAccess)) {
+          MemoryAccess *UseDefAccess = UseDef->getDefiningAccess();
+          if (UseDefAccess == NULL) {
----------------
Why are we getting get defining access here? Shouldn't we just look at the uses?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:2415
+          if (UseInst->isIdenticalTo(DefInst)) {
+            deleteDeadInstruction(DefInst);
+            MadeChange = true;
----------------
This seems off, shouldn't we remove DefInst? Also, we need to make sure we only remove defs that do not also read memory I think.


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

https://reviews.llvm.org/D90328