Skip to content

Conversation

mxpoch
Copy link
Contributor

@mxpoch mxpoch commented Sep 1, 2025

Adding changes to LV for compatibility with 1.12

Done as part of my contributions for the SciML small grants initiative

Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.33%. Comparing base (641e31e) to head (4fd0f3d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/constructors.jl 85.71% 2 Missing ⚠️
ext/ForwardDiffExt.jl 91.66% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Project.toml Outdated

[extensions]
ForwardDiffExt = ["ChainRulesCore", "ForwardDiff"]
ForwardDiffExt = ["ChainRulesCore", "ForwardDiff", "NNlib"]
Copy link
Member

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.

Comment on lines 161 to 162
@generated function NNlib.leakyrelu(
x::ForwardDiff.Dual{T,S,N},
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

@mxpoch mxpoch Oct 6, 2025

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.

Copy link
Contributor Author

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?

Copy link
Member

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:

@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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Comment on lines 144 to 145
@generated function NNlib.relu(
x::ForwardDiff.Dual{T,S,N}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is type piracy.

@ChrisRackauckas
Copy link
Member

Because relu is not from LoopVec anymore, it needs to be careful to not be pirating, while not disabling the adding of other dispatches. The way you had it, not adding NNlib would disable all ForwardDiff overloads, and it would pirate NNlib.relu's rrule when it is added. I made a few code movements so that those bad behaviors are avoided.

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.

@ChrisRackauckas
Copy link
Member

To me, if this passes tests then it's good to go.

@ChrisRackauckas ChrisRackauckas merged commit 9dc2ffd into JuliaSIMD:main Oct 8, 2025
68 of 75 checks passed
@ChrisRackauckas
Copy link
Member

Open a PR to the small grants to claim your finish.

@chriselrod
Copy link
Member

chriselrod commented Oct 8, 2025

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.
This is probably just the inevitable decay as LLVM changes, and some of the tricks for getting good code gen out of it fall out of date.

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.
Otherwise, probably not worth touching the code.

@ForceBru
Copy link

ForceBru commented Oct 9, 2025

Does this mean that LoopVectorization now fully supports Julia 1.12?

@oscardssmith
Copy link
Member

yes.

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.

5 participants