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

float16 cbrt, 50% faster #39441

Merged
merged 4 commits into from
Apr 17, 2021
Merged

Conversation

oscardssmith
Copy link
Member

.5 ULP error tested on all Float16

@oscardssmith oscardssmith added float16 maths Mathematical functions performance Must go faster labels Jan 28, 2021
@ViralBShah
Copy link
Member

@simonbyrne @pkofod - Could you guys review?

@ViralBShah
Copy link
Member

@oscardssmith Should we merge if it is passing the comprehensive float16 testsuite?

@oscardssmith
Copy link
Member Author

If it passes, this is just free performance.

base/special/cbrt.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member Author

So bad news: This doesn't pass the test. Worse news, this shows that the test is actually wrong. With this PR, we can see

setprecision(BigFloat, 2048)
x=Float16(-1.089e4)
julia> cbrt(big(x))-cbrt(x)
0.0078122 ...
julia> cbrt(big(x))-Float16(cbrt(big(x)))
-0.0078127...

In other words, 2048 bits of precision is not enough for double rounding not to mess things up.

@ViralBShah ViralBShah changed the title float16 cbrt, 50% faster [WIP] float16 cbrt, 50% faster Feb 17, 2021
@petvana
Copy link
Member

petvana commented Feb 21, 2021

In other words, 2048 bits of precision is not enough for double rounding not to mess things up.

Interestingly, this doesn't seem to be a problem of BigFloat. But rather of double rounding from BigFloat through Float32 down to Float16, which is a separate issue, I guess. Similar example of rounding from Float64 to Float16.

julia> x = -22.164062733931335
-22.164062733931335
julia> abs(Float16(x) - x)
0.007812733931334748
julia> abs(prevfloat(Float16(x)) - x)
0.007812266068665252

@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2021

IIUC, the above discussion concludes that we should merge this, as it is actually more accurate than the tests. Correct?

@oscardssmith
Copy link
Member Author

I'm not sure we should merge this until #40315 is fixed. Basically right now, Julia does not have a correct way for me to test this since conversion from BigFloat to Float16 is screwed up. As such, the tests are not as reliable as I want them to be. The argument for merging is that this is almost certainly good enough (.5001 ULP would be my guess), and the performance is very real and easy to quantify.

@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2021

I think I'm still okay merging with knowing that. Aside: do we need to fix the definition of tryparse eventually (which currently parses as Float32) too?

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 16, 2021
@vtjnash vtjnash changed the title [WIP] float16 cbrt, 50% faster float16 cbrt, 50% faster Apr 16, 2021
@dkarrasch dkarrasch merged commit b6c6bc0 into JuliaLang:master Apr 17, 2021
@oscardssmith oscardssmith deleted the float16-cbrt branch April 17, 2021 17:16
vtjnash added a commit that referenced this pull request Apr 19, 2021
From #39432 and #39441, these were still using their old definition due
to method overwriting.
vtjnash added a commit that referenced this pull request Apr 19, 2021
From #39432 and #39441, these were still using their old definition due
to method overwriting.
oscardssmith pushed a commit that referenced this pull request Apr 20, 2021
From #39432 and #39441, these were still using their old definition due
to method overwriting.
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
From JuliaLang#39432 and JuliaLang#39441, these were still using their old definition due
to method overwriting.
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
From JuliaLang#39432 and JuliaLang#39441, these were still using their old definition due
to method overwriting.
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
From JuliaLang#39432 and JuliaLang#39441, these were still using their old definition due
to method overwriting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
float16 maths Mathematical functions performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants