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

don't inline float64^float64 #42966

Merged

Conversation

oscardssmith
Copy link
Member

It was leading to too much code bloat. This code is slow enough that no noticeable slowdown is expected.

@oscardssmith oscardssmith added the maths Mathematical functions label Nov 5, 2021
@Keno
Copy link
Member

Keno commented Nov 5, 2021

Seems like there's similar issues for the other methods of ^ below. Presumably we should @inline from them all?

Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

Can confirm that this fixes the observed compile time blow ups.

@oscardssmith
Copy link
Member Author

the float16 and float64 methods can be inlined since exp2 and log2 don't inline. float64 to integer is the other that I'm unsure about.

@KristofferC
Copy link
Member

KristofferC commented Nov 6, 2021

I also think all of these forced inlines should be removed. There no way that is beneficial on the whole. You can always call site inline then if you really need.

@oscardssmith
Copy link
Member Author

Any ideas how removing the inlining is causing tests to fail? That seems really weird to me.

@aviatesk
Copy link
Member

aviatesk commented Nov 8, 2021

Inlineability is used as a constant-propagation heuristic. So I guess the same precision problem as reported in #41450 happens here as well ?

@oscardssmith
Copy link
Member Author

theoretically, this should fix test failures and make Float64^-Int more accurate.

@oscardssmith oscardssmith reopened this Nov 8, 2021
@KristofferC KristofferC requested a review from stevengj November 8, 2021 15:11
@KristofferC
Copy link
Member

@stevengj can you look at the literal_pow stuff, since you have a lot of historical context there.

Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
@KristofferC
Copy link
Member

KristofferC commented Nov 8, 2021

Remember the other @inlines. Many of them are on very big function bodies like @inline function ^(x::Float64, n::Integer).

base/intfuncs.jl Outdated
@@ -328,7 +331,11 @@ const HWNumber = Union{HWReal, Complex{<:HWReal}, Rational{<:HWReal}}
# be computed in a type-stable way even for e.g. integers.
@inline function literal_pow(f::typeof(^), x, ::Val{p}) where {p}
if p < 0
literal_pow(^, inv(x), Val(-p))
if x isa BitInteger64
f(Float64(x), p)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add some comment on this? Is this to improve performance, accuracy, or both?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. The reasoning is that computing inv(x)^(-p) rounds twice. Once to compute inv(x) and once to compute the power. Since the Float64^Float64 code works to half an ULP (even for negative numbers), we want to let that code handle the negative powers since it does so better.

@oscardssmith
Copy link
Member Author

I believe this is finally working correctly. This was a shockingly annoying change for something that should have been trivial.

@oscardssmith
Copy link
Member Author

I'm going to merge this tonight if there aren't objections.

@oscardssmith oscardssmith merged commit bbf762d into JuliaLang:master Nov 10, 2021
@oscardssmith oscardssmith deleted the remove-float64float64-inline branch November 10, 2021 01:31
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants