-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Implement AddSaturate, SubtractSaturate and NarrowWithSaturation on the Vector types #115525
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
CC. @dotnet/jit-contrib, @EgorBo, @kunalspathak to review the JIT side changes CC. @michaelgsharp for the libraries side changes |
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.
Pull Request Overview
This PR adds saturating add/subtract operations and narrowing-with-saturation to various vector types and their scalar fallbacks, updates the intrinsic reference surfaces, and extends JIT and tests to cover these new APIs.
- Introduce
AddSaturate
andSubtractSaturate
methods inVector64/128/256/512
and genericVector<T>
and their scalar implementations. - Add
NarrowWithSaturation
overloads for all vector sizes and wire them through intrinsics lists and JIT import logic. - Extend tests in all
VectorXXTests.cs
andGenericVectorTests.cs
to exercise the new saturating and narrowing behaviours.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector512.cs | Implements AddSaturate , SubtractSaturate , NarrowWithSaturation , fixes unsafe patterns |
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Scalar.cs | Adds scalar AddSaturate /SubtractSaturate implementations |
src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs | Adds ref API entries for new saturating and narrowing methods |
src/coreclr/jit/hwintrinsiclistxarch.h | Registers new intrinsics in x86/x64 list |
src/coreclr/jit/hwintrinsiclistarm64.h | Registers new intrinsics in ARM64 list |
src/coreclr/jit/hwintrinsicarm64.cpp | Implements ARM64 import logic for saturating add/sub/narrowing |
src/libraries/System.Runtime.Intrinsics/tests/Vectors/VectorXXTests.cs | Adds unit tests for saturating and narrowing operations |
src/libraries/System.Numerics.Vectors/src/* | Adds AddSaturate /SubtractSaturate to generic Vector<T> |
src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs | Adds generic tests for new Vector<T> saturating ops |
Comments suppressed due to low confidence (2)
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector512.cs:4
- The
ShiftLeft
implementation usesvector._lower
for both halves. The upper half should be shifted fromvector._upper
, i.e., useVector256.ShiftLeft(vector._upper, shiftCount._upper)
.
Vector256.ShiftLeft(vector._lower, shiftCount._upper)
src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector512Tests.cs:7799
- Consider adding a test for
NarrowWithSaturation
fromdouble
tofloat
(e.g.Vector512<double>
→Vector512<float>
) to validate floating-point narrowing behavior.
public void NarrowWithSaturationUInt64Test()
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Scalar.cs
Show resolved
Hide resolved
/azp run Fuzzlyn, Antigen |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
JIT changes LGTM
/azp run runtime-coreclr jitstress-isas-x86, Fuzzlyn, Antigen |
Azure Pipelines successfully started running 3 pipeline(s). |
Fuzzlyn, JitStress ISA, and Antigen failures are unrelated and tracked by existing issues. |
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.
LGTM
No description provided.