Skip to content

Commit

Permalink
replace checked_fptosi intrinsics with Julia implementation (#14763)
Browse files Browse the repository at this point in the history 
Replaces checked_fptosi/checked_fptoui intrinsics with Julia implementations. Fixes #14549. Explain logic behind float->integer conversion checking

(cherry picked from commit f935a50)
  • Loading branch information
@simonbyrne @tkelman
simonbyrne authored and tkelman committed Feb 22, 2017
1 parent c378dee commit 5512553
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 107 deletions.
46 changes: 38 additions & 8 deletions base/float.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,21 +453,51 @@ prevfloat(x::AbstractFloat) = nextfloat(x,-1)

for Ti in (Int8, Int16, Int32, Int64, Int128, UInt8, UInt16, UInt32, UInt64, UInt128)
for Tf in (Float32, Float64)
if sizeof(Ti) < sizeof(Tf) || Ti <: Unsigned # Tf(typemin(Ti))-1 is exact
@eval function trunc(::Type{$Ti},x::$Tf)
$(Tf(typemin(Ti))-one(Tf)) < x < $(Tf(typemax(Ti))+one(Tf)) || throw(InexactError())
unsafe_trunc($Ti,x)
if Ti <: Unsigned || sizeof(Ti) < sizeof(Tf)
# Here `Tf(typemin(Ti))-1` is exact, so we can compare the lower-bound
# directly. `Tf(typemax(Ti))+1` is either always exactly representable, or
# rounded to `Inf` (e.g. when `Ti==UInt128 && Tf==Float32`).
@eval begin
function trunc(::Type{$Ti},x::$Tf)
if $(Tf(typemin(Ti))-one(Tf)) < x < $(Tf(typemax(Ti))+one(Tf))
return unsafe_trunc($Ti,x)
else
throw(InexactError())
end
end
function convert(::Type{$Ti}, x::$Tf)
if ($(Tf(typemin(Ti))) <= x <= $(Tf(typemax(Ti)))) && (trunc(x) == x)
return unsafe_trunc($Ti,x)
else
throw(InexactError())
end
end
end
else
@eval function trunc(::Type{$Ti},x::$Tf)
$(Tf(typemin(Ti))) <= x < $(Tf(typemax(Ti))) || throw(InexactError())
unsafe_trunc($Ti,x)
# Here `eps(Tf(typemin(Ti))) > 1`, so the only value which can be truncated to
# `Tf(typemin(Ti)` is itself. Similarly, `Tf(typemax(Ti))` is inexact and will
# be rounded up. This assumes that `Tf(typemin(Ti)) > -Inf`, which is true for
# these types, but not for `Float16` or larger integer types.
@eval begin
function trunc(::Type{$Ti},x::$Tf)
if $(Tf(typemin(Ti))) <= x < $(Tf(typemax(Ti)))
return unsafe_trunc($Ti,x)
else
throw(InexactError())
end
end
function convert(::Type{$Ti}, x::$Tf)
if ($(Tf(typemin(Ti))) <= x < $(Tf(typemax(Ti)))) && (trunc(x) == x)
return unsafe_trunc($Ti,x)
else
throw(InexactError())
end
end
end
end
end
end


@eval begin
issubnormal(x::Float32) = (abs(x) < $(box(Float32,unbox(UInt32,0x00800000)))) & (x!=0)
issubnormal(x::Float64) = (abs(x) < $(box(Float64,unbox(UInt64,0x0010000000000000)))) & (x!=0)
Expand Down
9 changes: 1 addition & 8 deletions base/int.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,14 +236,7 @@ rem{T<:Integer}(x::T, ::Type{T}) = x
rem(x::Integer, ::Type{Bool}) = ((x&1)!=0)
mod{T<:Integer}(x::Integer, ::Type{T}) = rem(x, T)

convert{Tf<:Union{Float32,Float64}}(T::BitSigned64T, x::Tf) =
box(T,checked_fptosi(T,unbox(Tf,x)))
convert{Tf<:Union{Float32,Float64}}(T::BitUnsigned64T, x::Tf) =
box(T,checked_fptoui(T,unbox(Tf,x)))

convert{Tf<:Union{Float32,Float64}}(T::Union{Type{Int128},Type{UInt128}}, x::Tf) =
(isinteger(x) || throw(InexactError()) ; trunc(T,x))

unsafe_trunc{T<:Integer}(::Type{T}, x::Integer) = rem(x, T)
for (Ts, Tu) in ((Int8, UInt8), (Int16, UInt16), (Int32, UInt32), (Int64, UInt64), (Int128, UInt128))
@eval convert(::Type{Signed}, x::$Tu) = convert($Ts, x)
@eval convert(::Type{Unsigned}, x::$Ts) = convert($Tu, x)
Expand Down
78 changes: 0 additions & 78 deletions src/intrinsics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,80 +653,6 @@ static jl_cgval_t generic_zext(jl_value_t *targ, jl_value_t *x, jl_codectx_t *ct
return mark_julia_type(ans, false, jlto, ctx);
}

static Value *emit_eqfsi(Value *x, Value *y)
{
Value *fy = JL_INT(y);

// using all 64-bit is slightly faster than using mixed sizes
Value *xx = x, *vv = fy;
if (x->getType() == T_float32)
xx = builder.CreateFPExt(xx, T_float64);
if (vv->getType()->getPrimitiveSizeInBits() < 64)
vv = builder.CreateSExt(vv, T_int64);

Value *back = builder.CreateSIToFP(vv, xx->getType());
return builder.CreateAnd
(builder.CreateFCmpOEQ(xx, back),
builder.CreateICmpEQ(vv, builder.CreateFPToSI(back, vv->getType())));
}

static Value *emit_eqfui(Value *x, Value *y)
{
Value *fy = JL_INT(y);

// using all 64-bit is slightly faster than using mixed sizes
Value *xx = x, *vv = fy;
if (x->getType() == T_float32)
xx = builder.CreateFPExt(xx, T_float64);
if (vv->getType()->getPrimitiveSizeInBits() < 64)
vv = builder.CreateZExt(vv, T_int64);

Value *back = builder.CreateUIToFP(vv, xx->getType());
return builder.CreateAnd
(builder.CreateFCmpOEQ(xx, back),
builder.CreateICmpEQ(vv, builder.CreateFPToUI(back, vv->getType())));
}

static jl_cgval_t emit_checked_fptosi(jl_value_t *targ, jl_value_t *x, jl_codectx_t *ctx)
{
jl_value_t *jlto = staticeval_bitstype(targ, "checked_fptosi", ctx);
if (!jlto) return jl_cgval_t();
Type *to = staticeval_bitstype(jlto);
Value *fx = FP(auto_unbox(x, ctx));
if (fx->getType() == T_void) return jl_cgval_t(); // auto_unbox threw an error
Value *ans = builder.CreateFPToSI(fx, to);
if (fx->getType() == T_float32 && to == T_int32) {
raise_exception_unless
(builder.CreateFCmpOEQ(builder.CreateFPExt(fx, T_float64),
builder.CreateSIToFP(ans, T_float64)),
literal_pointer_val(jl_inexact_exception), ctx);
}
else {
raise_exception_unless(emit_eqfsi(fx, ans), literal_pointer_val(jl_inexact_exception), ctx);
}
return mark_julia_type(ans, false, jlto, ctx);
}

static jl_cgval_t emit_checked_fptoui(jl_value_t *targ, jl_value_t *x, jl_codectx_t *ctx)
{
jl_value_t *jlto = staticeval_bitstype(targ, "checked_fptoui", ctx);
if (!jlto) return jl_cgval_t();
Type *to = staticeval_bitstype(jlto);
Value *fx = FP(auto_unbox(x, ctx));
if (fx->getType() == T_void) return jl_cgval_t(); // auto_unbox threw an error
Value *ans = builder.CreateFPToUI(fx, to);
if (fx->getType() == T_float32 && to == T_int32) {
raise_exception_unless
(builder.CreateFCmpOEQ(builder.CreateFPExt(fx, T_float64),
builder.CreateUIToFP(ans, T_float64)),
literal_pointer_val(jl_inexact_exception), ctx);
}
else {
raise_exception_unless(emit_eqfui(fx, ans), literal_pointer_val(jl_inexact_exception), ctx);
}
return mark_julia_type(ans, false, jlto, ctx);
}

static jl_cgval_t emit_runtime_pointerref(jl_value_t *e, jl_value_t *i, jl_value_t *align, jl_codectx_t *ctx)
{
jl_cgval_t parg = emit_expr(e, ctx);
Expand Down Expand Up @@ -989,10 +915,6 @@ static jl_cgval_t emit_intrinsic(intrinsic f, jl_value_t **args, size_t nargs,
return generic_sext(args[1], args[2], ctx);
case zext_int:
return generic_zext(args[1], args[2], ctx);
case checked_fptosi:
return emit_checked_fptosi(args[1], args[2], ctx);
case checked_fptoui:
return emit_checked_fptoui(args[1], args[2], ctx);

case uitofp: {
jl_value_t *bt = staticeval_bitstype(args[1], "uitofp", ctx);
Expand Down
2 changes: 0 additions & 2 deletions src/intrinsics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@
ADD_I(fptrunc, 2) \
ADD_I(fpext, 2) \
/* checked conversion */ \
ADD_I(checked_fptosi, 2) \
ADD_I(checked_fptoui, 2) \
ADD_I(checked_trunc_sint, 2) \
ADD_I(checked_trunc_uint, 2) \
ADD_I(check_top_bit, 1) \
Expand Down
2 changes: 0 additions & 2 deletions src/julia_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,6 @@ JL_DLLEXPORT jl_value_t *jl_fpext(jl_value_t *ty, jl_value_t *a);
JL_DLLEXPORT jl_value_t *jl_fptoui_auto(jl_value_t *a);
JL_DLLEXPORT jl_value_t *jl_fptosi_auto(jl_value_t *a);

JL_DLLEXPORT jl_value_t *jl_checked_fptoui(jl_value_t *ty, jl_value_t *a);
JL_DLLEXPORT jl_value_t *jl_checked_fptosi(jl_value_t *ty, jl_value_t *a);
JL_DLLEXPORT jl_value_t *jl_checked_trunc_sint(jl_value_t *ty, jl_value_t *a);
JL_DLLEXPORT jl_value_t *jl_checked_trunc_uint(jl_value_t *ty, jl_value_t *a);

Expand Down
9 changes: 0 additions & 9 deletions src/runtime_intrinsics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,15 +824,6 @@ static unsigned check_trunc_uint(unsigned isize, unsigned osize, void *pa)
}
cvt_iintrinsic_checked(LLVMTrunc, check_trunc_uint, checked_trunc_uint)

#define checked_fptosi(pr, a) \
if (!LLVMFPtoSI_exact(sizeof(a) * host_char_bit, pa, osize, pr)) \
jl_throw(jl_inexact_exception);
un_fintrinsic_withtype(checked_fptosi, checked_fptosi)
#define checked_fptoui(pr, a) \
if (!LLVMFPtoUI_exact(sizeof(a) * host_char_bit, pa, osize, pr)) \
jl_throw(jl_inexact_exception);
un_fintrinsic_withtype(checked_fptoui, checked_fptoui)

JL_DLLEXPORT jl_value_t *jl_check_top_bit(jl_value_t *a)
{
jl_value_t *ty = jl_typeof(a);
Expand Down
7 changes: 7 additions & 0 deletions test/numbers.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2453,6 +2453,13 @@ end
@test_throws InexactError convert(Int16, typemax(UInt64))
@test_throws InexactError convert(Int, typemax(UInt64))

# issue #14549
for T in (Int8, Int16, UInt8, UInt16)
for F in (Float32,Float64)
@test_throws InexactError convert(T, F(200000.0))
end
end

let x = big(-0.0)
@test signbit(x) && !signbit(abs(x))
end
Expand Down

5 comments on commit 5512553

@tkelman
Copy link
Contributor

@tkelman tkelman commented on 5512553 Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at commit 5512553: @nanosoldier runbenchmarks(ALL, vs = "@3c9d75391c72d7c32eea75ff187ce77b2d5effc8")

This is almost certainly what slowed "array","convert" down. @simonbyrne how much of a problem would it be if I reverted this (the backport of #14763)?

Some, but not all, of the other regressions seen at intermediate points in this PR look like they were brief regressions fixed by later commits. There are some big worrying ones in the comparison run at the last commit in the PR though.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be fixed with some @inlines?

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`make -j3`, ProcessExited(2)) [2]

Logs and partial data can be found here
cc @jrevels

@tkelman
Copy link
Contributor

@tkelman tkelman commented on 5512553 Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right I needed to fix the mentions of the intrinsics.

Dunno without trying @inlines, where in particular is worth trying?

@simonbyrne
Copy link
Contributor Author

@simonbyrne simonbyrne commented on 5512553 Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug only showed up on ARM and power, so we might be able to manage without it. I'm not sure what exactly fixed the problem.

Please sign in to comment.