From e7c83023b54457c67c6182015137d59bbd821592 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 12 Sep 2022 11:59:20 -0700 Subject: [PATCH 1/3] Simplify and fix the Int128 *, /, and % logic --- .../src/System/Int128.cs | 118 ++++-------------- .../tests/System/Int128Tests.cs | 10 ++ .../tests/System/UInt128Tests.cs | 10 ++ 3 files changed, 44 insertions(+), 94 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Int128.cs b/src/libraries/System.Private.CoreLib/src/System/Int128.cs index d66655060f3e1..9043c391ca087 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Int128.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Int128.cs @@ -1172,17 +1172,13 @@ public static Int128 Log2(Int128 value) UInt128 result = (UInt128)(left) / (UInt128)(right); - if (result == 0U) - { - sign = 0; - } - else if (sign != 0) + if (sign != 0) { result = ~result + 1U; } return new Int128( - result.Upper | sign, + result.Upper, result.Lower ); } @@ -1227,36 +1223,8 @@ public static Int128 Log2(Int128 value) /// public static Int128 operator %(Int128 left, Int128 right) { - // We simplify the logic here by just doing unsigned modulus on the - // one's complement representation and then taking the correct sign. - - ulong sign = (left._upper ^ right._upper) & (1UL << 63); - - if (IsNegative(left)) - { - left = ~left + 1U; - } - - if (IsNegative(right)) - { - right = ~right + 1U; - } - - UInt128 result = (UInt128)(left) % (UInt128)(right); - - if (result == 0U) - { - sign = 0; - } - else if (sign != 0) - { - result = ~result + 1U; - } - - return new Int128( - result.Upper | sign, - result.Lower - ); + Int128 quotient = left / right; + return left - (quotient * right); } // @@ -1273,76 +1241,38 @@ public static Int128 Log2(Int128 value) /// public static Int128 operator *(Int128 left, Int128 right) { - // We simplify the logic here by just doing unsigned multiplication on - // the one's complement representation and then taking the correct sign. - - ulong sign = (left._upper ^ right._upper) & (1UL << 63); - - if (IsNegative(left)) - { - left = ~left + 1U; - } - - if (IsNegative(right)) - { - right = ~right + 1U; - } - - UInt128 result = (UInt128)(left) * (UInt128)(right); - - if (result == 0U) - { - sign = 0; - } - else if (sign != 0) - { - result = ~result + 1U; - } - - return new Int128( - result.Upper | sign, - result.Lower - ); + // Multiplication is the same for signed and unsigned provided the "upper" bits aren't needed + return (Int128)((UInt128)(left) * (UInt128)(right)); } /// public static Int128 operator checked *(Int128 left, Int128 right) { - // We simplify the logic here by just doing unsigned multiplication on - // the one's complement representation and then taking the correct sign. - - ulong sign = (left._upper ^ right._upper) & (1UL << 63); - - if (IsNegative(left)) - { - left = ~left + 1U; - } + Int128 upper = BigMul(left, right, out Int128 lower); - if (IsNegative(right)) + if (((upper != 0) || (lower < 0)) && (~upper != 0)) { - right = ~right + 1U; - } + // The upper bits can safely be either Zero or AllBitsSet + // where the former represents a positive value and the + // latter a negative value. + // + // However, when the upper bits are Zero, we also need to + // confirm the lower bits are positive, otherwise we have + // a positive value greater than MaxValue and should throw - UInt128 result = checked((UInt128)(left) * (UInt128)(right)); - - if ((long)(result.Upper) < 0) - { ThrowHelper.ThrowOverflowException(); } - if (result == 0U) - { - sign = 0; - } - else if (sign != 0) - { - result = ~result + 1U; - } + return lower; + } - return new Int128( - result.Upper | sign, - result.Lower - ); + internal static Int128 BigMul(Int128 left, Int128 right, out Int128 lower) + { + // This follows the same logic as is used in `long Math.BigMul(long, long, out long)` + + UInt128 upper = UInt128.BigMul((UInt128)(left), (UInt128)(right), out UInt128 ulower); + lower = (Int128)(ulower); + return (Int128)(upper) - ((left >> 63) & right) - ((right >> 63) & left); } // diff --git a/src/libraries/System.Runtime/tests/System/Int128Tests.cs b/src/libraries/System.Runtime/tests/System/Int128Tests.cs index 036872f0ddc1a..d0dee7dc57cf7 100644 --- a/src/libraries/System.Runtime/tests/System/Int128Tests.cs +++ b/src/libraries/System.Runtime/tests/System/Int128Tests.cs @@ -458,5 +458,15 @@ public static void TestNegativeNumberParsingWithHyphen() CultureInfo ci = CultureInfo.GetCultureInfo("sv-SE"); Assert.Equal(-15868, Int128.Parse("-15868", NumberStyles.Number, ci)); } + + [Fact] + public static void Runtime75416() + { + Int128 a = (Int128.MaxValue - 10) * +100; + Assert.Equal(a, -1100); + + Int128 b = (Int128.MaxValue - 10) * -100; + Assert.Equal(b, +1100); + } } } diff --git a/src/libraries/System.Runtime/tests/System/UInt128Tests.cs b/src/libraries/System.Runtime/tests/System/UInt128Tests.cs index a255a0444960d..57a2f42ce09cd 100644 --- a/src/libraries/System.Runtime/tests/System/UInt128Tests.cs +++ b/src/libraries/System.Runtime/tests/System/UInt128Tests.cs @@ -441,5 +441,15 @@ public static void TryFormat(UInt128 i, string format, IFormatProvider provider, Assert.Equal(expected.ToLowerInvariant(), new string(actual)); } } + + [Fact] + public static void Runtime75416() + { + UInt128 a = (UInt128Tests_GenericMath.Int128MaxValue - 10u) * +100u; + Assert.Equal(a, (UInt128)(Int128)(-1100)); + + UInt128 b = (UInt128Tests_GenericMath.Int128MaxValue - 10u) * (UInt128)(Int128)(-100); + Assert.Equal(b, 1100u); + } } } From da892418dc9926195beb92394616f305db31000e Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 12 Sep 2022 13:23:44 -0700 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: tfenise --- src/libraries/System.Private.CoreLib/src/System/Int128.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Int128.cs b/src/libraries/System.Private.CoreLib/src/System/Int128.cs index 9043c391ca087..f909be28f1261 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Int128.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Int128.cs @@ -1250,7 +1250,7 @@ public static Int128 Log2(Int128 value) { Int128 upper = BigMul(left, right, out Int128 lower); - if (((upper != 0) || (lower < 0)) && (~upper != 0)) + if (((upper != 0) || (lower < 0)) && ((~upper != 0) || (lower >= 0))) { // The upper bits can safely be either Zero or AllBitsSet // where the former represents a positive value and the @@ -1272,7 +1272,7 @@ internal static Int128 BigMul(Int128 left, Int128 right, out Int128 lower) UInt128 upper = UInt128.BigMul((UInt128)(left), (UInt128)(right), out UInt128 ulower); lower = (Int128)(ulower); - return (Int128)(upper) - ((left >> 63) & right) - ((right >> 63) & left); + return (Int128)(upper) - ((left >> 127) & right) - ((right >> 127) & left); } // From 667bd2b6dd30d899cd942f5e9a8f4fd4cd839210 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Tue, 13 Sep 2022 08:27:16 -0700 Subject: [PATCH 3/3] Fixing some code comments --- src/libraries/System.Private.CoreLib/src/System/Int128.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Int128.cs b/src/libraries/System.Private.CoreLib/src/System/Int128.cs index f909be28f1261..1208320880810 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Int128.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Int128.cs @@ -1156,7 +1156,7 @@ public static Int128 Log2(Int128 value) } // We simplify the logic here by just doing unsigned division on the - // one's complement representation and then taking the correct sign. + // two's complement representation and then taking the correct sign. ulong sign = (left._upper ^ right._upper) & (1UL << 63); @@ -1259,6 +1259,11 @@ public static Int128 Log2(Int128 value) // However, when the upper bits are Zero, we also need to // confirm the lower bits are positive, otherwise we have // a positive value greater than MaxValue and should throw + // + // Likewise, when the upper bits are AllBitsSet, we also + // need to confirm the lower bits are negative, otherwise + // we have a large negative value less than MinValue and + // should throw. ThrowHelper.ThrowOverflowException(); }