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 fastmath methods of sin, cos, tan, atan2 #24031

Merged
merged 1 commit into from
Jun 28, 2018

Conversation

giordano
Copy link
Contributor

@giordano giordano commented Oct 6, 2017

These functions now have a fast pure Julia implementation, let the fastmath
versions fall back on the default methods.

I'm not very familiar with @fastmath stuff, so please double (or even triple) check I'm not missing something :-) At least, fastmath tests are passing locally.

@ararslan ararslan requested a review from simonbyrne October 6, 2017 23:36
@ararslan ararslan added maths Mathematical functions performance Must go faster labels Oct 6, 2017
@giordano
Copy link
Contributor Author

Bump

@StefanKarpinski
Copy link
Member

@simonbyrne, it would be great if you could review this when you get a chance.

@giordano
Copy link
Contributor Author

Patch rebased on current master

@giordano giordano mentioned this pull request Mar 14, 2018
17 tasks
@giordano
Copy link
Contributor Author

giordano commented May 9, 2018

Bump

@KristofferC
Copy link
Member

Can you show some benchmarks of before and after this PR for the relevant functions when using fastmath?

@giordano
Copy link
Contributor Author

giordano commented May 9, 2018

You can simply compare sin and @fastmath sin, currently the former is faster than the latter:

julia> using BenchmarkTools

julia> function f()
       a = 0.0
       for x in -100:0.01:100
           a = @fastmath sin(x)
       end
       return a
       end
f (generic function with 1 method)

julia> function g()
       a = 0.0
       for x in -100:0.01:100
           a = sin(x)
       end
       return a
       end
g (generic function with 1 method)

julia> @benchmark f()
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     382.706 μs (0.00% GC)
  median time:      383.062 μs (0.00% GC)
  mean time:        389.019 μs (0.00% GC)
  maximum time:     655.759 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

julia> @benchmark g()
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     356.151 μs (0.00% GC)
  median time:      356.489 μs (0.00% GC)
  mean time:        356.659 μs (0.00% GC)
  maximum time:     486.692 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

With this PR the performance of f and g is just the same

@giordano
Copy link
Contributor Author

Bump. I think it might be good to have this reviewd and merged before 0.7

@simonbyrne
Copy link
Contributor

LGTM. Just needs a rebase.

These functions now have a fast pure Julia implementation, let the fastmath
versions fall back on the default methods.
@giordano
Copy link
Contributor Author

Rebased on master

@vchuravy
Copy link
Member

Circleci is: Got exception LoadError("/tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl", 56, KeyError("SparseArrays")) outside of a @test LoadError: KeyError: key "SparseArrays" not found Stacktrace:

@vchuravy vchuravy merged commit 6e035d9 into JuliaLang:master Jun 28, 2018
@giordano giordano deleted the fastmath branch June 28, 2018 17:31
maleadt added a commit to JuliaGPU/CUDA.jl that referenced this pull request Oct 12, 2022
Base decided in JuliaLang/julia#24031 that FastMath.sincos should
fall back to the native implementation in Julia, because it is
faster than the intrinsics (for the CPU at least). That does not
hold for CUDA GPUs, so have it again call sin_fast/cos_fast.
maleadt added a commit to JuliaGPU/CUDA.jl that referenced this pull request Oct 12, 2022
Base decided in JuliaLang/julia#24031 that FastMath.sincos should
fall back to the native implementation in Julia, because it is
faster than the intrinsics (for the CPU at least). That does not
hold for CUDA GPUs, so have it again call sin_fast/cos_fast.
maleadt added a commit to JuliaGPU/CUDA.jl that referenced this pull request Jan 5, 2023
Base decided in JuliaLang/julia#24031 that FastMath.sincos should
fall back to the native implementation in Julia, because it is
faster than the intrinsics (for the CPU at least). That does not
hold for CUDA GPUs, so have it again call sin_fast/cos_fast.
maleadt added a commit to JuliaGPU/CUDA.jl that referenced this pull request Jan 6, 2023
Base decided in JuliaLang/julia#24031 that FastMath.sincos should
fall back to the native implementation in Julia, because it is
faster than the intrinsics (for the CPU at least). That does not
hold for CUDA GPUs, so have it again call sin_fast/cos_fast.
simonbyrne pushed a commit to simonbyrne/CUDA.jl that referenced this pull request Nov 13, 2023
Base decided in JuliaLang/julia#24031 that FastMath.sincos should
fall back to the native implementation in Julia, because it is
faster than the intrinsics (for the CPU at least). That does not
hold for CUDA GPUs, so have it again call sin_fast/cos_fast.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants