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 vectorized math functions when SLEEFPirates is loaded (weakdep, Julia 1.9) #117

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dubosipsl
Copy link

This PR is a WIP to connect the fast vectorized implementations of mathematical functions provided by SLEEFPirates.jl to the SIMD.jl vector type.

It is proposed to use the 'weak dependencies' introduced in Julia 1.9. This avoids a strong dependency of SIMD.jl on SLEEFPirates (+ VectorizationBase) which is undesirable (#106). The features are confined in the extension module ext/SLEEF_Ext.jl which is loaded only when both SIMD and SLEEFPirates are loaded. Thus it is up to the user to trigger these features and decide whether to depend on SLEEFPirates.

I mark it as WIP because it lacks tests and a compat entry for SLEEFPirates, VectorizationBase. Otherwise it should be good to go.

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.94%. Comparing base (0334b82) to head (e3f6f0b).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
ext/SLEEF_Ext.jl 92.85% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   91.91%   91.94%   +0.03%     
==========================================
  Files           5        6       +1     
  Lines         532      559      +27     
==========================================
+ Hits          489      514      +25     
- Misses         43       45       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eschnett
Copy link
Owner

Some of the CI tests are failing, mostly for Julia nightly. Do you understand these failures? Are they related to your changes?

@dubosipsl
Copy link
Author

Some of the CI tests are failing, mostly for Julia nightly. Do you understand these failures? Are they related to your changes?

This looks rather like a problem with the master branch. Since AFAIK SLEEFPirates is not loaded during tests (so far), the extension should not be loaded either, and this PR should have no effect at all on tests for the moment. Does the curent master pass CI on Julia nightly ?

@eschnett
Copy link
Owner

Okay, let's ignore the julia-nightly failures. Do you want to add some tests for your PR?

@dubosipsl
Copy link
Author

Yes, I am willing to add some tests. I can do that in a couple of days.
Btw I realize that this will require a dependency on SLEEFPirates, but only for the tests. If that is a problem the only way is a separate package, I guess.

@KristofferC
Copy link
Collaborator

KristofferC commented Oct 29, 2023

What's the advantage of defining these here if they are already available from SLEEFPirates? To me, SIMD.jl is as it is a very "clear" package in that it directly translates things to the LLVM IR you expect. So it is surprising to me that a package like this should add things based on very complicated packages like SLEEFPirates (with a dependency on VectorizationBase and Static etc).

If anything, those packages should in my mind depend on SIMD.jl and implement things on top of that, not the other way around.

The implementation here also has the issue that it changes the behaviour on for example sin(v::Vec) for everyone if SLEEFPirates is imported, not only for the package that imported it. This is a "spooky action at a distance" and I think is bad enough to forcing this to have to be reconsidered.

@dubosipsl
Copy link
Author

@KristofferC

Regarding the advantages: vectorized math functions are defined in SLEEFPirates but they don't work with SIMD.Vec. On the other hand (AFAIK) VectorizationBase does not have the equivalent of SIMD.VecRange + getindex / setindex! which makes it easy to explicitly vectorize loops working with arrays. So if I want both SIMD.VecRange and fast math functions I need this extension. Or a separate package.

The extension does not change the behavior of sin(v::Vec), it changes the implementation. There are now tests that verify that, admittedly on a limited set of inputs. Right now the tolerance is 4eps for all functions, because the fast functions are less accurate, but it could be changed to 1eps for normal functions and 4eps for fast functions.

When I started using SIMD.jl about a year ago it took me some time to realize that maths functions were not vectorized. On some scientific code this is a huge performance hit compared to, say, Fortran. So I think the "behavior" with this extension may actually be closer to what users expect.

I understand your concerns regarding the dependency on SLEEFPirates and VectorizationBase. This PR uses a weak dependency, which is a compromise between a full dependency and having to create a separate package. I let the package maintainer(s) judge whether this compromise is acceptable.

@dubosipsl
Copy link
Author

I have two more commits ready:

  • one to better control the tolerance used in tests
  • another one to avoid the warnings about overwritten method definitions. This one changes simdvec.jl as so:
for (op, constraint, llvmop) in UNARY_OPS
    @eval @inline (Base.$op)(x::Vec{<:Any, <:$constraint}) = Vec($(llvmop)(x.data))
end

becomes

# vmap applies `op` to vector `vec`
# SLEEF_Ext specializes vmap for `op`
@inline vmap(op, x) = vmap_llvm(op, vec)

# Base.op(vec) = vmap(op,x) = vmap_llvm(op,x)
for (op, constraint, llvmop) in UNARY_OPS
    @eval begin
        @inline (Base.$op)(x::Vec{<:Any, <:$constraint}) = vmap(Base.$op, x)
        @inline vmap_llvm(::typeof(Base.$op), x) = Vec($(llvmop)(x.data))
    end
end

Shall I go ahead ?

@eschnett
Copy link
Owner

The current behaviour of SIMD which offers math functions (sin etc.) but does not implement them efficiently was a stop-gap solution that should be improved. I think implementing them via SLEEFPirates is a good way to go.

Personally I do not mind pulling in a lot of dependencies since this is handled quite transparently in Julia. However, others seem to disagree. Here is another suggestion:

  • We release a new major version of SIMD. In this version, only efficiently implemented functions are provided. sin etc. are not available any more for SIMD types, at least not by default. Maybe they would still be provided in a new module SIMDSlowMath, also available as part of SIMD.
  • Another package (e.g. SIMDMath? SIMDFastMath? SIMDSLEEF?) provides these efficient functions.

@dubosipsl I do not want to create more work for you since you're already putting in a lot of effort. I hope that this new structure would be a fairly straightforward modification of your current pull request. I really would like to see efficient math functions be available for our SIMD types.

@dubosipsl
Copy link
Author

@eschnett Your suggestion is fine for me. I like SIMDFastMath. In a sense I have used this PR to learn about the process, GitHub actions etc. And the work will not be lost anyway. By implementing the tests I have even found a bug in SLEEFPirates that I have filed and has now been corrected.

Some math functions are presently defined for Vec but not all those found in Base (e.g. log1p) which can surprise users. So removing math functions altogether from SIMD.jl would also improve consistency.

@dubosipsl
Copy link
Author

After some polishing, here it is:
https://github.com/dubosipsl/SIMDFastMath

@eschnett I think you can close this PR.

btw the CI errors were indeed due to the proposed extension. This occurred because:

  • Julia nightly fails when a method is overwritten during precompilation
  • all dependencies, even weak ones, are loaded during precompilation, which caused SLEEF_Ext to be loaded
  • SLEEF_Ext did overwrite a few methods from SIMD

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