Skip to content

Don't throw from TensorPrimitives.Clamp when min is greater than max - #130703

Merged
tannergooding merged 5 commits into
dotnet:mainfrom
tannergooding:tannergooding-fix-tensors-clamp-validation
Jul 15, 2026
Merged

Don't throw from TensorPrimitives.Clamp when min is greater than max#130703
tannergooding merged 5 commits into
dotnet:mainfrom
tannergooding:tannergooding-fix-tensors-clamp-validation

Conversation

@tannergooding

Copy link
Copy Markdown
Member

The span-based TensorPrimitives.Clamp overloads dispatch to the ClampOperator* structs whose scalar Invoke calls T.Clamp, which throws ArgumentException when min > max, while the Vector128/256/512 Invoke paths call Vector*.Clamp, which follow HLSL and compute Min(Max(x, min), max) without validation.

This made behavior position-dependent for the span-min/span-max overloads: an unordered min[i] > max[i] pair threw when it landed in the scalar remainder but was silently clamped when it landed in the vectorized region. The Clamp(ReadOnlySpan<T> x, T min, T max, ...) overload additionally validated min <= max up front and threw before dispatch.


Align the scalar path with the vector path by computing Min(Max(x, min), max) in the scalar Invoke and removing the up-front validation, so Clamp succeeds for unordered bounds regardless of element position, matching Vector.Clamp. T.Min(T.Max(x, min), max) is exactly T.Clamp minus the throw, and (via IEEE Min/Max) agrees with the vector path on NaN and signed zeros by construction.

Also removes the now-unused ThrowArgument_MinGreaterThanMax helper and its resource string, and updates the doc comments.

Tests: adds Clamp_UnorderedBoundsMatchScalarSemantics and Clamp_SpecialValuesMatchScalarSemantics to GenericNumberTensorPrimitivesTests<T>, exercising both the scalar (sub-vector lengths) and vectorized (length 128) paths across all affected overloads/operand orderings and types, including NaN and signed-zero parity between the two paths for the floating-point types.


Note

This is a documented-behavior change: the previously documented ArgumentException on min > max is removed, so it warrants breaking-change consideration.

Note

This PR was authored with GitHub Copilot.

The span-based Clamp overloads dispatch to the ClampOperator* structs whose
scalar Invoke previously called T.Clamp, which throws ArgumentException when
min > max, while the Vector128/256/512 Invoke paths call Vector*.Clamp, which
follow HLSL and compute Min(Max(x, min), max) without validation. This made
behavior position-dependent: an unordered min[i] > max[i] pair threw when it
landed in the scalar remainder but was silently clamped in the vectorized
region. The Clamp(x, T min, T max) overload additionally validated min <= max
up front and threw before dispatch.

Align the scalar path with the vector path by computing Min(Max(x, min), max)
in the scalar Invoke and removing the up-front validation, so Clamp succeeds
for unordered bounds regardless of element position, matching Vector.Clamp.
Remove the now-unused ThrowArgument_MinGreaterThanMax helper and its resource
string, and update the doc comments.

Add regression tests that exercise both the scalar and vectorized paths across
the affected overloads and types, including NaN and signed-zero parity between
the two paths for the floating-point types.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 changes TensorPrimitives.Clamp to avoid throwing when min > max by aligning the scalar fallback behavior with the vectorized Vector128/256/512.Clamp semantics (compute Min(Max(x, min), max)), and updates documentation/tests accordingly.

Changes:

  • Remove the scalar min > max validation and switch scalar ClampOperator* implementations to T.Min(T.Max(x, min), max).
  • Delete the now-unused throw helper and resource string for the removed ArgumentException.
  • Add generic tests to ensure scalar vs vectorized paths match (including floating-point special values like NaN and signed zero).
Show a summary per file
File Description
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.Clamp.cs Removes min > max throwing and makes scalar operator semantics match vector clamp; updates XML docs.
src/libraries/System.Numerics.Tensors/src/System/ThrowHelper.cs Removes unused ThrowArgument_MinGreaterThanMax.
src/libraries/System.Numerics.Tensors/src/Resources/Strings.resx Removes unused Argument_MinGreaterThanMax resource string.
src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Adds regression tests validating consistent clamp semantics across scalar/vectorized paths and special values.

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment thread src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 14, 2026 18:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new

The scalar ClampOperator Invoke previously computed Min(Max(x, min), max) for every T, which silently dropped the min > max validation even for types with no vector acceleration. Gate the bypass on Vector128<T>.IsSupported so vector-accelerated types match the non-throwing Vector.Clamp path while all other types defer to T.Clamp and keep throwing when min > max.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 14, 2026 19:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 4

Comment thread src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Outdated
Comment thread src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Outdated
… comments

Restore an ArgumentException note on the Clamp overloads describing that unordered bounds throw for a non-vectorizable T, and reword two test comments that misstated the vector-width and Half code paths.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 14, 2026 20:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 3

Comment thread src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Outdated
TensorPrimitives.Clamp vectorizes Half via the Half-as-Int16 path, so its scalar fallback must also compute Min(Max(x, min), max) instead of deferring to T.Clamp; otherwise Half would throw on unordered bounds only for short spans (below the reinterpret threshold) or when hardware acceleration is unavailable. Also add length 129 to cover the vectorized-plus-scalar-remainder mix in a single call.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 14, 2026 21:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new

@tannergooding
tannergooding merged commit 17bbb20 into dotnet:main Jul 15, 2026
89 checks passed
@tannergooding
tannergooding deleted the tannergooding-fix-tensors-clamp-validation branch July 15, 2026 11:56
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 11.0-preview7 milestone Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants