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

more efficient muladd for complex/real operations #15980

Merged
merged 2 commits into from
Apr 22, 2016

Conversation

stevengj
Copy link
Member

Fixes #15969, and corrects some efficiency problems in @evalpoly introduced in #9881.

@stevengj
Copy link
Member Author

stevengj commented Apr 21, 2016

Here's an oddity. If I do f(z) = @evalpoly(z, 3,4,5), and then @code_native f(3.0), the result looks nice: inlined Horner's rule, as expected. However, @code_llvm f(3.0) is a total mess, with dozens of store %jl_value_t* instructions that look like operations on boxed values.

Clearly, the optimizer is doing the right thing at the end, as indicated by @code_native, but a few days ago the @code_llvm output was much cleaner. @vtjnash, has something changed recently in the code generation that would have caused this?

muladd(z::Complex, x::Real, y::Real) = Complex(muladd(real(z),x,y), imag(z)*x)
muladd(z::Complex, x::Real, w::Complex) =
Complex(muladd(real(z),x,real(w)), muladd(imag(z),x,imag(w)))
muladd(x::Real, y::Real, z::Complex) = Complex(muladd(x,y,real(z)), imag(z))
Copy link
Contributor

Choose a reason for hiding this comment

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

This leaves out the case (Complex, Complex, Real). Is this intentional? If so, a comment might make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was intentional. All of the benefit from the muladd(Complex, Complex, Real) case could be gained by using muladd in Complex*Complex

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this save one addition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.

Copy link
Member Author

@stevengj stevengj Apr 21, 2016

Choose a reason for hiding this comment

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

@eschnett, no, because there is already an addition in both the real and imaginary parts of Complex*Complex that could be fused with one of the multiplications.

Oh wait, you're right; you could save an extra addition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, added the Complex*Complex cases

@vtjnash
Copy link
Member

vtjnash commented Apr 21, 2016

@stevengj that's #15898

@stevengj stevengj merged commit 64685dc into JuliaLang:master Apr 22, 2016
@stevengj stevengj deleted the muladd_complex branch April 22, 2016 02:45
@@ -904,6 +904,11 @@ end
# issue #10926
@test typeof(π - 1im) == Complex{Float64}

# issue #15969: specialized muladd for complex types
for x in (3, 3+13im), y in (2, 2+7im), z in (5, 5+11im)
@test muladd(x,y,z) == x*y + z
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably be ===

Copy link
Member Author

Choose a reason for hiding this comment

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

They are Complex{Int} values, so what does it matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

to test that the return type stays that way

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in de6468b

stevengj added a commit that referenced this pull request Apr 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants