-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
System.UInt64.TryMultiply to check for overflow without throwing exceptions #46259
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @tannergooding, @pgovind Issue Details@tannergooding, @pgovind namespace System
{
public struct UInt64 // Alternatively System.Math
{
public static unsafe UInt64? TryMultiply(UInt64 a, UInt64 b)
{
unchecked {
if (System.Runtime.Intrinsics.X86.Bmi2.X64.IsSupported)
{
uint64 low;
if (System.Runtime.Intrinsics.X86.Bmi2.X64.MultiplyNoFlags(a, b, &low) == 0)
return low;
return null;
}
if (b == 0 || a <= (UInt64.MaxValue / b)) return a * b;
return null;
}
}
// And for System.UInt32.TryMultiply or System.Math.TryMultiply(UInt32, UInt32):
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
public static UInt32? TryMultiply(UInt32 a, UInt32 b)
{
unchecked {
UInt64 product64 = (UInt64)a * b;
UInt32 product32 = (UInt32)product64;
if (product32 == product64) return product32;
return null;
}
}
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
public static UInt64? TryAdd(UInt64 a, UInt64 b)
{
UInt64 result = unchecked(a + b);
return (result >= a) ? result : default(UInt64?);
}
}
} These methods intentionally do not use Maybe you know, and could answer, the question of whether my In any event, regardless of whether the above implementations are the best-possible, they're already at least good implementations that could be added to NETFW 5.x immediately, and later further optimized if necessary. See also issue #13026 where @RobertBouillon suggests new CIL instructions. Although I like his idea, it's far more work than the alternative of
|
FWIW I've also encountered scenarios where |
Should these method take nullables too so that you can write expressions without checking the result after each operation?
https://docs.microsoft.com/en-us/cpp/safeint/safeint-class is a prior art for this in C++. |
@jkotas -- That's a nice idea. I see what you mean. The methods could be overloaded -- both nullable and non-nullable versions. For the overloads with nullable parameters, they operate with a principle akin to [MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
public static UInt64? TryAdd(UInt64? a, UInt64? b)
{
UInt64 result = unchecked(a.GetValueOrDefault() + b.GetValueOrDefault());
return (result >= a & a.HasValue & b.HasValue) ? result : default(UInt64?);
}
public static Int64? TryAdd(Int64? a, Int64? b)
{
Int64 result = unchecked(a.GetValueOrDefault() + b.GetValueOrDefault());
return (unchecked((UInt64)((result ^ a) & (result ^ b)) >> 63) == 0 & a.HasValue & b.HasValue) ? result : default(Int64?);
} Therefore, like you said, you can perform multiple math operations, and instead of checking for null after every operation, you can check for null once at the end. Likewise the technique of "check once at the end" works with NaN. Int64? x = Int64.TryAdd(a, b);
x = Int64.TryAdd(x, q);
var z = Int64.TryMultiply(x, t);
if (!z.HasValue) OverflowOccurred(); The equivalent with NaN is: double x = a + b;
x = x + q;
double z = x * t;
if (double.IsNaN(z)) AnyOfTheVariablesWasNaN(); |
What do you think of possibly supporting that by adding one more constructor to public partial struct Nullable<T> where T : struct
{
private readonly bool hasValue;
internal T value;
public Nullable(T inValue, bool inHasValue)
{
this.value = inValue;
this.hasValue = inHasValue;
}
public Nullable(T inValue)
{
this.value = inValue;
this.hasValue = true;
}
public readonly T GetValueOrDefault() => hasValue ? this.value : default(T);
public readonly T GetValueOrDefault(T defaultValue) => hasValue ? this.value : defaultValue;
public readonly bool ValueUnchecked { get => this.value; }
public readonly T Value { get { if (!hasValue) throw xxxx; return this.value; } }
} With such an additional constructor for [MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
public static UInt64? TryAdd(UInt64 a, UInt64 b)
{
UInt64 result = unchecked(a + b);
return new Nullable<UInt64>(result, result >= a);
}
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
public static UInt64? TryAdd(UInt64? a, UInt64? b)
{
UInt64 result = unchecked(a.GetValueOrDefault() + b.GetValueOrDefault());
// OR if ValueUnchecked exists: UInt64 result = unchecked(a.ValueUnchecked + b.ValueUnchecked);
return new Nullable<UInt64>(result, result >= a & a.HasValue & b.HasValue);
} Would it cause any bad side-effects if the field The above example code preserves the preexisting behavior for The above example takes a conservative (but possibly unnecessary) approach of changing the implementation of public readonly T GetValueOrDefault() => hasValue ? this.value : default(T); But it might still be satisfactory to leave the implementation of public readonly T GetValueOrDefault() => this.value; ...considering that we're not talking about a change to all nullables, rather the change only affects nullables produced by special methods such as i.e. if a caller of |
It does not preserve the performance characteristics. This change would be performance regression for number of current uses of |
Neat, even in cases where both the flag and sum are NOT needed, if both are returned anyway, then it has a nice little side-effect of making public static Int64? TryAdd__NormalNullable(Int64? a, Int64? b)
{
Int64 result = unchecked(a.GetValueOrDefault() + b.GetValueOrDefault());
return (unchecked((UInt64)((result ^ a) & (result ^ b)) >> 63) == 0 & a.HasValue & b.HasValue) ? result : default(Int64?);
}
public static Int64? TryAdd__NullableWithPayload(Int64? a, Int64? b)
{
Int64 result = unchecked(a.ValueUnchecked + b.ValueUnchecked);
return new Nullable<Int64>(result, unchecked((UInt64)((result ^ a) & (result ^ b)) >> 63) == 0 & a.HasValue & b.HasValue);
} So in this case returning more information is actually slightly better for performance. |
OK, in that case, it sounds like the better option might be to leave Additionally also still add the Anyway I never liked the fact that |
This note would really have to say that you cannot pass this Nullable to other methods that take Nullable. It would be less confusing to create a completely new type that has conversion to proper Nullable. |
I've not given this a proper lookover yet due to the holidays. But my initial thought is that this is very different to how .NET exposes "try" APIs and I'd wonder if the JIT could have efficient codegen with it and if it would be useable with common overflow patterns. For example, #include <stdint.h>
#include <intrin.h>
struct UInt128 { uint64_t lo, hi; };
UInt128 Test(UInt128 left, UInt128 right)
{
UInt128 result;
auto carry = _addcarry_u64(false, left.lo, right.lo, &result.lo);
carry = _addcarry_u64(carry, left.hi, right.hi, &result.hi);
return result;
} This works well because the I wonder if it would be better to mirror the general API style or if returning a We already handle |
Ahhhh! Lightbulb! After reading what @tannergooding and @jkotas wrote, I see that apparently the best design is:
This discussion has revealed an interesting key point. I see now why this fundamental "basics" issue (overflow checking) wasn't already completed long ago. I now understand that there are 2 different goals that have been disrupting each other and being interchanged:
I see now the reason why these 2 different issues have been forever interchanged is that the addition carry bit happens to be exactly the same size (1 bit) as an overflow flag, thus it's so easy to interchange these 2 different issues, or to get seduced by the goal of hitting 2 birds with 1 stone. Apparently, in this particular case, if we try to make one hammer that hits both of those goals, then it ends up being a weak hammer that works (albeit poorly) for both goals. Therefore apparently the goals need to be separated, and two different hammers created. Therefore it would be best to first focus on multiplication, because multiplication makes it apparent that 2 different goals exist, whereas addition causes confusion because an overflow flag and an addition carry flag happen to be the same size (1 bit). Multiplication makes it clear because the multiplication "carry" is not a carry bit, flag, or boolean, rather it's the high and low halves of the product -- not a boolean! Thus a UInt64? TryMultiply(UInt64 a, UInt64 b, out UInt64 highHalf);
// or
UInt64 TryMultiply(UInt64 a, UInt64 b, out UInt64 highHalf, out bool isOverflow); That'd be nonsense because obviously the overflow boolean or public static ulong BigMul (ulong a, ulong b, out ulong low); Thus in cases where the goal is #2 (larger-integer purposes such as a 128-bit integer), it'd be nonsense to use The water becomes murky when you think about addition instead of multiplication, because in multiplication, the |
@GrabYourPitchforks wrote:
I have to revise my previous answer to you. Previously I said maybe it can be supported, but now I see apparently it shouldn't be supported, because it's a different purpose. For purposes where you want both flag and sum, then apparently designs like this should be used: // Already exists:
public static ulong BigMul (ulong a, ulong b, out ulong low);
// Could be made in theory:
public static bool BigAdd (ulong a, ulong b, out ulong sum);
// Tanner says this AddCarry works well:
bool AddCarry(bool inputFlag, uint left, uint right, out uint result); Whereas for the different purpose of checking for overflow -- the purpose I originally described -- then a suitable design is: UInt64? TryMultiply(UInt64 a, UInt64 b);
UInt64? TryMultiply(UInt64? a, UInt64? b);
UInt64? TryAdd(UInt64 a, UInt64 b);
UInt64? TryAdd(UInt64? a, UInt64? b); Apparently if we try to make a design that serves both of those purposes, then the design is poor for both purposes, therefore most likely this issue #46259 should be shrunk back to its original purpose: Only for the purpose of checking for overflow, not for helping with 128-bit math etc. To confirm, this means most likely the idea of giving |
@jkotas wrote:
I think this idea should be kept on the table as a possible alternate to using struct SafeMath
{
public bool OverflowOccurred;
public unsafe UInt64 Multiply(UInt64 a, UInt64 b)
{
unchecked {
if (System.Runtime.Intrinsics.X86.Bmi2.X64.IsSupported)
{
uint64 low;
OverflowOccurred |= System.Runtime.Intrinsics.X86.Bmi2.X64.MultiplyNoFlags(a, b, &low) != 0;
return low;
}
OverflowOccurred |= (b != 0 && (a > (UInt64.MaxValue / b)));
return a * b;
}
}
public UInt64 Add(UInt64 a, UInt64 b)
{
UInt64 result = unchecked(a + b);
OverflowOccurred |= (result < a); // This is NOT intended to be a way of returning a carry flag!
return result;
}
} Coincidentally the above Note the above I'm tempted to rewrite the above example to make it return zero when overflow occurs, in order to stop people trying to use this for 128-bit math etc, but if I do that, unfortunately it would terminate the little branch-free performance advantage. We should keep in mind that |
Disable overflow exceptions per-thread at runtime?Is this a good or bad idea? try
{
System.CheckOverflowWithoutExceptions.Enter();
checked
{
Int64 x = a + b;
x = x + q;
Int64 z = x * t;
}
if (CheckOverflowWithoutExceptions.OverflowOccurred) DoSomething();
}
finally
{
bool overflowOccurred = CheckOverflowWithoutExceptions.Exit();
} This would operate similar to the C# The above code could be either written manually as shown above, or automatically generate by the C# compiler, in the same manner as how currently you can either use the C# A question is whether it would affect only the current method (like how An ability to switch off overflow exceptions would have to be a per-thread switch, not per-process. Likewise the I don't know enough about the CLR, JIT, AOT to say whether or not this idea would introduce big problems or not, and whether or not it would cause any performance degradation at normal times when the feature is unused. |
New type with NaN-like behaviorAlternatively, more in line with what @jkotas suggested, here is a struct that uses operator overloads to behave like a normal UInt64 but with a flag that makes it operate similar to Double.NaN, and allows the overflow flag to be checked once at the end. The following also includes the typecast operator for implicit typecast to public readonly struct SafeUInt64
{
public readonly UInt64 Value;
public readonly bool Overflow;
public SafeUInt64(UInt64 inValue, bool inOverflow)
{
this.Value = inValue;
this.Overflow = inOverflow;
}
public static SafeUInt64 operator *(SafeUInt64 a, SafeUInt64 b)
{
unchecked {
if (System.Runtime.Intrinsics.X86.Bmi2.X64.IsSupported)
{
uint64 low;
bool ovf = System.Runtime.Intrinsics.X86.Bmi2.X64.MultiplyNoFlags(a.Value, b.Value, &low) != 0;
return new SafeUInt64(low, ovf | a.Overflow | b.Overflow);
}
UInt64 av = a.Value;
UInt64 bv = b.Value;
return new SafeUInt64(av * bv, (bv != 0 && (av > (UInt64.MaxValue / bv))) | a.Overflow | b.Overflow);
}
}
public static SafeUInt64 operator +(SafeUInt64 a, SafeUInt64 b)
{
UInt64 av = a.Value;
UInt64 result = unchecked(a.Value + b.Value);
return new SafeUInt64(result, result < av | a.Overflow | b.Overflow);
}
public static SafeUInt64 operator +(SafeUInt64 a, UInt64 b)
{
UInt64 av = a.Value;
UInt64 result = unchecked(a.Value + b);
return new SafeUInt64(result, result < av | a.Overflow);
}
public static SafeUInt64 operator +(SafeUInt64 a, UInt64? b)
{
UInt64 av = a.Value;
UInt64 result = unchecked(a.Value + b.GetValueOrDefault());
return new SafeUInt64(result, result < av | a.Overflow | !b.HasValue);
}
public static implicit operator UInt64?(SafeUInt64 input)
{
return input.Overflow ? default(UInt64?) : input.Value;
}
} Example of usage: public static SafeUInt64 ExampleUsage(SafeUInt64 t, SafeUInt64 a, SafeUInt64 b)
{
SafeUInt64 x = a + b;
x = x + 12345;
SafeUInt64 z = x * t;
if (z.Overflow) DoSomething();
return z;
} Incidentally this version of a |
This is actually exactly how
This is likewise how |
You can express that using try/catch today. The only difference is performance. If performance is the only reason for introducing new API, we should always look at whether it is feasible to optimized the existing pattern in the JIT for a reasonable cost. What would it take for the JIT to optimize |
While I think it would be nice to optimize the pattern, I think it's also really counterintuitive to how you would typically write .NET code and how most .NET users think about exceptions, their side effects, etc. |
I agree, however we have prior art in |
Given the above, it would likely be good to open a new issue tracking any JIT work required around optimizing this pattern (or possibly updating the original post and the relevant labels on this issue). |
@tannergooding, @pgovind
What do you guys think of the idea of adding the following
TryMultiply
methods toSystem.UInt64
orSystem.Math
? They return null if the multiplication would overflow:These methods intentionally do not use
checked(a * b)
because they're intended for scenarios where the high cost of throwing exceptions is unacceptable.Maybe you know, and could answer, the question of whether my
TryMultiply
implementations above are the best-possible implementations. Because of gaps in my knowledge of Intel processors, I'm unsure whether the above are the best implementations. I wonder whether or not it would be beneficial to add a few more intrinsics/methods toSystem.Runtime.Intrinsics.X86
in order to gain the ability to read the carry and/or overflow flags -- if necessary. I don't know enough CPU detail to say whether or not the x86-64 carry and/or overflow flags should be used, versus whether the above implementations are already the best overall. Maybe someone with detailed CPU knowledge could answer this question.In any event, regardless of whether the above implementations are the best-possible, they're already at least good implementations that could be added to NETFW 5.x immediately, and later further optimized if necessary.
See also issue #13026 where @RobertBouillon suggests new CIL instructions. Although I like his idea, it's far more work than the alternative of
TryMultiply
etc methods such as the above. The aboveTryMultiply
etc methods could provide the desired functionality very soon, unlike the complexity and politics of creating new CIL instructions.The text was updated successfully, but these errors were encountered: