Fix intrinsic testing add and subtract overflow helpers #122977
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
@dotnet/arm64-contrib |
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
There was a problem hiding this comment.
Pull request overview
This PR refactors overflow detection and saturating arithmetic helper methods in the ARM hardware intrinsics test helpers. It replaces multiple type-specific implementations with generic methods using IBinaryInteger<T> and BigInteger for accurate overflow detection.
Key changes:
- Consolidates duplicate overflow detection logic into generic methods
- Fixes overflow detection bugs by using
BigIntegerfor precise range checking - Reduces code duplication across sbyte, short, int, and long types
| byte result; | ||
| bool addOvf = TryAddUnsigned(op1, rndCns, out result); |
There was a problem hiding this comment.
nit: No need to fix this, but just as FYI you can do this, which is a bit more idiomatic in modern C#:
| byte result; | |
| bool addOvf = TryAddUnsigned(op1, rndCns, out result); | |
| bool addOvf = TryAddUnsigned(op1, rndCns, out byte result); |
| result = unchecked(left + TSigned.CreateChecked(right)); | ||
| return result < left; |
There was a problem hiding this comment.
Is this one correct?
Presumably this is for SUQADD (signed saturating addition of unsigned value), in which case the implementation is converting right from unsigned to signed which will throw if it is greater than TSigned.MaxValue.
That seems incorrect because I'd expect, for example, that -128 + 128 was valid and would produce 0
| bool ovf; | ||
|
|
||
| if (op2 < 0) | ||
| if (right < TSigned.Zero) |
There was a problem hiding this comment.
nit:
| if (right < TSigned.Zero) | |
| if (TSigned.IsNegative(right)) |
| { | ||
| ovf = (result < op1); | ||
| var mag = TUnsigned.CreateChecked(-right); |
There was a problem hiding this comment.
This seems "wrong" since it will fail for TSigned.MinValue.
I believe you rather just want TUnsigned.CreateTruncating(-right), since all other values will have become positive while right and the two's complement representation is the same between MinValue and (MaxValue + 1).
-- Noting this is only sensible if TSigned and TUnsigned are the same width.
| ovf = (result < op1); | ||
| var mag = TUnsigned.CreateChecked(-right); | ||
| result = unchecked(left - mag); | ||
| return left < mag; |
There was a problem hiding this comment.
Shouldn't this be checking result > left?
That is, we're subtracting two unsigned values, so we overflow if the new result is greater than the original input.
2 participants
Fixes #122728