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

Update base/complex.jl for Complex round (Fix/Workaround #10027) #10145

Merged
merged 1 commit into from
Feb 10, 2015
Merged

Update base/complex.jl for Complex round (Fix/Workaround #10027) #10145

merged 1 commit into from
Feb 10, 2015

Conversation

rickhg12hs
Copy link
Contributor

Adding superfluous tuple splatting to Complex return arguments seems to be an effective workaround for 32-bit system rounding problems for Complex types (#10027).

Adding superfluous tuple splatting to Complex return arguments seems to be an effective workaround for 32-bit systems
@StefanKarpinski
Copy link
Member

It's weird that that fixed things, but compilers be crazy. How'd you even figure that out?

@rickhg12hs
Copy link
Contributor Author

@StefanKarpinski : Bottom line, I didn't really figure it out.

I saw that my splatted map work around for round(z::Complex) (from a comment in #10027) wasn't going to translate well for function round{T<:FloatingPoint, MR, MI}(z::Complex{T}, ::RoundingMode{MR}, ::RoundingMode{MI}) (also broken) so I looked at other options. I couldn't figure out how @noinline works (as @vtjnash hinted at), so I just tried the simplest splat I could think of since it seemed that an intermediate result was getting lost/corrupted somehow and perhaps an intermediate tuple might preserve it. Splatting the tuple makes the Complex constructor happy and make test-complex passed on my 32-bit system. Yay! 8-)

N.B.: I have bent screwdrivers from using them as crowbars too.

@staticfloat
Copy link
Member

I can also confirm that this fixes things. I say merge this so that the build is not broken, and then we can iterate on a nicer fix.

staticfloat added a commit that referenced this pull request Feb 10, 2015
…ound

Update base/complex.jl for Complex round (Fix/Workaround #10027)
@staticfloat staticfloat merged commit 1575bc8 into JuliaLang:master Feb 10, 2015
@rickhg12hs rickhg12hs deleted the complex-round-32-bit-workaround branch February 10, 2015 06:31
@timholy
Copy link
Member

timholy commented Feb 10, 2015

An interesting fix. There are a lot of tuple-related bugs open now (convertalypse, the --inline=no bugs, etc), I wonder if such shenanigans might help elsewhere too.

CCing @JeffBezanson to see if he has any insight.

@JeffBezanson
Copy link
Member

This seems to work by preventing inlining of the Complex constructor. This is probably working around an LLVM bug. Our compiler removes the tuple entirely, of course. This fix is not terribly safe, since we could certainly improve the compiler to be able to inline in this case.

@vtjnash
Copy link
Member

vtjnash commented Feb 10, 2015

iirc, llvm is trying to return this complex number in fp(0) and fp(1). Is it possible that the call to round is corrupting the floating point stack that is being expected by llvm?

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.

6 participants