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

Improve CopySign performance for integer types #90970

Closed
wants to merge 10 commits into from
Closed

Improve CopySign performance for integer types #90970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
271014a
Fix #77579
LEI-Hongfaan Aug 23, 2023
630c704
Merge branch 'main' of https://github.com/LEI-Hongfaan/dotnet-runtime
LEI-Hongfaan Aug 23, 2023
e8cee13
Code style: Put constants on the right side.
LEI-Hongfaan Aug 23, 2023
1dd6e3d
Use Math.ThrowNegateTwosCompOverflow() to throw OverflowException.
LEI-Hongfaan Aug 23, 2023
12274ee
Use a better bit operation for SignZeroToOne.
LEI-Hongfaan Aug 24, 2023
725bd61
Code Style: Remove some unnecessary `unchecked`
LEI-Hongfaan Aug 24, 2023
fe5b230
Avoid expensive imul in Int128.CopySign for 32-bit
LEI-Hongfaan Aug 29, 2023
64044bd
Update src/libraries/System.Private.CoreLib/src/System/Int64.cs
LEI-Hongfaan Nov 8, 2023
3e650ba
Rename SignZeroToOne to SignOrOneIfZero.
LEI-Hongfaan Nov 8, 2023
3c1be6d
Merge branch 'main' of https://github.com/LEI-Hongfaan/dotnet-runtime
LEI-Hongfaan Nov 8, 2023
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
35 changes: 23 additions & 12 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 @@ -1290,22 +1290,33 @@ public static Int128 Clamp(Int128 value, Int128 min, Int128 max)
/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
public static Int128 CopySign(Int128 value, Int128 sign)
{
Int128 absValue = value;

if (IsNegative(absValue))
{
absValue = -absValue;
}

if (IsPositive(sign))
#if TARGET_32BIT
Int128 t = value;
if (Int128.IsPositive(sign))
Copy link
Member

Choose a reason for hiding this comment

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

Int128. is unnecessary as we are in the Int128 type

I'm notably not really a fan of this additional complexity to maintain two copies of the code. 32-bit is already extremely pessimized for Int128 and I don't think it's worth the additional micro-optimizations here. Just have the singular code path that is consistent with the other types instead.

{
if (IsNegative(absValue))
if (Int128.IsNegative(t))
{
Math.ThrowNegateTwosCompOverflow();
t = -t;
if (Int128.IsNegative(t))
{
Math.ThrowNegateTwosCompOverflow();
}
}
return absValue;
return t;
}
return -absValue;
if (Int128.IsPositive(t))
{
t = -t;
}
return t;
#else
// Int128.MinValue == value && 0 <= sign
if ((long)value._upper == long.MinValue && value._lower == 0 && (long)sign._upper >= 0)
Copy link
Member

Choose a reason for hiding this comment

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

I'd just do this the simple/readable way as:

Suggested change
if ((long)value._upper == long.MinValue && value._lower == 0 && (long)sign._upper >= 0)
if ((value == Int128.MinValue) && IsPositive(sign))

The JIT should take care of inlining the comparison and IsPositive check to give good codegen.

{
Math.ThrowNegateTwosCompOverflow();
}
return value * Math.SignOrOneIfZero((long)value._upper ^ (long)sign._upper);
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not a fan of this name nor of the cost for multiplication (which increases significantly as the size of the type does).

I think the entire logic here can be simplified down to:

if (SameSign(value, sign))
{
    return value;
}
else if (value == Int128.MinValue)
{
    ThrowNegateTwosCompOverflow();
}

return -value;

where you have:

bool SameSign(Int128 left, Int128 right)
{
    // All positive values have the most significant bit clear
    // All negative values have the most significant bit set
    //
    // (x ^ y) produces 0 if the bits are the same and 1 if they
    // differ. Therefore, (x ^ y) produces a positive value if the
    // signs are the same and a negative value if they differ.

    return (long)(left._upper ^ right._upper) >= 0;
}

This gives you up to the same 2 comparisons you currently have, but reduces the bit toggle to simply a two's complement operation (negation), which is far cheaper.

Copy link
Member

Choose a reason for hiding this comment

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

The SameSign operation can even be optimized for 32-bit platforms by only comparing the upper half of _upper, which is a much more reasonable platform specific optimization and keeps it heavily isolated away to only where needed.

#endif
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down
19 changes: 3 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Int16.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,24 +631,11 @@ public static short Log2(short value)
/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
public static short CopySign(short value, short sign)
{
short absValue = value;

if (absValue < 0)
{
absValue = (short)(-absValue);
}

if (sign >= 0)
if (value == short.MinValue && sign >= 0)
{
if (absValue < 0)
{
Math.ThrowNegateTwosCompOverflow();
}

return absValue;
Math.ThrowNegateTwosCompOverflow();
}

return (short)(-absValue);
return (short)(value * Math.SignOrOneIfZero(value ^ sign));
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down
19 changes: 3 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Int32.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,24 +666,11 @@ public static int Log2(int value)
/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
public static int CopySign(int value, int sign)
{
int absValue = value;

if (absValue < 0)
{
absValue = -absValue;
}

if (sign >= 0)
if (value == int.MinValue && sign >= 0)
{
if (absValue < 0)
{
Math.ThrowNegateTwosCompOverflow();
}

return absValue;
Math.ThrowNegateTwosCompOverflow();
}

return -absValue;
return value * Math.SignOrOneIfZero(value ^ sign);
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down
20 changes: 4 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Int64.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,26 +661,14 @@ public static long Log2(long value)
public static long Clamp(long value, long min, long max) => Math.Clamp(value, min, max);

/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static long CopySign(long value, long sign)
{
long absValue = value;

if (absValue < 0)
if (value == long.MinValue && sign >= 0)
{
absValue = -absValue;
Math.ThrowNegateTwosCompOverflow();
}

if (sign >= 0)
{
if (absValue < 0)
{
Math.ThrowNegateTwosCompOverflow();
}

return absValue;
}

return -absValue;
return value * Math.SignOrOneIfZero(value ^ sign);
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down
19 changes: 3 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/IntPtr.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,24 +674,11 @@ public static nint Log2(nint value)
/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
public static nint CopySign(nint value, nint sign)
{
nint absValue = value;

if (absValue < 0)
{
absValue = -absValue;
}

if (sign >= 0)
if (value == nint.MinValue && sign >= 0)
{
if (absValue < 0)
{
Math.ThrowNegateTwosCompOverflow();
}

return absValue;
Math.ThrowNegateTwosCompOverflow();
}

return -absValue;
return value * Math.SignOrOneIfZero(value ^ sign);
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down
21 changes: 21 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Math.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,6 +1468,27 @@ public static int Sign(float value)
throw new ArithmeticException(SR.Arithmetic_NaN);
}

// Helper functions for CopySign
// >= 0: returns 1
// < 0: returns -1
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int SignOrOneIfZero(int value)
{
return (value >> (32 - 2)) | 1;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int SignOrOneIfZero(nint value)
{
return (int)(value >> (8 * nint.Size - 2)) | 1;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int SignOrOneIfZero(long value)
{
return (int)(value >> (64 - 2)) | 1;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static decimal Truncate(decimal d)
{
Expand Down
19 changes: 3 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/SByte.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,24 +592,11 @@ public static sbyte Log2(sbyte value)
/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
public static sbyte CopySign(sbyte value, sbyte sign)
{
sbyte absValue = value;

if (absValue < 0)
{
absValue = (sbyte)(-absValue);
}

if (sign >= 0)
if (value == sbyte.MinValue && sign >= 0)
{
if (absValue < 0)
{
Math.ThrowNegateTwosCompOverflow();
}

return absValue;
Math.ThrowNegateTwosCompOverflow();
}

return (sbyte)(-absValue);
return (sbyte)(value * Math.SignOrOneIfZero(value ^ sign));
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down