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

Thu Aug 27 17:09:48 PDT 2015

----- 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.

 -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