[PATCH] D13021: [DeadStoreElimination] Remove dead zero store to calloc initialized memory

[Philip Reames via llvm-commits]

reames added a comment.

Mostly looks pretty good.  Minor, but required, comments.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:535
@@ +534,3 @@
+      // Remove null stores into the calloc'ed objects
+      const DataLayout &DL = BB.getModule()->getDataLayout();
+      Instruction *UnderlyingPointer = dyn_cast<Instruction>(
----------------
This should be outside the surrounding loop.  

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:537
@@ +536,3 @@
+      Instruction *UnderlyingPointer = dyn_cast<Instruction>(
+          GetUnderlyingObject(SI->getPointerOperand(), DL));
+      Constant *StoredConstant = dyn_cast<Constant>(SI->getValueOperand());
----------------
The code here looks correct, but I'm a bit concerned about compile time impact.  GetUnderlyingObject might be fairly slow (since it has to chase through GEPs and bitcasts).  Possibly you should check to see if you have an appropriate constant before executing this query?

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:663
@@ -639,2 +662,3 @@
 /// instruction.
-bool DSE::MemoryIsNotModifiedBetween(LoadInst *LI, StoreInst *SI) {
+bool DSE::MemoryIsNotModifiedBetween(Instruction *FirstI,
+                                     Instruction *SecondI) {
----------------
The rename looks correct, but I haven't reviewed the correctness of the existing code.

================
Comment at: test/Transforms/DeadStoreElimination/calloc-store.ll:9
@@ +8,3 @@
+; Function Attrs: nounwind uwtable
+define noalias i32* @test_store() {
+; CHECK-LABEL: test_store
----------------
Please add a couple of negative tests, specifically:
- base object is not a calloc function
- memory is clobbered
- store is volatile (i.e. not removable)
- store of wrong constant
- store of non-constant


Repository:
  rL LLVM

http://reviews.llvm.org/D13021