-
Notifications
You must be signed in to change notification settings - Fork 35
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ 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. |
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 ? |
Okay, let's ignore the julia-nightly failures. Do you want to add some tests for your PR? |
Yes, I am willing to add some tests. I can do that in a couple of days. |
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 |
Regarding the advantages: vectorized math functions are defined in The extension does not change the behavior of When I started using I understand your concerns regarding the dependency on |
I have two more commits ready:
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 ? |
The current behaviour of 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:
@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. |
@eschnett Your suggestion is fine for me. I like Some math functions are presently defined for |
After some polishing, here it is: @eschnett I think you can close this PR. btw the CI errors were indeed due to the proposed extension. This occurred because:
|
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.