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

[Hal Finkel via llvm-commits]

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

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