Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify and fix the Int128 *, /, and % logic #75470

Merged
merged 3 commits into from
Sep 13, 2022
Merged

Simplify and fix the Int128 *, /, and % logic #75470

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7c8302
Simplify and fix the Int128 *, /, and % logic
tannergooding Sep 12, 2022
da89241
Apply suggestions from code review
tannergooding Sep 12, 2022
667bd2b
Fixing some code comments
tannergooding Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 24 additions & 94 deletions src/libraries/System.Private.CoreLib/src/System/Int128.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
{
result = ~result + 1U;
}

return new Int128(
result.Upper | sign,
result.Upper,
result.Lower
);
}
Expand Down Expand Up @@ -1227,36 +1223,8 @@ public static Int128 Log2(Int128 value)
/// <inheritdoc cref="IModulusOperators{TSelf, TOther, TResult}.op_Modulus(TSelf, TOther)" />
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);
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
}

//
Expand All @@ -1273,76 +1241,38 @@ public static Int128 Log2(Int128 value)
/// <inheritdoc cref="IMultiplyOperators{TSelf, TOther, TResult}.op_Multiply(TSelf, TOther)" />
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));
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
}

/// <inheritdoc cref="IMultiplyOperators{TSelf, TOther, TResult}.op_CheckedMultiply(TSelf, TOther)" />
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) || (lower >= 0)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Something about this logic is hard to parse but I can't figure out a better way to structure it.

{
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
tannergooding marked this conversation as resolved.
Show resolved Hide resolved

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 >> 127) & right) - ((right >> 127) & left);
}

//
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Runtime/tests/System/Int128Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
10 changes: 10 additions & 0 deletions src/libraries/System.Runtime/tests/System/UInt128Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}