<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 27, 2015 at 5:09 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"><span class="">----- Original Message -----<br>
> From: "David Majnemer" <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: "llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>><br>
> Sent: Thursday, August 27, 2015 6:57:32 PM<br>
> Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts are fair game for speculation<br>
><br>
> On Thu, Aug 27, 2015 at 4:20 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> Hi David,<br>
><br>
> I'd really like this to be a correct change, but I'm not sure that it<br>
> is. I'd like it to be correct because we add readnone for functions<br>
> that, at the C level, have __attribute__((const)), and I believe<br>
> those can be speculated (assuming my understanding of GCC's<br>
> semantics is correct). However, in terms of readnone itself, we need<br>
> to be careful about undefined behavior. Specifically,<br>
><br>
> int divide(int x, int y) { return x/y; }<br>
><br>
> is readnone, according to our definition, but could trap if<br>
> speculated with y == 0.<br>
><br>
> Thanks again,<br>
> Hal<br>
><br>
> Would it be correct for a function which non-deterministically traps<br>
> (but which does not read or write memory) to be marked readnone?<br>
<br>
</span>I believe this is correct, as the IR is currently defined. Plus FunctionAttrs will happily do this today (that divide function is marked readnone).<br>
<span class=""><br>
> If<br>
> so, I think our optimizer is in quite a bit of trouble.<br>
<br>
</span>:-)<br>
<span class=""><br>
><br>
> I'll also note that we go out of our way not to mark things like<br>
> rdrand/rdseed as readnone despite them not reading any memory. I'd<br>
> prefer that readnone should imply that speculation, CSE, etc. are<br>
> safe. Would it be problematic to let 'divide' not be readnone?<br>
><br>
<br>
</span>I'd really prefer that we kept the speculation safety and the pointer aliasing properties separate, and so I don't want to mark divide as potentially accessing memory. I think that we should just add a safe_to_speculate attribute (modulo bikeshedding), and infer it by running isSafeToSpeculativelyExecute over the function in FunctionAttrs just like we do for readnone and mayWriteToMemory, etc.<br></blockquote><div><br></div><div>I'm not at all opposed to this resolution.  However, I am bit confused.  The langref states:</div><div>"It does not write through any pointer arguments (including byval arguments) and never changes any state visible to callers. This means that it cannot unwind exceptions by calling the C++ exception throwing methods."</div><div><br></div><div>It seems that readnone functions imply nounwind and "never changes any state visible to callers".  Would trapping count?</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">
<span class=""><font color="#888888"><br>
 -Hal<br>
