[llvm] r214897 - Remove dead zero store to calloc initialized memory

[Rui Ueyama]

We found that this patch seems to miscompile and confuse msan. I'm going to
roll it back. I think the follow up comments about the issue will come from
the people who know more on this.


On Tue, Aug 5, 2014 at 10:48 AM, Philip Reames <listmail at philipreames.com>
wrote:

> Author: reames
> Date: Tue Aug  5 12:48:20 2014
> New Revision: 214897
>
> URL: http://llvm.org/viewvc/llvm-project?rev=214897&view=rev
> Log:
> Remove dead zero store to calloc initialized memory
>
> Optimize the following IR:
>
> %1 = tail call noalias i8* @calloc(i64 1, i64 4)
> %2 = bitcast i8* %1 to i32*
> ; This store is dead and should be removed
> store i32 0, i32* %2, align 4
>
> Memory returned by calloc is guaranteed to be zero initialized. If the
> value being stored is the constant zero (and the store is not otherwise
> observable across threads), we can delete the store.  If the store is to an
> out of bounds address, it is undefined and thus also removable.
>
> Reviewed By: nicholas
>
> Differential Revision: http://reviews.llvm.org/D3942
>
>
>
> Added:
>     llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll
> Modified:
>     llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
>
> Modified: llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp?rev=214897&r1=214896&r2=214897&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp Tue Aug  5
> 12:48:20 2014
> @@ -514,30 +514,50 @@ bool DSE::runOnBasicBlock(BasicBlock &BB
>      if (!InstDep.isDef() && !InstDep.isClobber())
>        continue;
>
> -    // If we're storing the same value back to a pointer that we just
> -    // loaded from, then the store can be removed.
> +    // Check for cases where the store is redundant.
>      if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
> +      bool DeleteStore = false;
> +      // If we're storing the same value back to a pointer that we just
> +      // loaded from, then the store can be removed.
>        if (LoadInst *DepLoad = dyn_cast<LoadInst>(InstDep.getInst())) {
>          if (SI->getPointerOperand() == DepLoad->getPointerOperand() &&
>              SI->getOperand(0) == DepLoad && isRemovable(SI)) {
>            DEBUG(dbgs() << "DSE: Remove Store Of Load from same pointer:\n
>  "
>                         << "LOAD: " << *DepLoad << "\n  STORE: " << *SI <<
> '\n');
> +          DeleteStore = true;
> +        }
> +      }
>
> -          // DeleteDeadInstruction can delete the current instruction.
>  Save BBI
> -          // in case we need it.
> -          WeakVH NextInst(BBI);
> -
> -          DeleteDeadInstruction(SI, *MD, TLI);
> -
> -          if (!NextInst)  // Next instruction deleted.
> -            BBI = BB.begin();
> -          else if (BBI != BB.begin())  // Revisit this instruction if
> possible.
> -            --BBI;
> -          ++NumFastStores;
> -          MadeChange = true;
> -          continue;
> +      // If we find a store to memory which was defined by calloc
> +      // we can remove the store if the value being stored is a
> +      // constant zero (since calloc initialized the memory to
> +      // that same value) or the store is undefined (if out of
> +      // bounds).
> +      if (isCallocLikeFn(InstDep.getInst(), TLI) && isRemovable(SI)) {
> +        Value *V = SI->getValueOperand();
> +        if (isa<Constant>(V) && cast<Constant>(V)->isNullValue()) {
> +          DEBUG(dbgs() << "DSE: Remove Store Of Zero to Calloc:\n  "
> +                << "CALLOC: " << *InstDep.getInst() << "\n"
> +                << "STORE: " << *SI << '\n');
> +          DeleteStore = true;
>          }
>        }
> +
> +      if (DeleteStore) {
> +        // DeleteDeadInstruction can delete the current instruction.
>  Save BBI
> +        // in case we need it.
> +        WeakVH NextInst(BBI);
> +
> +        DeleteDeadInstruction(SI, *MD, TLI);
> +
> +        if (!NextInst)  // Next instruction deleted.
> +          BBI = BB.begin();
> +        else if (BBI != BB.begin())  // Revisit this instruction if
> possible.
> +          --BBI;
> +        ++NumFastStores;
> +        MadeChange = true;
> +        continue;
> +      }
>      }
>
>      // Figure out what location is being stored to.
>
> Added: llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll?rev=214897&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll (added)
> +++ llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll Tue
> Aug  5 12:48:20 2014
> @@ -0,0 +1,17 @@
> +; RUN: opt < %s -basicaa -dse -S | FileCheck %s
> +
> +target datalayout =
> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
> +
> +; Function Attrs: nounwind
> +declare noalias i8* @calloc(i64, i64)
> +
> +; Function Attrs: nounwind uwtable
> +define noalias i32* @test_store() {
> +; CHECK-LABEL: test_store
> +  %1 = tail call noalias i8* @calloc(i64 1, i64 4)
> +  %2 = bitcast i8* %1 to i32*
> +  ; This store is dead and should be removed
> +  store i32 0, i32* %2, align 4
> +; CHECK-NOT: store i32 0, i32* %2, align 4
> +  ret i32* %2
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140806/267620f2/attachment.html>