[llvm] r246232 - [ValueTracking] readnone CallInsts are fair game for speculation

Thu Aug 27 17:16:40 PDT 2015

On Thu, Aug 27, 2015 at 5:09 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "David Majnemer" <david.majnemer at gmail.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> > Sent: Thursday, August 27, 2015 6:57:32 PM
> > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts are
> fair game for speculation
> >
> > On Thu, Aug 27, 2015 at 4:20 PM, Hal Finkel < hfinkel at anl.gov >
> > wrote:
> >
> >
> > Hi David,
> >
> > I'd really like this to be a correct change, but I'm not sure that it
> > is. I'd like it to be correct because we add readnone for functions
> > that, at the C level, have __attribute__((const)), and I believe
> > those can be speculated (assuming my understanding of GCC's
> > semantics is correct). However, in terms of readnone itself, we need
> > to be careful about undefined behavior. Specifically,
> >
> > int divide(int x, int y) { return x/y; }
> >
> > is readnone, according to our definition, but could trap if
> > speculated with y == 0.
> >
> > Thanks again,
> > Hal
> >
> > Would it be correct for a function which non-deterministically traps
> > (but which does not read or write memory) to be marked readnone?
>
> I believe this is correct, as the IR is currently defined. Plus
> FunctionAttrs will happily do this today (that divide function is marked
> readnone).
>
> > If
> > so, I think our optimizer is in quite a bit of trouble.
>
> :-)
>
> >
> > I'll also note that we go out of our way not to mark things like
> > rdrand/rdseed as readnone despite them not reading any memory. I'd
> > prefer that readnone should imply that speculation, CSE, etc. are
> > safe. Would it be problematic to let 'divide' not be readnone?
> >
>
> 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.
>

I'm not at all opposed to this resolution.  However, I am bit confused.
The langref states:
"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."

It seems that readnone functions imply nounwind and "never changes any
state visible to callers".  Would trapping count?

>
>  -Hal
>
> >
> > ----- Original Message -----
> > > From: "David Majnemer via llvm-commits" <
> > > llvm-commits at lists.llvm.org >
> > > To: llvm-commits at lists.llvm.org
> > > Sent: Thursday, August 27, 2015 6:03:01 PM
> > > Subject: [llvm] r246232 - [ValueTracking] readnone CallInsts are
> > > fair game for speculation
> > >
> > > Author: majnemer
> > > Date: Thu Aug 27 18:03:01 2015
> > > New Revision: 246232
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=246232&view=rev
> > > Log:
> > > [ValueTracking] readnone CallInsts are fair game for speculation
> > >
> > > Any call which is side effect free is trivially OK to speculate. We
> > > already had similar logic in EarlyCSE and GVN but we were missing
> > > it
> > > from isSafeToSpeculativelyExecute.
> > >
> > > This fixes PR24601.
> > >
> > > Modified:
> > > llvm/trunk/lib/Analysis/ValueTracking.cpp
> > > llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll
> > >
> > > Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
> > > URL:
> > >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=246232&r1=246231&r2=246232&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> > > +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Thu Aug 27 18:03:01
> > > 2015
> > > @@ -3147,46 +3147,10 @@ bool llvm::isSafeToSpeculativelyExecute(
> > > LI->getPointerOperand(), LI->getAlignment(), DL, CtxI, DT,
> > > TLI);
> > > }
> > > case Instruction::Call: {
> > > - if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
> > > - switch (II->getIntrinsicID()) {
> > > - // These synthetic intrinsics have no side-effects and just
> > > mark
> > > - // information about their operands.
> > > - // FIXME: There are other no-op synthetic instructions that
> > > potentially
> > > - // should be considered at least *safe* to speculate...
> > > - case Intrinsic::dbg_declare:
> > > - case Intrinsic::dbg_value:
> > > - return true;
> > > -
> > > - case Intrinsic::bswap:
> > > - case Intrinsic::ctlz:
> > > - case Intrinsic::ctpop:
> > > - case Intrinsic::cttz:
> > > - case Intrinsic::objectsize:
> > > - case Intrinsic::sadd_with_overflow:
> > > - case Intrinsic::smul_with_overflow:
> > > - case Intrinsic::ssub_with_overflow:
> > > - case Intrinsic::uadd_with_overflow:
> > > - case Intrinsic::umul_with_overflow:
> > > - case Intrinsic::usub_with_overflow:
> > > - return true;
> > > - // Sqrt should be OK, since the llvm sqrt intrinsic isn't
> > > defined to set
> > > - // errno like libm sqrt would.
> > > - case Intrinsic::sqrt:
> > > - case Intrinsic::fma:
> > > - case Intrinsic::fmuladd:
> > > - case Intrinsic::fabs:
> > > - case Intrinsic::minnum:
> > > - case Intrinsic::maxnum:
> > > - return true;
> > > - // TODO: some fp intrinsics are marked as having the same
> > > error handling
> > > - // as libm. They're safe to speculate when they won't error.
> > > - // TODO: are convert_{from,to}_fp16 safe?
> > > - // TODO: can we list target-specific intrinsics here?
> > > - default: break;
> > > - }
> > > - }
> > > + if (cast<CallInst>(Inst)->doesNotAccessMemory())
> > > + return true;
> > > return false; // The called function could have undefined
> > > behavior or
> > > - // side-effects, even if marked readnone nounwind.
> > > + // side-effects.
> > > }
> > > case Instruction::VAArg:
> > > case Instruction::Alloca:
> > >
> > > Modified: llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll
> > > URL:
> > >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll?rev=246232&r1=246231&r2=246232&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll
> > > (original)
> > > +++ llvm/trunk/test/Transforms/SimplifyCFG/speculate-math.ll Thu
> > > Aug
> > > 27 18:03:01 2015
> > > @@ -6,6 +6,7 @@ declare float @llvm.fmuladd.f32(float, f
> > > declare float @llvm.fabs.f32(float) nounwind readonly
> > > declare float @llvm.minnum.f32(float, float) nounwind readonly
> > > declare float @llvm.maxnum.f32(float, float) nounwind readonly
> > > +declare float @llvm.copysign.f32(float, float) nounwind readonly
> > >
> > > ; CHECK-LABEL: @sqrt_test(
> > > ; CHECK: select
> > > @@ -107,4 +108,21 @@ test_maxnum.exit:
> > > %cond.i = phi float [ %0, %cond.else.i ], [ 0x7FF8000000000000,
> > > %entry ]
> > > store float %cond.i, float addrspace(1)* %out, align 4
> > > ret void
> > > +}
> > > +
> > > +; CHECK-LABEL: @copysign_test(
> > > +; CHECK: select
> > > +define void @copysign_test(float addrspace(1)* noalias nocapture
> > > %out, float %a, float %b) nounwind {
> > > +entry:
> > > + %cmp.i = fcmp olt float %a, 0.000000e+00
> > > + br i1 %cmp.i, label %test_copysign.exit, label %cond.else.i
> > > +
> > > +cond.else.i: ; preds = %entry
> > > + %0 = tail call float @llvm.copysign.f32(float %a, float %b)
> > > nounwind readnone
> > > + br label %test_copysign.exit
> > > +
> > > +test_copysign.exit: ; preds =
> > > %cond.else.i, %entry
> > > + %cond.i = phi float [ %0, %cond.else.i ], [ 0x7FF8000000000000,
> > > %entry ]
> > > + store float %cond.i, float addrspace(1)* %out, align 4
> > > + ret void
> > > }
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > >
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> >
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150827/76230afc/attachment.html>