</font></span><div class=""><div class="h5"><br>
><br>
> ----- Original Message -----<br>
> > From: "David Majnemer via llvm-commits" <<br>
> > <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a> ><br>
> > To: <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> > Sent: Thursday, August 27, 2015 6:03:01 PM<br>
> > Subject: [llvm] r246232 - [ValueTracking] readnone CallInsts are<br>
> > fair game for speculation<br>
> ><br>
> > Author: majnemer<br>
> > Date: Thu Aug 27 18:03:01 2015<br>
> > New Revision: 246232<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=246232&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=246232&view=rev</a><br>
> > Log:<br>
> > [ValueTracking] readnone CallInsts are fair game for speculation<br>
> ><br>
> > Any call which is side effect free is trivially OK to speculate. We<br>
> > already had similar logic in EarlyCSE and GVN but we were missing<br>
> > it<br>
> > from isSafeToSpeculativelyExecute.<br>
> ><br>
> > This fixes PR24601.<br>
> ><br>
> > Modified:<br>
> > llvm/trunk/lib/Analysis/ValueTracking.cpp<br>
> > llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll<br>
> ><br>
> > Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp<br>
> > URL:<br>
> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=246232&r1=246231&r2=246232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=246232&r1=246231&r2=246232&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)<br>
> > +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Thu Aug 27 18:03:01<br>
> > 2015<br>
> > @@ -3147,46 +3147,10 @@ bool llvm::isSafeToSpeculativelyExecute(<br>
> > LI->getPointerOperand(), LI->getAlignment(), DL, CtxI, DT,<br>
> > TLI);<br>
> > }<br>
> > case Instruction::Call: {<br>
> > - if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {<br>
> > - switch (II->getIntrinsicID()) {<br>
> > - // These synthetic intrinsics have no side-effects and just<br>
> > mark<br>
> > - // information about their operands.<br>
> > - // FIXME: There are other no-op synthetic instructions that<br>
> > potentially<br>
> > - // should be considered at least *safe* to speculate...<br>
> > - case Intrinsic::dbg_declare:<br>
> > - case Intrinsic::dbg_value:<br>
> > - return true;<br>
> > -<br>
> > - case Intrinsic::bswap:<br>
> > - case Intrinsic::ctlz:<br>
> > - case Intrinsic::ctpop:<br>
> > - case Intrinsic::cttz:<br>
> > - case Intrinsic::objectsize:<br>
> > - case Intrinsic::sadd_with_overflow:<br>
> > - case Intrinsic::smul_with_overflow:<br>
> > - case Intrinsic::ssub_with_overflow:<br>
> > - case Intrinsic::uadd_with_overflow:<br>
> > - case Intrinsic::umul_with_overflow:<br>
> > - case Intrinsic::usub_with_overflow:<br>
> > - return true;<br>
> > - // Sqrt should be OK, since the llvm sqrt intrinsic isn't<br>
> > defined to set<br>
> > - // errno like libm sqrt would.<br>
> > - case Intrinsic::sqrt:<br>
> > - case Intrinsic::fma:<br>
> > - case Intrinsic::fmuladd:<br>
> > - case Intrinsic::fabs:<br>
> > - case Intrinsic::minnum:<br>
> > - case Intrinsic::maxnum:<br>
> > - return true;<br>
> > - // TODO: some fp intrinsics are marked as having the same<br>
> > error handling<br>
> > - // as libm. They're safe to speculate when they won't error.<br>
> > - // TODO: are convert_{from,to}_fp16 safe?<br>
> > - // TODO: can we list target-specific intrinsics here?<br>
> > - default: break;<br>
> > - }<br>
> > - }<br>
> > + if (cast<CallInst>(Inst)->doesNotAccessMemory())<br>
> > + return true;<br>
> > return false; // The called function could have undefined<br>
> > behavior or<br>
> > - // side-effects, even if marked readnone nounwind.<br>
> > + // side-effects.<br>
> > }<br>
> > case Instruction::VAArg:<br>
> > case Instruction::Alloca:<br>
> ><br>
> > Modified: llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll<br>
> > URL:<br>
> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll?rev=246232&r1=246231&r2=246232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll?rev=246232&r1=246231&r2=246232&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll<br>
> > (original)<br>
> > +++ llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll Thu<br>
> > Aug<br>
> > 27 18:03:01 2015<br>
> > @@ -6,6 +6,7 @@ declare float @llvm.fmuladd.f32(float, f<br>
> > declare float @llvm.fabs.f32(float) nounwind readonly<br>
> > declare float @llvm.minnum.f32(float, float) nounwind readonly<br>
> > declare float @llvm.maxnum.f32(float, float) nounwind readonly<br>
> > +declare float @llvm.copysign.f32(float, float) nounwind readonly<br>
> ><br>
> > ; CHECK-LABEL: @sqrt_test(<br>
> > ; CHECK: select<br>
> > @@ -107,4 +108,21 @@ test_maxnum.exit:<br>
> > %cond.i = phi float [ %0, %cond.else.i ], [ 0x7FF8000000000000,<br>
> > %entry ]<br>
> > store float %cond.i, float addrspace(1)* %out, align 4<br>
> > ret void<br>
> > +}<br>
> > +<br>
> > +; CHECK-LABEL: @copysign_test(<br>
> > +; CHECK: select<br>
> > +define void @copysign_test(float addrspace(1)* noalias nocapture<br>
> > %out, float %a, float %b) nounwind {<br>
> > +entry:<br>
> > + %cmp.i = fcmp olt float %a, 0.000000e+00<br>
> > + br i1 %cmp.i, label %test_copysign.exit, label %cond.else.i<br>
> > +<br>
> > +cond.else.i: ; preds = %entry<br>
> > + %0 = tail call float @llvm.copysign.f32(float %a, float %b)<br>
> > nounwind readnone<br>
> > + br label %test_copysign.exit<br>
> > +<br>
> > +test_copysign.exit: ; preds =<br>
> > %cond.else.i, %entry<br>
> > + %cond.i = phi float [ %0, %cond.else.i ], [ 0x7FF8000000000000,<br>
> > %entry ]<br>
> > + store float %cond.i, float addrspace(1)* %out, align 4<br>
> > + ret void<br>
> > }<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
> ><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div></div>