Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unchecked conversions #49

Merged
merged 1 commit into from
Sep 16, 2016
Merged

Add unchecked conversions #49

merged 1 commit into from
Sep 16, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 14, 2016

These are most useful in conjunction with pre-checking, for example if you want to return a more informative error than InexactError:

function gray(x)
    0 <= x <= 1 || throw_converterror(x, UFixed8)
    x % UFixed8
end

@noinline function throw_converterror{T}(x, ::Type{T})
    error("$x is not between $(oftype(x, typemin(T))) and $(oftype(x, typemax(T))) and cannot be converted to type $T")
end

The performance of gray is within spitting distance of just calling convert(UFixed8, x). I'm going to use this in ColorTypes, since people are perennially confused by this (e.g., JuliaGraphics/Colors.jl#252).

@@ -47,6 +47,9 @@ convert{U<:UFixed}(::Type{U}, x::Real) = _convert(U, rawtype(U), x)
_convert{U<:UFixed,T}(::Type{U}, ::Type{T}, x) = U(round(T, widen1(rawone(U))*x), 0)
_convert{U<:UFixed }(::Type{U}, ::Type{UInt128}, x) = U(round(UInt128, rawone(U)*x), 0)

rem{T<:UFixed}(x::T, ::Type{T}) = x
rem{T<:UFixed}(x::UFixed, ::Type{T}) = reinterpret(T, rem(Integer(round((rawone(T)/rawone(x))*reinterpret(x))), rawtype(T)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could use round(Integer, x) directly or reinterpret(T, round((rawone(T)/rawone(x)) * reinterpret(x)) % rawtype(T)) also since this will do floating point division with Float64 I would replace Interger with Int64.

It might be worthwhile thinking about using div(widenmul(rawone(T), reinterpret(x)), rawone(x)) if it doesn't lose to much precision.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very interesting. I looked at three "inner" implementations:

@inline iscale1{T}(::Type{T}, x) = round(UInt, (rawone(T)/rawone(x))*reinterpret(x))
@inline iscale1a{T}(::Type{T}, x) = unsafe_trunc(UInt, round((rawone(T)/rawone(x))*reinterpret(x)))
@inline function iscale2{T}(::Type{T}, x)
    d, r = divrem(widemul(rawone(T), reinterpret(x)), rawone(x))
    d + oftype(d, r >= rawone(x)>>1)
end

The fastest seems to be iscale1a. Here's its LLVM:

julia> @code_llvm iscale1a(UFixed12, one(UFixed10))

define i64 @julia_iscale1a_70902(%jl_value_t*, %UFixed.65*) #0 {
top:
  %2 = getelementptr inbounds %UFixed.65, %UFixed.65* %1, i64 0, i32 0
  %3 = load i16, i16* %2, align 2
  %4 = uitofp i16 %3 to double
  %5 = fmul double %4, 0x40100300C0300C03
  %6 = call double @llvm.rint.f64(double %5)
  %7 = fptoui double %6 to i64
  ret i64 %7
}

iscale1 looks similar except the last expression is a call, and that call turns out to involve a branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How interesting that going to FP makes this faster. It seems that divrem is killing the performance. Using only rem makes it as fast

These are most useful in conjunction with pre-checking, for example if you want to return a more informative error than InexactError
@timholy
Copy link
Member Author

timholy commented Sep 15, 2016

Wow, with the new implementation, gray is actually faster than convert(UFixed8, x), and the error message (when necessary) is infinitely more informative. I wonder if we want to consider this approach more generally (here and perhaps throughout julia).

julia> grayold(x) = UFixed8(x)
grayold (generic function with 1 method)

julia> y = rand(10^4);

julia> using BenchmarkTools

julia> gray.(y);

julia> grayold.(y);

julia> @benchmark gray.($y)
BenchmarkTools.Trial: 
  samples:          10000
  evals/sample:     1
  time tolerance:   5.00%
  memory tolerance: 1.00%
  memory estimate:  9.95 kb
  allocs estimate:  2
  minimum time:     27.08 μs (0.00% GC)
  median time:      27.20 μs (0.00% GC)
  mean time:        27.53 μs (0.00% GC)
  maximum time:     48.52 μs (0.00% GC)

julia> @benchmark grayold.($y)
BenchmarkTools.Trial: 
  samples:          10000
  evals/sample:     1
  time tolerance:   5.00%
  memory tolerance: 1.00%
  memory estimate:  9.95 kb
  allocs estimate:  2
  minimum time:     33.79 μs (0.00% GC)
  median time:      33.89 μs (0.00% GC)
  mean time:        34.12 μs (0.00% GC)
  maximum time:     67.45 μs (0.00% GC)

julia> gray(1.2)
ERROR: 1.2 is not between 0.0 and 1.0 and cannot be converted to type FixedPointNumbers.UFixed{UInt8,8}
 in throw_converterror(::Float64, ::Type{FixedPointNumbers.UFixed{UInt8,8}}) at ./REPL[3]:2
 in gray(::Float64) at ./REPL[2]:2

julia> grayold(1.2)
ERROR: InexactError()
 in trunc(::Type{UInt8}, ::Float64) at ./float.jl:458
 in _convert at /home/tim/.julia/v0.5/FixedPointNumbers/src/ufixed.jl:47 [inlined]
 in convert at /home/tim/.julia/v0.5/FixedPointNumbers/src/ufixed.jl:46 [inlined]
 in Type at /home/tim/.julia/v0.5/FixedPointNumbers/src/ufixed.jl:8 [inlined]
 in grayold(::Float64) at ./REPL[4]:1

Copy link
Collaborator

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

This looks good. LGTM

@@ -47,6 +47,9 @@ convert{U<:UFixed}(::Type{U}, x::Real) = _convert(U, rawtype(U), x)
_convert{U<:UFixed,T}(::Type{U}, ::Type{T}, x) = U(round(T, widen1(rawone(U))*x), 0)
_convert{U<:UFixed }(::Type{U}, ::Type{UInt128}, x) = U(round(UInt128, rawone(U)*x), 0)

rem{T<:UFixed}(x::T, ::Type{T}) = x
rem{T<:UFixed}(x::UFixed, ::Type{T}) = reinterpret(T, rem(unsafe_trunc(UInt, round((rawone(T)/rawone(x))*reinterpret(x))), rawtype(T)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsafe_trunc(rawtype(T), ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, shoot, just noticed this. Seems like a good idea. I will try to remember to fix this in the morning.

@timholy
Copy link
Member Author

timholy commented Sep 16, 2016

I'll merge this now, and depending on the decision on JuliaLang/julia#18521 we might switch convert someday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants