<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 1, 2017 at 6:51 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Feb 1, 2017 at 6:43 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
><br>
><br>
> On Wed, Feb 1, 2017 at 5:48 PM, Davide Italiano <<a href="mailto:davide@freebsd.org">davide@freebsd.org</a>> wrote:<br>
>><br>
>> On Wed, Feb 1, 2017 at 5:36 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Tue, Jan 31, 2017 at 5:01 PM, Davide Italiano via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> Author: davide<br>
>> >> Date: Tue Jan 31 19:01:22 2017<br>
>> >> New Revision: 293727<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=293727&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=293727&view=rev</a><br>
>> >> Log:<br>
>> >> [IPSCCP] Teach how to not propagate return values of naked functions.<br>
>> >><br>
>> >> Differential Revision: <a href="https://reviews.llvm.org/D29360" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29360</a><br>
>> >><br>
>> >> Added:<br>
>> >> llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll<br>
>> >> Modified:<br>
>> >> llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp<br>
>> >><br>
>> >> Modified: llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp?rev=293727&r1=293726&r2=293727&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/Scalar/SCCP.cpp?<wbr>rev=293727&r1=293726&r2=<wbr>293727&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================<wbr>==============================<wbr>==================<br>
>> >> --- llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp (original)<br>
>> >> +++ llvm/trunk/lib/Transforms/<wbr>Scalar/SCCP.cpp Tue Jan 31 19:01:22 2017<br>
>> >> @@ -1712,7 +1712,10 @@ static bool runIPSCCP(Module &M, const D<br>
>> >><br>
>> >> // If this is an exact definition of this function, then we can<br>
>> >> propagate<br>
>> >> // information about its result into callsites of it.<br>
>> >> - if (F.hasExactDefinition())<br>
>> >> + // Don't touch naked functions. They may contain asm returning a<br>
>> >> + // value we don't see, so we may end up interprocedurally<br>
>> >> propagating<br>
>> >> + // the return value incorrectly.<br>
>> >> + if (F.hasExactDefinition() && !F.hasFnAttribute(Attribute::<wbr>Naked))<br>
>> >> Solver.AddTrackedFunction(&F);<br>
>> >><br>
>> >> // If this function only has direct calls that we can see, we can<br>
>> >> track its<br>
>> >><br>
>> >> Added: llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll?rev=293727&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/IPConstantProp/<wbr>naked-return.ll?rev=293727&<wbr>view=auto</a><br>
>> >><br>
>> >><br>
>> >> ==============================<wbr>==============================<wbr>==================<br>
>> >> --- llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll (added)<br>
>> >> +++ llvm/trunk/test/Transforms/<wbr>IPConstantProp/naked-return.ll Tue Jan<br>
>> >> 31<br>
>> >> 19:01:22 2017<br>
>> >> @@ -0,0 +1,28 @@<br>
>> >> +; RUN: opt -ipsccp -S %s | FileCheck %s<br>
>> >> +<br>
>> >> +target datalayout = "e-m:x-p:32:32-i64:64-f80:32-<wbr>n8:16:32-a:0:32-S32"<br>
>> >> +target triple = "i686-pc-windows-msvc19.0.<wbr>24215"<br>
>> >> +<br>
>> >> +define i32 @dipsy(i32, i32) local_unnamed_addr #0 {<br>
>> >> +BasicBlock0:<br>
>> >> + call void asm "\0D\0Apushl %ebp\0D\0Amovl 8(%esp),%eax\0D\0Amovl<br>
>> >> 12(%esp), %ebp\0D\0Acalll *%eax\0D\0Apopl %ebp\0D\0Aretl\0D\0A", ""()<br>
>> >> + ret i32 0<br>
>> ><br>
>> ><br>
>> > If this asm is going to return, shouldn't we have `unreachable` instead<br>
>> > of<br>
>> > `ret i32 0`? That would naturally be correct I think without requiring<br>
>> > special handling for naked functions. I.e., the verifier should ensure<br>
>> > that<br>
>> > naked functions always end with `unreachable`.<br>
>> ><br>
>><br>
>> That would require some other code to make sure we don't propagate the<br>
>> unreachable to the caller (which apparently PruneEH currently does). WDYT?<br>
><br>
><br>
> It sounds like the missing thing is (at least) being able to mark an inline<br>
> asm as returning (hard to do without making it a terminator?). More<br>
> generally, marking inline asm's as affecting control flow is also a<br>
> longstanding issue preventing us from implementing "asm goto". So this might<br>
> be pretty nontrivial.<br>
> Maybe we can model it as the inline asm having a dummy out param which is<br>
> the thing that is returned? (or marking the inline asm such that the<br>
> optimizer thinks it might throw an exception, do a longjmp, or any other<br>
> side effect that will conservatively model the asm's behavior of returning<br>
> from the current function)<br>
><br>
<br>
</div></div>It generally lacks 'being able to reason about inline asm'. As far as<br>
I can tell transformations consider asm in IR as an opaque string, and<br>
this causes problems in many cases.<br></blockquote><div><br></div><div>We have "volatile" (whatever we turn C/C++ `asm volatile` into). Also the in-out params of the asm.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> It just seems wrong for the special case check to be for naked functions<br>
> since the issue seems to be more about modeling what an inline asm will do.<br>
><br>
><br>
<br>
</span>I agree on the last bit (it seems this issue is more about modeling<br>
what inline asm should do). Also, please note a slight modification of<br>
this problem in DeadArgElim (where we force functions to be marked<br>
live because we can't make assumptions of what the asm might use).<br>
<div class="HOEnZb"><div class="h5"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</div></div></blockquote></div><br></div></div>