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

add BigFloat fma and muladd #211

Merged
merged 1 commit into from
Apr 20, 2023
Merged

add BigFloat fma and muladd #211

merged 1 commit into from
Apr 20, 2023

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Apr 18, 2023

No description provided.

@nsajko nsajko changed the title WIP, help needed: add BigFloat fma and muladd add BigFloat fma and muladd Apr 19, 2023
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.16 🎉

Comparison is base (86a502e) 88.80% compared to head (4c50cf9) 88.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   88.80%   88.96%   +0.16%     
==========================================
  Files          22       22              
  Lines        2108     2121      +13     
==========================================
+ Hits         1872     1887      +15     
+ Misses        236      234       -2     
Impacted Files Coverage Δ
src/implementations/BigFloat.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

# one at http://mozilla.org/MPL/2.0/.

@testset "fma $op output values" for op in (MA.operate_to!, MA.operate_to!!),
i in 1:100
Copy link
Member

Choose a reason for hiding this comment

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

I prefer a few fixed tests for which we know muladd is different than fma than 100 random tests since it's difficult to reproduce

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the testing approach is to try a range of two-bit BigFloat values.

y::F,
z::F,
) where {F<:BigFloat}
return operate_to!(output, Base.fma, x, y, z)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to do this because then muladd(x, y, z) would give a different result that operate!(muladd, x, y, z) wouldn't it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Julia Base is being changed in the same direction, using the fused mpfr_fma for muladd: JuliaLang/julia#49401

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to go then

@nsajko
Copy link
Contributor Author

nsajko commented Apr 19, 2023

How to fix this:

julia> MA.operate!!(fma, 1, 1, 1)
ERROR: MethodError: no method matching operate(::typeof(fma), ::Int64, ::Int64, ::Int64)

Do I need to define an operate method for fma and muladd?

@nsajko
Copy link
Contributor Author

nsajko commented Apr 19, 2023

The documentation actually leads me to believe that I shouldn't need to do anything, as immutable types should fall back to op(args...) automatically, no?

@blegat
Copy link
Member

blegat commented Apr 20, 2023

The documentation actually leads me to believe that I shouldn't need to do anything, as immutable types should fall back to op(args...) automatically, no?

Yes, operate(op, args...) should redirect to op(args...) like
but also make sure it returns a fresh copy which is what is done in

operate(::typeof(convert), ::Type{T}, x::T) where {T} = copy_if_mutable(x)
function operate(::Union{typeof(+),typeof(*),typeof(gcd),typeof(lcm)}, x)
return copy_if_mutable(x)
end

You can add fma in
function operate(
op::Union{
typeof(+),
typeof(*),
AddSubMul,
typeof(add_dot),
typeof(gcd),
typeof(lcm),
},
x,
y,
args::Vararg{Any,N},
) where {N}
return op(x, y, args...)
end

@blegat blegat merged commit c527f4a into jump-dev:master Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants