<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 27, 2015 at 5:37 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"><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>
</span><div><div class="h5">> Sent: Thursday, August 27, 2015 7:16:40 PM<br>
> Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts are fair game for speculation<br>
><br>
><br>
> On Thu, Aug 27, 2015 at 5:09 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> ----- 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<br>
> > 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<br>
> > 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<br>
> > 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<br>
> > traps<br>
> > (but which does not read or write memory) to be marked readnone?<br>
><br>
> I believe this is correct, as the IR is currently defined. Plus<br>
> FunctionAttrs will happily do this today (that divide function is<br>
> marked readnone).<br>
><br>
> > If<br>
> > so, I think our optimizer is in quite a bit of trouble.<br>
><br>
> :-)<br>
><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>
> I'd really prefer that we kept the speculation safety and the pointer<br>
> aliasing properties separate, and so I don't want to mark divide as<br>
> potentially accessing memory. I think that we should just add a<br>
> safe_to_speculate attribute (modulo bikeshedding), and infer it by<br>
> running isSafeToSpeculativelyExecute over the function in<br>
> FunctionAttrs just like we do for readnone and mayWriteToMemory,<br>
> etc.<br>
><br>
><br>
><br>
> I'm not at all opposed to this resolution. However, I am bit<br>
> confused. The langref states:<br>
> "It does not write through any pointer arguments (including byval<br>
> arguments) and never changes any state visible to callers. This<br>
> means that it cannot unwind exceptions by calling the C++ exception<br>
> throwing methods."<br>
><br>
><br>
> It seems that readnone functions imply nounwind and "never changes<br>
> any state visible to callers". Would trapping count?<br>
><br>
<br>
</div></div>I believe that it did not count prior to r246232 :-)<br>
<br>
Seriously, however, I don't disagree with you. Trapping certainly seems like it could be considered a behavior visible to the caller. However, you could also argue that trapping terminates the program, and the caller can't actually observe that. This latter viewpoint seems to be the one guiding what we've currently implemented, and it seems like a relatively-useful distinction.<br></blockquote><div><br></div><div>I'm starting to split this gordian knot.  One last question: the 'nounwind' implication should be part of the new attribute, not the old one right?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks again,<br>
Hal<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> -Hal<br>
><br>
><br>
><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.<br>
> > > 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:<br>
> > > 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>
><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>