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 6 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
18 changes: 4 additions & 14 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,12 @@ 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))
// 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.

{
if (IsNegative(absValue))
{
Math.ThrowNegateTwosCompOverflow();
}
return absValue;
Math.ThrowNegateTwosCompOverflow();
}
return -absValue;
return value * Math.SignZeroToOne((long)value._upper ^ (long)sign._upper);
}

/// <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.SignZeroToOne(value ^ sign));
Copy link
Member

Choose a reason for hiding this comment

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

Multiplication is much more expensive on older CPUs and may significantly regress performance.

It's not necessarily "cheap" on modern CPUs either (still a minimum of 3-4 cycles for 32-bit, and 3-6 cycles for 64-bit) and while reduction of branches can be goodness, it might hurt things overall and hinder JIT optimizations for some constants.

}

/// <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.SignZeroToOne(value ^ sign);
}

/// <inheritdoc cref="INumber{TSelf}.Max(TSelf, TSelf)" />
Expand Down
19 changes: 3 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 @@ -663,24 +663,11 @@ public static long Log2(long value)
/// <inheritdoc cref="INumber{TSelf}.CopySign(TSelf, TSelf)" />
public static long CopySign(long value, long sign)
LEI-Hongfaan marked this conversation as resolved.
Show resolved Hide resolved
{
long absValue = value;

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

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

return absValue;
Math.ThrowNegateTwosCompOverflow();
}

return -absValue;
return value * Math.SignZeroToOne(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.SignZeroToOne(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)]
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
internal static int SignZeroToOne(int value)
Copy link
Member

Choose a reason for hiding this comment

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

This name isn't "clear" IMO and needs something that makes its meaning clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SignOrOneIfZero?

{
return (value >> (32 - 2)) | 1;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
internal static int SignZeroToOne(nint value)
{
return (int)(value >> (8 * nint.Size - 2)) | 1;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
internal static int SignZeroToOne(long value)
{
return (int)(value >> (64 - 2)) | 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Casting to int32 will introduce unnecessary truncate&extend on 64 bit platform, but saves a move on 32 bit platform.

Also note that we typically put these methods in System.Numerics.BitOperations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think returning an int (Int32) for Sign is reasonable, because it is the common return type for sign functions. Also, using a shorter bit length than long (Int64) may provide optimization opportunities for subsequent multiplications. Regarding where to put these methods, I think Sign and Abs are closely related, so it makes sense to group them together. Besides, the original code already referenced some throw helper methods from Math, so moving them to System.Numerics.BitOperations would make the code more scattered.

Copy link
Member

Choose a reason for hiding this comment

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

In such primitive function, one instruction counts. The helper isn't exposed to public, so it should be most performant form. In case when needed, you can use #if TARGETS_64BIT to provide different implementation.

We need to understand the performance better.


[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.SignZeroToOne(value ^ sign));
}

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