-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
only use accurate powf function #19890
Changes from all commits
5a1f971
1c5d733
cc7990b
0c5eac2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -56,8 +56,8 @@ MINEXP(::Type{Float32}) = -103.97207708f0 # log 2^-150 | |||||||||
@inline exp_kernel(x::Float32) = @horner(x, 1.6666625440f-1, -2.7667332906f-3) | ||||||||||
|
||||||||||
# for values smaller than this threshold just use a Taylor expansion | ||||||||||
exp_small_thres(::Type{Float64}) = 2.0^-28 | ||||||||||
exp_small_thres(::Type{Float32}) = 2.0f0^-13 | ||||||||||
@eval exp_small_thres(::Type{Float64}) = $(2.0^-28) | ||||||||||
@eval exp_small_thres(::Type{Float32}) = $(2.0f0^-13) | ||||||||||
|
||||||||||
""" | ||||||||||
exp(x) | ||||||||||
|
@@ -114,13 +114,13 @@ function exp{T<:Union{Float32,Float64}}(x::T) | |||||||||
# scale back | ||||||||||
if k > -significand_bits(T) | ||||||||||
# multiply by 2.0 first to prevent overflow, which helps extends the range | ||||||||||
k == exponent_max(T) && return y*T(2.0)*T(2.0)^(exponent_max(T) - 1) | ||||||||||
k == exponent_max(T) && return y * T(2.0) * T(2.0)^(exponent_max(T) - 1) | ||||||||||
twopk = reinterpret(T, rem(exponent_bias(T) + k, fpinttype(T)) << significand_bits(T)) | ||||||||||
return y*twopk | ||||||||||
else | ||||||||||
# add significand_bits(T) + 1 to lift the range outside the subnormals | ||||||||||
twopk = reinterpret(T, rem(exponent_bias(T) + significand_bits(T) + 1 + k, fpinttype(T)) << significand_bits(T)) | ||||||||||
return y*twopk*T(2.0)^(-significand_bits(T) - 1) | ||||||||||
return y * twopk * T(2.0)^(-significand_bits(T) - 1) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this also be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better as is, since the intent of the code is a lot more clear with it being explicit same with but if they must change it would also make sense to change both I have to admit it's a little disappointing that we have to now work around these cases as such There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All of these options generate exactly the same result. Just trying to understand what should be in the code for the most clarity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @musm I'm still not sure I quite understand the issue here. All these expressions are still being constant folded. e.g. on this branch:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (so this means 1282205 is unnecessary, and can be removed). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that commit seems like an improvement in readability to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm a little confused then why we need: Line 121 in 1282205
and Line 127 in 1282205
should also be constant folded (i.e. T(2.0)^(exponent_max(T) - 1) and T(2.0)^(-significand_bits(T) - 1) ).
So doesn't that make the Line 751 in 1282205
The reason I suggested the current code is more clear is that it's really obvious what's currently happening with the This type of scaling is also used in almost the exact same logic, repeatedly, in the Line 118 in 1282205
I'd prefer to keep as it as is currently, if possible, due to these reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The benchmark is for an older commit which had a bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, then we should get rid of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any way to add a test more visible than the perf tracking that the constant propagation is working reliably? |
||||||||||
end | ||||||||||
elseif xa < reinterpret(Unsigned, exp_small_thres(T)) # |x| < exp_small_thres | ||||||||||
# Taylor approximation for small x | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vtjnash Out of curiosity, what was the reason behind moving these functions from
float.jl
tomath.jl
? I had some code that importedBase.significand_bits
, and now it needsBase.Math.significand_bits
to work on master.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were incorrectly marked
@pure
, and only used here. You can import them fromBase.Math
on old code also, to be compatible.