<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 26, 2016 at 1:57 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br><br><hr><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>From: </b>"David Majnemer via llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br><b>To: </b>"Eli Friedman" <<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>><br><b>Cc: </b>"llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br><b>Sent: </b>Thursday, May 26, 2016 3:33:13 PM<br><b>Subject: </b>Re: [llvm] r270892 - [MemCpyOpt] Don't perform callslot optimization across may-throw calls<div><div class="h5"><br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 26, 2016 at 1:21 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div>On Thu, May 26, 2016 at 12:24 PM, David Majnemer via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br></div></div><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: majnemer<br>
Date: Thu May 26 14:24:24 2016<br>
New Revision: 270892<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=270892&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=270892&view=rev</a><br>
Log:<br>
[MemCpyOpt] Don't perform callslot optimization across may-throw calls<br>
<br>
An exception could prevent a store from occurring but MemCpyOpt's<br>
callslot optimization would fire anyway, causing the store to occur.<br>
<br>
This fixes PR27849.<br>
<br>
Added:<br>
llvm/trunk/test/Transforms/MemCpyOpt/callslot_throw.ll<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
llvm/trunk/test/Transforms/MemCpyOpt/loadstore-sret.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=270892&r1=270891&r2=270892&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=270892&r1=270891&r2=270892&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Thu May 26 14:24:24 2016<br>
@@ -496,7 +496,7 @@ static unsigned findCommonAlignment(cons<br>
<br>
// This method try to lift a store instruction before position P.<br>
// It will lift the store and its argument + that anything that<br>
-// lay alias with these.<br>
+// may alias with these.<br>
// The method returns true if it was successful.<br>
static bool moveUp(AliasAnalysis &AA, StoreInst *SI, Instruction *P) {<br>
// If the store alias this position, early bail out.<br>
@@ -675,6 +675,8 @@ bool MemCpyOpt::processStore(StoreInst *<br>
if (C) {<br>
// Check that nothing touches the dest of the "copy" between<br>
// the call and the store.<br>
+ Value *CpyDest = SI->getPointerOperand()->stripPointerCasts();<br>
+ bool CpyDestIsLocal = isa<AllocaInst>(CpyDest);<br>
AliasAnalysis &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();<br>
MemoryLocation StoreLoc = MemoryLocation::get(SI);<br>
for (BasicBlock::iterator I = --SI->getIterator(), E = C->getIterator();<br>
@@ -683,6 +685,12 @@ bool MemCpyOpt::processStore(StoreInst *<br>
C = nullptr;<br>
break;<br>
}<br>
+ // The store to dest may never happen if an exception can be thrown<br>
+ // between the load and the store.<br>
+ if (I->mayThrow() && !CpyDestIsLocal) {<br>
+ C = nullptr;<br>
+ break;<br>
+ }<br>
}<br>
}<br>
<br>
@@ -815,6 +823,10 @@ bool MemCpyOpt::performCallSlotOptzn(Ins<br>
if (destSize < srcSize)<br>
return false;<br>
} else if (Argument *A = dyn_cast<Argument>(cpyDest)) {<br>
+ // The store to dest may never happen if the call can throw.<br>
+ if (C->mayThrow())<br>
+ return false;<br></blockquote><div><br></div></div></div><div>I'm not sure mayThrow is sufficient; it doesn't cover longjmp. Maybe not worth worrying about, though.</div></div></div></div></blockquote><div><br></div><div>Ah, great point! IMO, we should model longjmp as a "throw" as the optimizer has to worry about it in the same ways.</div></div></div></div></div></div></blockquote>Don't we model it with some kind of does-not-return annotation?<br></div></div></blockquote><div><br></div><div>That is likely insufficient without changing what noreturn implies.</div><div><br></div><div>Calling a noreturn function implies that simple memory operations before the call to allocas are not observable.</div><div>For example:</div><div><br></div><div>int x;</div><div>int y;</div><div>...</div><div>if (y)</div><div> exit(0);</div><div>x = 9;<br></div><div><br></div><div>Here, we are free to hoist that store to 'x' above the call to 'exit' so long as it doesn't escape (because of 'atexit' handlers).</div><div><br></div><div>The following is a 'setjmp'/'longjmp' version of the above:</div><div><br></div><div>int x;</div><div>int y;</div><div>...</div><div>if (y)</div><div> longjmp(jb, 1);</div><div>x = 9;</div><div><br></div><div>Here it would not be safe to hoist the store to 'x'. This is because the 'longjmp' might take us to a place where 'x' is still live.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br> -Hal<br><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span><font color="#888888"><br><br></font></span></div><span><font color="#888888"><div>-Eli<br></div></font></span></div></div></div>
</blockquote></div><br></div></div><span class="">
<br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br></span></blockquote><span class="HOEnZb"><font color="#888888"><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></font></span></div></div></blockquote></div><br></div></div>