<div dir="ltr"><div class="gmail_extra">I don't know the details of how to reproduce this easily, and the person who did it is in a different timezone. I hope a followup will come soon. I'm sorry for the inconvenience.<br>
<br><div class="gmail_quote">On Wed, Aug 6, 2014 at 3:43 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
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. <br>
<br>
I'll confirm this, prepare another version, and send it out for
review. <br>
<br>
Are there easy instructions somewhere for replicating the msam build
that failed?<div class=""><br>
<br>
Philip<br>
<br>
<div>On 08/06/2014 12:28 PM, Rui Ueyama
wrote:<br>
</div>
</div><div><div class="h5"><blockquote type="cite">
<div dir="ltr">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.</div>
<div class="gmail_extra">
<br>
<br>
<div class="gmail_quote">On Tue, Aug 5, 2014 at 10:48 AM, Philip
Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: reames<br>
Date: Tue Aug 5 12:48:20 2014<br>
New Revision: 214897<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=214897&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=214897&view=rev</a><br>
Log:<br>
Remove dead zero store to calloc initialized memory<br>
<br>
Optimize the following IR:<br>
<br>
%1 = tail call noalias i8* @calloc(i64 1, i64 4)<br>
%2 = bitcast i8* %1 to i32*<br>
; This store is dead and should be removed<br>
store i32 0, i32* %2, align 4<br>
<br>
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.<br>
<br>
Reviewed By: nicholas<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D3942" target="_blank">http://reviews.llvm.org/D3942</a><br>
<br>
<br>
<br>
Added:<br>
llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp<br>
<br>
Modified:
llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp?rev=214897&r1=214896&r2=214897&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp?rev=214897&r1=214896&r2=214897&view=diff</a><br>
==============================================================================<br>
---
llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
(original)<br>
+++
llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
Tue Aug 5 12:48:20 2014<br>
@@ -514,30 +514,50 @@ bool DSE::runOnBasicBlock(BasicBlock
&BB<br>
if (!InstDep.isDef() && !InstDep.isClobber())<br>
continue;<br>
<br>
- // If we're storing the same value back to a pointer
that we just<br>
- // loaded from, then the store can be removed.<br>
+ // Check for cases where the store is redundant.<br>
if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {<br>
+ bool DeleteStore = false;<br>
+ // If we're storing the same value back to a pointer
that we just<br>
+ // loaded from, then the store can be removed.<br>
if (LoadInst *DepLoad =
dyn_cast<LoadInst>(InstDep.getInst())) {<br>
if (SI->getPointerOperand() ==
DepLoad->getPointerOperand() &&<br>
SI->getOperand(0) == DepLoad &&
isRemovable(SI)) {<br>
DEBUG(dbgs() << "DSE: Remove Store Of Load
from same pointer:\n "<br>
<< "LOAD: " << *DepLoad
<< "\n STORE: " << *SI << '\n');<br>
+ DeleteStore = true;<br>
+ }<br>
+ }<br>
<br>
- // DeleteDeadInstruction can delete the current
instruction. Save BBI<br>
- // in case we need it.<br>
- WeakVH NextInst(BBI);<br>
-<br>
- DeleteDeadInstruction(SI, *MD, TLI);<br>
-<br>
- if (!NextInst) // Next instruction deleted.<br>
- BBI = BB.begin();<br>
- else if (BBI != BB.begin()) // Revisit this
instruction if possible.<br>
- --BBI;<br>
- ++NumFastStores;<br>
- MadeChange = true;<br>
- continue;<br>
+ // If we find a store to memory which was defined by
calloc<br>
+ // we can remove the store if the value being stored
is a<br>
+ // constant zero (since calloc initialized the memory
to<br>
+ // that same value) or the store is undefined (if out
of<br>
+ // bounds).<br>
+ if (isCallocLikeFn(InstDep.getInst(), TLI) &&
isRemovable(SI)) {<br>
+ Value *V = SI->getValueOperand();<br>
+ if (isa<Constant>(V) &&
cast<Constant>(V)->isNullValue()) {<br>
+ DEBUG(dbgs() << "DSE: Remove Store Of Zero
to Calloc:\n "<br>
+ << "CALLOC: " <<
*InstDep.getInst() << "\n"<br>
+ << "STORE: " << *SI <<
'\n');<br>
+ DeleteStore = true;<br>
}<br>
}<br>
+<br>
+ if (DeleteStore) {<br>
+ // DeleteDeadInstruction can delete the current
instruction. Save BBI<br>
+ // in case we need it.<br>
+ WeakVH NextInst(BBI);<br>
+<br>
+ DeleteDeadInstruction(SI, *MD, TLI);<br>
+<br>
+ if (!NextInst) // Next instruction deleted.<br>
+ BBI = BB.begin();<br>
+ else if (BBI != BB.begin()) // Revisit this
instruction if possible.<br>
+ --BBI;<br>
+ ++NumFastStores;<br>
+ MadeChange = true;<br>
+ continue;<br>
+ }<br>
}<br>
<br>
// Figure out what location is being stored to.<br>
<br>
Added:
llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll?rev=214897&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll?rev=214897&view=auto</a><br>
==============================================================================<br>
---
llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll
(added)<br>
+++
llvm/trunk/test/Transforms/DeadStoreElimination/calloc-store.ll
Tue Aug 5 12:48:20 2014<br>
@@ -0,0 +1,17 @@<br>
+; RUN: opt < %s -basicaa -dse -S | FileCheck %s<br>
+<br>
+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"<br>
+<br>
+; Function Attrs: nounwind<br>
+declare noalias i8* @calloc(i64, i64)<br>
+<br>
+; Function Attrs: nounwind uwtable<br>
+define noalias i32* @test_store() {<br>
+; CHECK-LABEL: test_store<br>
+ %1 = tail call noalias i8* @calloc(i64 1, i64 4)<br>
+ %2 = bitcast i8* %1 to i32*<br>
+ ; This store is dead and should be removed<br>
+ store i32 0, i32* %2, align 4<br>
+; CHECK-NOT: store i32 0, i32* %2, align 4<br>
+ ret i32* %2<br>
+}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>