[llvm] r214897 - Remove dead zero store to calloc initialized memory
Benjamin Kramer
benny.kra at gmail.com
Thu Aug 7 01:51:24 PDT 2014
On Thu, Aug 7, 2014 at 12:43 AM, Philip Reames
<listmail at philipreames.com> wrote:
> I think I just spotted the error in this patch. I was checking that the
> defining instruction was a calloc, but I was not checking that the dependent
> instruction (the store) was actually storing to that calloc. Oops.
>
> I'll confirm this, prepare another version, and send it out for review.
>
> Are there easy instructions somewhere for replicating the msam build that
> failed?
The setup is a bit complex as there is some self-hosting required. Get
an LLVM+compiler-rt build and use that build LLVM with msan (there's a
cmake flag for that, LLVM_USE_SANITIZER=Memory). Run the tests, it'll
report false positives in FileCheck.
If you don't want to spend that much time on builds this may work for
you: compile lib/Support/regcomp.c with -fsanitize=memory and look at
the IR output before and after your patch is applied. It's pretty
obvious that a store unrelated to the calloc gets optimized out. That
store happens to be to the shadow memory where msan stores whether
memory at an address is initialized.
hope that helps
- Ben
> Philip
>
> On 08/06/2014 12:28 PM, Rui Ueyama wrote:
>
> 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
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list