-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Make various core math functions easier for the compiler to reason about #43907
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
Conversation
xs = xu & ~sign_mask(T) | ||
xs >= exponent_mask(T) && return x # NaN or Inf | ||
k = Int(xs >> significand_bits(T)) | ||
k = (xs >> significand_bits(T)) % Int |
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.
What's the difference between these?
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.
Int
throws if the sign bit is set and inference does not have any range modeling to know that that is impossible.
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.
Ah. That's somewhat unfortunate, but this is a good fix then.
# internal use only. Could be written as | ||
# @assume_effects :nothrow exponent() | ||
# but currently this form is easier on the compiler. | ||
function _exponent_finite_nonzero(x::T) where T<:IEEEFloat |
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.
This deserves a better name.
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.
It's exponent
, but only for for finite and nonzero values. Seems perfectly descriptive to me.
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.
understood, but given that it's something that I will probably want to use, I think something like nothrow_exponent
with documentation as to when it gives wrong answers would be better.
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.
Since we don't have any precondition enforcement, I do like having the precondition in the name, so it's obvious to people reading it at the callsite what is being asserted here.
Can you post the performance differences? |
It lets the compiler reason about them and delete them if unused (and nothrow), e.g.: master:
PR + #43899:
|
Note that this is quite fancy, because the non-throwness is constant dependent:
Because
|
Why is |
Bit of a display artifact. The type isn't stored if the statement is unused. |
We have an inference loop fma_emulated -> ldexp -> ^(::Float64, ::Int) -> fma -> fma_emulated. The arguments to `^` are constant, so constprop will figure it out, but it does require a bunch of extra processing. There is a simpler way to write this using elementary bit operations. Since resolving the inference loop requires constprop, this was breaking #43852. That is fixable, but I think we should also make this change to avoid having an unnecessary inference loop in our basic math functions, which will make future analyses easier.
The fact that the `exponent` call in `fma_emulated` requires reasoning about the ranges of the floating point values in question, which the compiler is not capable of doing (and is unlikely to ever do automatically). Thus, in order for the compiler to know that `fma_emulated` (and by extension `fma`) is :nothrow in a post-#43852 world, create a separate version of the `exponent` function that assumes its precondition. We could use `@assume_effects` instead, but this version is currently slightly easier on the compiler.
The integer branch is nothrow, so if the caller does something like `^(x::Float64, 2.0)`, we'd like to discover that.
return pow_body(x, y) | ||
end | ||
|
||
@inline function pow_body(x::Float64, y::Float64) |
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.
Perhaps worth putting this inside the function, so it is clearer the purpose of it?
return pow_body(x, y) | |
end | |
@inline function pow_body(x::Float64, y::Float64) | |
return (@inline function pow_body(x::Float64, y::Float64) ... end)(x, y) |
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.
SGTM
Should have been part of #43907, but I forgot.
Should have been part of #43907, but I forgot.
…out (JuliaLang#43907) * ldexp: Break inference loop We have an inference loop fma_emulated -> ldexp -> ^(::Float64, ::Int) -> fma -> fma_emulated. The arguments to `^` are constant, so constprop will figure it out, but it does require a bunch of extra processing. There is a simpler way to write this using elementary bit operations. Since resolving the inference loop requires constprop, this was breaking JuliaLang#43852. That is fixable, but I think we should also make this change to avoid having an unnecessary inference loop in our basic math functions, which will make future analyses easier. * Make fma_emulated easier for the compiler to reason about The fact that the `exponent` call in `fma_emulated` requires reasoning about the ranges of the floating point values in question, which the compiler is not capable of doing (and is unlikely to ever do automatically). Thus, in order for the compiler to know that `fma_emulated` (and by extension `fma`) is :nothrow in a post-JuliaLang#43852 world, create a separate version of the `exponent` function that assumes its precondition. We could use `@assume_effects` instead, but this version is currently slightly easier on the compiler. * pow: Make integer vs float branch obvious to constprop The integer branch is nothrow, so if the caller does something like `^(x::Float64, 2.0)`, we'd like to discover that.
Should have been part of JuliaLang#43907, but I forgot.
…out (JuliaLang#43907) * ldexp: Break inference loop We have an inference loop fma_emulated -> ldexp -> ^(::Float64, ::Int) -> fma -> fma_emulated. The arguments to `^` are constant, so constprop will figure it out, but it does require a bunch of extra processing. There is a simpler way to write this using elementary bit operations. Since resolving the inference loop requires constprop, this was breaking JuliaLang#43852. That is fixable, but I think we should also make this change to avoid having an unnecessary inference loop in our basic math functions, which will make future analyses easier. * Make fma_emulated easier for the compiler to reason about The fact that the `exponent` call in `fma_emulated` requires reasoning about the ranges of the floating point values in question, which the compiler is not capable of doing (and is unlikely to ever do automatically). Thus, in order for the compiler to know that `fma_emulated` (and by extension `fma`) is :nothrow in a post-JuliaLang#43852 world, create a separate version of the `exponent` function that assumes its precondition. We could use `@assume_effects` instead, but this version is currently slightly easier on the compiler. * pow: Make integer vs float branch obvious to constprop The integer branch is nothrow, so if the caller does something like `^(x::Float64, 2.0)`, we'd like to discover that.
Should have been part of JuliaLang#43907, but I forgot.
Details are in the commit messages, but the overall goal here is for a post-#43852 compiler to be able to figure out that all of these are
:effect_free
and:terminates
as well as:nothrow
where appropriate. This is achieved by moving the code around a bit and avoiding inference loops. The primary benefit of this is not seen until #43852 is merged, but the change is independent and will be marginally better on the current compiler as well.