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

Simplify/optimize Int128 comparison operators #101848

Merged
merged 4 commits into from
May 9, 2024
Merged

Simplify/optimize Int128 comparison operators #101848

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
70e457c
Optimize In128 comparison operators
lilinus Apr 27, 2024
848c12b
Merge branch 'main' into optimize-int128-comparisons
lilinus May 3, 2024
75a3579
Document how Int128 comparison operators work
lilinus May 5, 2024
0630152
Merge branch 'main' into optimize-int128-comparisons
lilinus May 5, 2024
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
44 changes: 8 additions & 36 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 @@ -1043,57 +1043,29 @@ public static Int128 Log2(Int128 value)
/// <inheritdoc cref="IComparisonOperators{TSelf, TOther, TResult}.op_LessThan(TSelf, TOther)" />
public static bool operator <(Int128 left, Int128 right)
{
if (IsNegative(left) == IsNegative(right))
{
return (left._upper < right._upper)
|| ((left._upper == right._upper) && (left._lower < right._lower));
}
else
{
return IsNegative(left);
}
return ((long)left._upper < (long)right._upper)
Copy link
Member

Choose a reason for hiding this comment

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

A comment (in the code) explaining why this is valid would be beneficial.

Actually brute forcing this for some struct S16 { byte _lower; byte _upper; } is pretty trivial to do, but so is describing why this works out given the two's complement representation. -- Notably this comes down to the fact that all negative values have the sign bit set, so if lhs.upper < rhs.upper in signed form, then either lhs/rhs have different signs, and the single comparison was sufficient or they have the same sign and the same was true.

There may be other optimizations available here as well... In particular, comparisons x cmp y in general are done as if x - y and then setting CPU flags, without actually producing the result. That general premise is why the "most optimal" pattern is actually cmp lower; sbb upper; setcc

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 tried my best documenting it with comments.

|| ((left._upper == right._upper) && (left._lower < right._lower));
}

/// <inheritdoc cref="IComparisonOperators{TSelf, TOther, TResult}.op_LessThanOrEqual(TSelf, TOther)" />
public static bool operator <=(Int128 left, Int128 right)
{
if (IsNegative(left) == IsNegative(right))
{
return (left._upper < right._upper)
|| ((left._upper == right._upper) && (left._lower <= right._lower));
}
else
{
return IsNegative(left);
}
return ((long)left._upper < (long)right._upper)
|| ((left._upper == right._upper) && (left._lower <= right._lower));
}

/// <inheritdoc cref="IComparisonOperators{TSelf, TOther, TResult}.op_GreaterThan(TSelf, TOther)" />
public static bool operator >(Int128 left, Int128 right)
{
if (IsNegative(left) == IsNegative(right))
{
return (left._upper > right._upper)
|| ((left._upper == right._upper) && (left._lower > right._lower));
}
else
{
return IsNegative(right);
}
return ((long)left._upper > (long)right._upper)
|| ((left._upper == right._upper) && (left._lower > right._lower));
}

/// <inheritdoc cref="IComparisonOperators{TSelf, TOther, TResult}.op_GreaterThanOrEqual(TSelf, TOther)" />
public static bool operator >=(Int128 left, Int128 right)
{
if (IsNegative(left) == IsNegative(right))
{
return (left._upper > right._upper)
|| ((left._upper == right._upper) && (left._lower >= right._lower));
}
else
{
return IsNegative(right);
}
return ((long)left._upper > (long)right._upper)
|| ((left._upper == right._upper) && (left._lower >= right._lower));
}

//
Expand Down
24 changes: 24 additions & 0 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Int128Tests.GenericMath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,12 @@ public static void op_GreaterThanTest()
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(MaxValue, 1));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(MinValue, 1));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(NegativeOne, 1));

Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(Zero, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(One, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(MaxValue, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(MinValue, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThan(NegativeOne, NegativeOne));
}

[Fact]
Expand All @@ -1022,6 +1028,12 @@ public static void op_GreaterThanOrEqualTest()
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(MaxValue, 1));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(MinValue, 1));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(NegativeOne, 1));

Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(Zero, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(One, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(MaxValue, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(MinValue, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_GreaterThanOrEqual(NegativeOne, NegativeOne));
}

[Fact]
Expand All @@ -1032,6 +1044,12 @@ public static void op_LessThanTest()
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(MaxValue, 1));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(MinValue, 1));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(NegativeOne, 1));

Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(Zero, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(One, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(MaxValue, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(MinValue, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThan(NegativeOne, NegativeOne));
}

[Fact]
Expand All @@ -1042,6 +1060,12 @@ public static void op_LessThanOrEqualTest()
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(MaxValue, 1));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(MinValue, 1));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(NegativeOne, 1));

Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(Zero, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(One, NegativeOne));
Assert.False(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(MaxValue, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(MinValue, NegativeOne));
Assert.True(ComparisonOperatorsHelper<Int128, Int128, bool>.op_LessThanOrEqual(NegativeOne, NegativeOne));
}

//
Expand Down
Loading