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

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 21 08:18:45 PDT 2015

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



More information about the llvm-commits mailing list