[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