Implement promotion among UFixed, make convert errors more informative#53
Implement promotion among UFixed, make convert errors more informative#53
Conversation
|
Closing and reopening to see if AppVeyor triggers. |
timholy
force-pushed
the
teh/convert_promote
branch
from
9d2bdf3 to
59331a8
Compare
| U(_unsafe_trunc(T, y), 0) | ||
| end | ||
| function _convert{U<:UFixed}(::Type{U}, ::Type{UInt128}, x) | ||
| y = round(rawone(U)*x) # for UInt128, we can't widen |
There was a problem hiding this comment.
If we definedwiden1(::Type{UInt128}) = UInt128 we wouldn't need to duplicate the code path and we would be forward compatible for the glorious day when we have UInt256.
There was a problem hiding this comment.
I thought about that. The flip side is that if one is counting on widen1 to actually protect you, then you'll be mistaken.
But perhaps that's at too low of a level to matter, and that the only issue that matters is whether convert successfully catches the problem. Which refusing to defining widen1 doesn't actually help with. I will change it.
There was a problem hiding this comment.
OK, we had a number of problems with high-precision UFixed types.
timholy
force-pushed
the
teh/convert_promote
branch
2 times, most recently
from
ef03354 to
a399254
Compare
| convert{T1<:UFixed}(::Type{T1}, x::UFixed) = reinterpret(T1, round(rawtype(T1), (rawone(T1)/rawone(x))*reinterpret(x))) | ||
| function convert{T<:UFixed}(::Type{T}, x::UFixed) | ||
| y = round((rawone(T)/rawone(x))*reinterpret(x)) | ||
| (0 <= y) & (y <= typemax(rawtype(T))) || throw_converterror(T, x) |
There was a problem hiding this comment.
why not just say (0 <= y <= typemax(rawtype(T))) || ... ?
There was a problem hiding this comment.
Performance. Currently a <= x <= b gets lowered to (a <= x) && (x <= b) rather than (a <= x) & (x <= b), which means there are two branches rather than one.
This is something that should probably be changed in Julia, but neither I nor anyone else has gotten to it yet.
timholy
force-pushed
the
teh/convert_promote
branch
3 times, most recently
from
39ef3f5 to
18615f8
Compare
timholy
force-pushed
the
teh/convert_promote
branch
2 times, most recently
from
a0628e5 to
526a7b9
Compare
| f = 2^nbitsfrac(T)-1 | ||
| :( T($f,0) ) | ||
| function one{T<:UFixed}(::Type{T}) | ||
| T(typemax(rawtype(T)) >> (8*sizeof(T)-nbitsfrac(T)), 0) |
There was a problem hiding this comment.
the generated version might be faster. see #44 (comment). or does the use of >> instead of ^ close the gap?
There was a problem hiding this comment.
LLVM inlines this new version to a constant:
julia> @code_llvm one(UFixed{UInt64, 51})
define %UFixed @julia_one_70810(%jl_value_t*) #0 {
top:
ret %UFixed { i64 2251799813685247 }
}The bigger issue was that for UInt128 (or even for UInt64 on 32-bit machines), 2^f was returning 0 for f bigger than the WORD_SIZE. This version is guaranteed to be safe no matter what kind of machine or what kind of precision we're using.
3 participants
On the master branch, we have the following bug:
We also have this uninformative message:
This PR fixes the bug by implementing
promote_typeamongUFixedtypes. It also produces the more verbose error message:Assuming people like this, I think it should be merged before #51, and we can tag a new version before making a more incompatible change.