-
Notifications
You must be signed in to change notification settings - Fork 72
Updating LoopVectorization for 1.12 #557
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #557 +/- ##
==========================================
+ Coverage 77.48% 86.33% +8.85%
==========================================
Files 39 39
Lines 9561 9579 +18
==========================================
+ Hits 7408 8270 +862
+ Misses 2153 1309 -844 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Project.toml
Outdated
|
||
[extensions] | ||
ForwardDiffExt = ["ChainRulesCore", "ForwardDiff"] | ||
ForwardDiffExt = ["ChainRulesCore", "ForwardDiff", "NNlib"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NNLib should be a separate extension. Not only is this breaking, it also means that it won't properly trigger for most users.
ext/ForwardDiffExt.jl
Outdated
@generated function NNlib.leakyrelu( | ||
x::ForwardDiff.Dual{T,S,N}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is type piracy. Why is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those functions were once written with branches.
LV only added methods for the SIMD types and Duals of SIMD types where this was problematic.
Not sure why this PR dropped the constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that's what the test suite tested for and was causing this error -- which occurs across pre, 1, and lts. I figured it would be cleaner to move the definition to the extension instead of defining it in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it appears that I may have missed a subtlety in the original implementation with branching; what was the original goal for these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concern is that the old functions were only overloaded for AbstractSIMD
:
LoopVectorization.jl/test/forwarddiffext.jl
Lines 20 to 31 in 945c2f7
@inline function NNlib.leakyrelu( | |
x::LoopVectorization.AbstractSIMD, | |
a = NNlib.oftf(x, NNlib.leakyrelu_a), | |
) | |
LoopVectorization.ifelse(x > zero(x), float(x), NNlib.oftf(x, a * x)) # max(a*x, x) is 3x slower | |
end | |
@inline function NNlib.leakyrelu( | |
x::ForwardDiff.Dual{<:Any,<:LoopVectorization.AbstractSIMD}, | |
a = NNlib.oftf(x, NNlib.leakyrelu_a), | |
) | |
LoopVectorization.ifelse(x > zero(x), float(x), NNlib.oftf(x, a * x)) # max(a*x, x) is 3x slower | |
end |
The new ones work for any
S
. This is why the new ones commit type piracy, while the old ones only sort of did (sort of, because LoopVectorization doesn't itself define AbstractSIMD
, but has permission to add methods if needed).
Probably SLEEFPirates is a better place for this, with an NNlib extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, should I remove these tests and the new associated functions from the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it seems strange that many functions defined in the extension aren't being tested; I can add a new set of tests if that's within scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make them dispatch on AbstractSIMD again.
ext/ForwardDiffExt.jl
Outdated
@generated function NNlib.relu( | ||
x::ForwardDiff.Dual{T,S,N} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is type piracy.
Because It is still a bit odd that the rrules for SLEEFPirates are in there since they have nothing to do with ForwardDiff, but they at least were there before so this is at least not a regression from the previous behavior. |
To me, if this passes tests then it's good to go. |
Open a PR to the small grants to claim your finish. |
Congrats on the PR! As an aside that is unrelated to this PR, I looked at an assembly example from LV in the past week and realized the quality regressed since a year ago or so. There was a lot of awkward code related to that change -- casting to use geps of different types to control scalar multiples in addressing operations -- which could be removed, if someone needs to touch the code again. |
Does this mean that LoopVectorization now fully supports Julia 1.12? |
yes. |
Adding changes to LV for compatibility with 1.12
Done as part of my contributions for the SciML small grants initiative