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

Thu Aug 27 19:40:29 PDT 2015

On Thu, Aug 27, 2015 at 5:37 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 7:16:40 PM
> > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts are
> fair game for speculation
> >
> >
> > 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?
> >
>
> I believe that it did not count prior to r246232 :-)
>
> 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.
>

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?

>
> Thanks again,
> Hal
>
> >
> >
> > -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
> >
> >
>
> --
> 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/3c8ca056/attachment.html>