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

Fix Float32/16 raised to integer typemin #57488

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kuszmaul
Copy link

@kuszmaul kuszmaul commented Feb 21, 2025

Fixes #57464

The code for x^n where x::Float32 and n::Int previously failed for Float32(1.1)^typemin(Int) because it would reduce the problem to inv(x)^-n. That works fine unless n is typemin, in which case n==-n.

This PR makes a special case for n==typemin to effectively compute (x^(n/2))^2. Just in case n is also odd, we do x^cld(n,2) * x^fld(n,2).

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! You picked a great first issue to contribute based on.

@LilithHafner LilithHafner added bugfix This change fixes an existing bug maths Mathematical functions labels Feb 21, 2025
@oscardssmith
Copy link
Member

This looks like a functional fix, but I think it might be better to use the same approach that #53967 adds where we use a floating point algorithm for large powers (especially since it would turn this PR into a performance improvement rather than a regression. Specifically, I think the following would work well. (testing now)

@constprop :aggressive @inline function ^(x::Float32, n::Integer)
    n = clamp(n, Int64)
    n == 0 && return one(x)
    if use_power_by_squaring(n)
        n < 0 && return oftype(x, Base.power_by_squaring(inv(widen(x)), -n))
        return oftype(x, Base.power_by_squaring(inv(widen(x)), n))
    else
        s = ifelse(x < 0 && isodd(n), -1f0, 1f0)
        x = abs(x)
        y = float(n)
        return copysign(Float32(exp2(log2(Float64(x)*y))), s)
    end
end

@kuszmaul
Copy link
Author

It does look like a performance improvement (once you get y outside of the log2. It should be exp2(log2(Float64(x))*y)
And it seems not to suffer from any errors in precision. You can just use pow_body.

@kuszmaul
Copy link
Author

Although I don't think this PR is a regression. It runs the same code that it always ran for cases except typemin, which previously crashed.

@oscardssmith
Copy link
Member

I meant performance regression. This code is pretty fast to the point that extra branches can have a significant speed impact. This way the branch is still there, but it is providing a speedup for lots of large powers as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Float32/16 raised to an integer's typemin throws a DomainError
4 participants