Widen small integers only to Int32, not Int #14414
Conversation
8caab27
to
a003504
Compare
Reasons, as described in JuliaLang#14407: - 32-bit integer division is significantly faster than 64-bit integer division. - Consistency across platforms (widen(Int(8)) will do the same thing on 32 & 64 bit) - Smaller integers can have larger gains from vectorization Changes: - Widen small integers only to Int32, not Int - Ensure that `widen` tests actually check the result type, not just the value. - Update tests. - No documentation update necessary since the actual return type is left unspecified. Closes JuliaLang#14407.
a003504
to
29fe93a
Compare
|
LGTM 👍 |
|
Makes sense to me. cc @JeffBezanson |
|
Yes, this does seem like a good idea. |
|
How many places does |
|
@tkelman I think |
|
This might break FixedPointNumbers. I can test this over the weekend. cc: @timholy |
|
It's used in the rational code, via |
| @@ -339,10 +339,10 @@ typemax(::Type{UInt64}) = 0xffffffffffffffff | |||
| @eval typemin(::Type{Int128} ) = $(convert(Int128,1)<<127) | |||
| @eval typemax(::Type{Int128} ) = $(box(Int128,unbox(UInt128,typemax(UInt128)>>1))) | |||
|
|
|||
| widen{T<:Union{Int8, Int16}}(::Type{T}) = Int | |||
| widen{T<:Union{Int8, Int16}}(::Type{T}) = Int32 | |||
There was a problem hiding this comment.
Why not promote Int8 to Int16?
There was a problem hiding this comment.
Yes, that would have been my first choice as well. However, (a) current behaviour widens to larger integer types, and (b) 16-bit operations are sometimes slower than 32-bit operations (on modern Intel CPUs).
On the other hand, if we go for auto-vectorization, then keeping the type as small as possible would be beneficial.
|
Yes, this seems reasonable to me. |
|
@eschnett, make a call on whether you think the smaller integer types should widen to |
|
Oh, and once you've made a call, feel free to merge. |
|
@StefanKarpinski I think widening to 32 bits instead of Int is a good idea. |
Widen small integers only to Int32, not Int
Reasons, as described in #14407:
Changes:
widentests actually check the result type, not just the value.Closes #14407.