[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
Repository:
rL LLVM
http://reviews.llvm.org/D13021
More information about the llvm-commits
mailing list