[PATCH] D11143: [RFC] Cross Block DSE

[Erik Eckstein via llvm-commits]

eeckstein added a subscriber: eeckstein.
eeckstein added a comment.

Hi Karthik,
Daniel pointed me to your patch, because I'm also working on something in DeadStoreElimination: http://reviews.llvm.org/D11854

I think your patch is not directly related to what I'm doing. Nevertheless I looked at your patch and posted some comments.

Erik


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:949
@@ +948,3 @@
+
+  for (auto BBE = SrcBlock->end(); BBI != BBE; ++BBI) {
+    Instruction *I = BBI;
----------------
As I understand, this loop is just to get the iterator for I.
You can get it simply by:
BasicBlock::iterator BBI(SI);

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:959
@@ +958,3 @@
+    Instruction *I = BI;
+    ModRefInfo Res = AA->getModRefInfo(I, Pointer, PtrSize);
+    if (Res != MRI_NoModRef) {
----------------
I think using PtrSize here is wrong. It should be the size of the memory access. This can give you a wrong NoModRef, e.g. if I is a load of a global which has a smaller size than the pointer size.

Maybe you should just get the MemoryLocation of SI instead of Pointer+PtrSize and use AA->getModRefInfo(I, Location).

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1011
@@ +1010,3 @@
+        continue;
+      // A path with backedge may not be safe.Conservatively mark
+      // this store unsafe.
----------------
Why is a path with a backedge not safe?


http://reviews.llvm.org/D11143