<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 26, 2016 at 3:00 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br><hr><blockquote style="border-left-width:2px;border-left-style:solid;border-left-color: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" <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>><br><b>To: </b>"Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br><b>Cc: </b>"llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>, "Eli Friedman" <<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>><br><b>Sent: </b>Thursday, May 26, 2016 4:52:48 PM<div><div class="h5"><br><b>Subject: </b>Re: [llvm] r270892 - [MemCpyOpt] Don't perform callslot optimization across may-throw calls<br><br><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:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br><br><hr><blockquote style="border-left-width:2px;border-left-style:solid;border-left-color: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><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-width:1px;border-left-style:solid;border-left-color: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-width:1px;border-left-style:solid;border-left-color: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></div></div></div></div></blockquote><br>Yes (although for local variables, I think they need to be volatile).<br></div></div></blockquote><div><br></div><div>The wording in the standard is a little more specific than that. It says that any modification that happens between the setjmp and longjmp can only be observed using volatile. After the longjmp, has jumped back to the setjmp you are not obligated to keep using volatile.</div><div><br></div><div>C11 <a href="http://7.13.2.1">7.13.2.1</a>:<br><div>All accessible objects have values, and all other components of the abstract machine have state, as of the time the longjmp function was called, except that the values of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call are indeterminate.</div></div><div><br></div><div>In the above "program", 'y' could be set to a non-zero value if we have never performed a 'longjmp'. We then 'longjmp' back to the 'setjmp', store zero into 'y' and dutifully skip over the 'longjmp' on our second way through. We would then execute the store of 9 to 'x' which, by my reading of the spec, should not require any volatile annotation whatsoever.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br>More broadly, however, we don't want to mark all functions that might call longjmp as mayThrow(), because that would essentially be all external functions?</div></div></blockquote><div><br></div><div>It depends. How well do we want to model the semantics of setjmp/longjmp? FWIW there is precedence for using unwinding to model this in LLVM, we currently model pthread cancelation semantics via unwind (e.g. open may throw...)</div><div><br></div><div>I think that if we as a community really care about setjmp/longjmp, we should model them better. If we feel like our current model is sufficient, then that's fine too.</div><div>Personally, I have no dog in this fight.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><span class=""><font color="#888888"><br><br> -Hal</font></span><span class=""><br><blockquote style="border-left-width:2px;border-left-style:solid;border-left-color: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><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br> -Hal<br><blockquote style="border-left-width:2px;border-left-style:solid;border-left-color: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-width:1px;border-left-style:solid;border-left-color: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>
<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><font color="#888888"><br><br><br>-- <br><div><span></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span></span><br></div></font></span></div></div></blockquote></div><br></div></div>
</blockquote><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></span></div></div></blockquote></div><br></div></div>