Fix TensorPrimitives.IndexOfMaxMagnitude signed integer tie#128484
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
|
@lilinus could you resolve the merge conflicts here? |
jeffhandley
left a comment
There was a problem hiding this comment.
Note
This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.
Holistic Review
Motivation: The linked issue demonstrates a real behavioral bug: for signed integers, IndexOfMaxMagnitude could disagree with MaxMagnitude when equal magnitudes have opposite signs. The fix is appropriately narrow and also hardens adjacent tie-breaking tests.
Approach: The vectorized signed-integer compare now mirrors the scalar T.MaxMagnitude/T.MinMagnitude tie rules while preserving the existing Abs(MinValue) overflow handling. I checked the scalar fallback, the 128/256/512-bit SIMD paths, the final aggregate/index selection logic, and the sibling IndexOfMinMagnitude path for consistency.
Summary: ✅ LGTM. I did not find a correctness issue in the updated tie handling: equal-magnitude signed integer ties now select positive for max-magnitude and negative for min-magnitude, while MinValue overflow still wins/loses as expected. The new tests cover scalar and vector-length inputs, opposite-sign ties, floating-point signed-zero parity, and restore MinValue exposure in the all-length magnitude tests.
Detailed Findings
✅ Correctness — signed integer ties and MinValue overflow remain consistent
In src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.IndexOfMaxMagnitude.cs lines 74-77, 100-103, and 126-129, the new equalResult is included only in the non-overflow comparison and remains masked by Vector*.IsNegative(yMag). That means current = +1, result = -1 now updates, current = -1, result = +1 does not, current = MinValue still updates over any non-MinValue, and an existing MinValue still prevents replacement by a smaller magnitude. The analogous IndexOfMinMagnitude changes in lines 74-77, 100-103, and 126-129 apply the inverse tie rule and keep MinValue treated as maximum magnitude, so it does not become a false minimum.
✅ Scalar/vector parity — the SIMD path matches the scalar operator semantics
The scalar Compare(T x, T y) methods already defer to T.MaxMagnitude/T.MinMagnitude and only update on the documented sign tie-break. The updated vector masks now apply the same replacement rule lane-by-lane, and IndexOfMinMaxCore then keeps the first index for equal retained values by preserving existing indices during same-value ties and selecting the minimum index among lanes equal to the horizontal aggregate. This addresses the linked vectorized-path mismatch without changing public API surface.
✅ Tests — coverage targets the regression and adjacent tie cases
src/libraries/System.Numerics.Tensors/tests/TensorPrimitivesTests.cs adds all-length coverage for equal-magnitude -1/+1 ties (IndexOfMaxMagnitude_Negative1LesserThanPositive1 and IndexOfMinMagnitude_Negative1LesserThanPositive1) and expands signed-zero tests across Helpers.TensorLengths, which exercises scalar and vector-length paths. The IndexOf*Magnitude_AllLengths tests now compute expectations with MaxMagnitude/MinMagnitude and use CreateAndFillTensor, so random signed integer inputs can include MinValue rather than avoiding the overflow-sensitive value. TensorPrimitives.Generic.cs also classifies NFloat, Half, nuint, and UInt128 correctly for these generic test guards.
Fixes #128478
I saw there are reported perf regressions for
TensorPrimitives.IndexOfMaxin #128088, so I understand if you can't take this PR to fix that (or revert #127454).PR also includes:
IndexOf*Magnitude_Negative1LesserThanPositive1MinValueinIndexOf*Magnitude_AllLengthsIndexOf*_Negative0LesserThanPositive0to cover vectorized paths.TensorPrimtiviesare run forNFloat