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.

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

Remove ternary workaround for string.IsNullOrEmpty #63095

Merged
merged 8 commits into from
Jun 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -428,22 +428,19 @@ public sealed override Delegate[] GetInvocationList()
return del;
}

// Force inline as the true/false ternary takes it above ALWAYS_INLINE size even though the asm ends up smaller
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2)
{
// Test d2 first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (d2 is null)
{
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (d1 is null) ? true : false;
return d1 is null;
}

return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
return ReferenceEquals(d2, d1) || d2.Equals((object?)d1);

Copy link
Member Author

Choose a reason for hiding this comment

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

This is quite a similar pattern to the one addressed by this PR. It's better to confirm there's no codegen regression separately.

}

// Force inline as the true/false ternary takes it above ALWAYS_INLINE size even though the asm ends up smaller
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator !=(MulticastDelegate? d1, MulticastDelegate? d2)
{
Expand All @@ -453,8 +450,7 @@ public sealed override Delegate[] GetInvocationList()
// so it can become a simple test
if (d2 is null)
{
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (d1 is null) ? false : true;
return d1 is not null;
}

return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1);
return !ReferenceEquals(d2, d1) && !d2.Equals(d1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime;
using System.Runtime.Serialization;
using System.Runtime.CompilerServices;
using System.Runtime.Serialization;

using Internal.Runtime.CompilerServices;

Expand Down Expand Up @@ -109,22 +109,19 @@ public override sealed int GetHashCode()
}
}

// Force inline as the true/false ternary takes it above ALWAYS_INLINE size even though the asm ends up smaller
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2)
{
// Test d2 first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (d2 is null)
{
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (d1 is null) ? true : false;
return d1 is null;
}

return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
}

// Force inline as the true/false ternary takes it above ALWAYS_INLINE size even though the asm ends up smaller
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator !=(MulticastDelegate? d1, MulticastDelegate? d2)
{
Expand All @@ -134,8 +131,7 @@ public override sealed int GetHashCode()
// so it can become a simple test
if (d2 is null)
{
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (d1 is null) ? false : true;
return d1 is not null;
}

return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -556,45 +556,44 @@ private static void InsertionSort(Span<T> keys)
// - The floating-point comparisons here assume no NaNs, which is valid only because the sorting routines
// themselves special-case NaN with a pre-pass that ensures none are present in the values being sorted
// by moving them all to the front first and then sorting the rest.
// - The `? true : false` is to work-around poor codegen: https://github.com/dotnet/runtime/issues/37904#issuecomment-644180265.
// - These are duplicated here rather than being on a helper type due to current limitations around generic inlining.

[MethodImpl(MethodImplOptions.AggressiveInlining)] // compiles to a single comparison or method call
private static bool LessThan(ref T left, ref T right)
{
if (typeof(T) == typeof(byte)) return (byte)(object)left < (byte)(object)right ? true : false;
if (typeof(T) == typeof(sbyte)) return (sbyte)(object)left < (sbyte)(object)right ? true : false;
if (typeof(T) == typeof(ushort)) return (ushort)(object)left < (ushort)(object)right ? true : false;
if (typeof(T) == typeof(short)) return (short)(object)left < (short)(object)right ? true : false;
if (typeof(T) == typeof(uint)) return (uint)(object)left < (uint)(object)right ? true : false;
if (typeof(T) == typeof(int)) return (int)(object)left < (int)(object)right ? true : false;
if (typeof(T) == typeof(ulong)) return (ulong)(object)left < (ulong)(object)right ? true : false;
if (typeof(T) == typeof(long)) return (long)(object)left < (long)(object)right ? true : false;
if (typeof(T) == typeof(nuint)) return (nuint)(object)left < (nuint)(object)right ? true : false;
if (typeof(T) == typeof(nint)) return (nint)(object)left < (nint)(object)right ? true : false;
if (typeof(T) == typeof(float)) return (float)(object)left < (float)(object)right ? true : false;
if (typeof(T) == typeof(double)) return (double)(object)left < (double)(object)right ? true : false;
if (typeof(T) == typeof(Half)) return (Half)(object)left < (Half)(object)right ? true : false;
return left.CompareTo(right) < 0 ? true : false;
if (typeof(T) == typeof(byte)) return (byte)(object)left < (byte)(object)right;
Copy link
Member

Choose a reason for hiding this comment

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

why these if’s and not using switch expressions for these in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

The IL patterns of typeof(T) == constant and (the same type)(object)value are specially recognized by JIT and will be reduced to a nop. Other patterns are harder to analysis.

Copy link

Choose a reason for hiding this comment

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

Also, typeof() isn't a constant value and can't be used in a switch statement/expression.

if (typeof(T) == typeof(sbyte)) return (sbyte)(object)left < (sbyte)(object)right;
if (typeof(T) == typeof(ushort)) return (ushort)(object)left < (ushort)(object)right;
if (typeof(T) == typeof(short)) return (short)(object)left < (short)(object)right;
if (typeof(T) == typeof(uint)) return (uint)(object)left < (uint)(object)right;
if (typeof(T) == typeof(int)) return (int)(object)left < (int)(object)right;
if (typeof(T) == typeof(ulong)) return (ulong)(object)left < (ulong)(object)right;
if (typeof(T) == typeof(long)) return (long)(object)left < (long)(object)right;
if (typeof(T) == typeof(nuint)) return (nuint)(object)left < (nuint)(object)right;
if (typeof(T) == typeof(nint)) return (nint)(object)left < (nint)(object)right;
if (typeof(T) == typeof(float)) return (float)(object)left < (float)(object)right;
if (typeof(T) == typeof(double)) return (double)(object)left < (double)(object)right;
if (typeof(T) == typeof(Half)) return (Half)(object)left < (Half)(object)right;
return left.CompareTo(right) < 0;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)] // compiles to a single comparison or method call
private static bool GreaterThan(ref T left, ref T right)
{
if (typeof(T) == typeof(byte)) return (byte)(object)left > (byte)(object)right ? true : false;
if (typeof(T) == typeof(sbyte)) return (sbyte)(object)left > (sbyte)(object)right ? true : false;
if (typeof(T) == typeof(ushort)) return (ushort)(object)left > (ushort)(object)right ? true : false;
if (typeof(T) == typeof(short)) return (short)(object)left > (short)(object)right ? true : false;
if (typeof(T) == typeof(uint)) return (uint)(object)left > (uint)(object)right ? true : false;
if (typeof(T) == typeof(int)) return (int)(object)left > (int)(object)right ? true : false;
if (typeof(T) == typeof(ulong)) return (ulong)(object)left > (ulong)(object)right ? true : false;
if (typeof(T) == typeof(long)) return (long)(object)left > (long)(object)right ? true : false;
if (typeof(T) == typeof(nuint)) return (nuint)(object)left > (nuint)(object)right ? true : false;
if (typeof(T) == typeof(nint)) return (nint)(object)left > (nint)(object)right ? true : false;
if (typeof(T) == typeof(float)) return (float)(object)left > (float)(object)right ? true : false;
if (typeof(T) == typeof(double)) return (double)(object)left > (double)(object)right ? true : false;
if (typeof(T) == typeof(Half)) return (Half)(object)left > (Half)(object)right ? true : false;
return left.CompareTo(right) > 0 ? true : false;
if (typeof(T) == typeof(byte)) return (byte)(object)left > (byte)(object)right;
if (typeof(T) == typeof(sbyte)) return (sbyte)(object)left > (sbyte)(object)right;
if (typeof(T) == typeof(ushort)) return (ushort)(object)left > (ushort)(object)right;
if (typeof(T) == typeof(short)) return (short)(object)left > (short)(object)right;
if (typeof(T) == typeof(uint)) return (uint)(object)left > (uint)(object)right;
if (typeof(T) == typeof(int)) return (int)(object)left > (int)(object)right;
if (typeof(T) == typeof(ulong)) return (ulong)(object)left > (ulong)(object)right;
if (typeof(T) == typeof(long)) return (long)(object)left > (long)(object)right;
if (typeof(T) == typeof(nuint)) return (nuint)(object)left > (nuint)(object)right;
if (typeof(T) == typeof(nint)) return (nint)(object)left > (nint)(object)right;
if (typeof(T) == typeof(float)) return (float)(object)left > (float)(object)right;
if (typeof(T) == typeof(double)) return (double)(object)left > (double)(object)right;
if (typeof(T) == typeof(Half)) return (Half)(object)left > (Half)(object)right;
return left.CompareTo(right) > 0;
}
}

Expand Down Expand Up @@ -1054,44 +1053,43 @@ private static void InsertionSort(Span<TKey> keys, Span<TValue> values)
// - The floating-point comparisons here assume no NaNs, which is valid only because the sorting routines
// themselves special-case NaN with a pre-pass that ensures none are present in the values being sorted
// by moving them all to the front first and then sorting the rest.
// - The `? true : false` is to work-around poor codegen: https://github.com/dotnet/runtime/issues/37904#issuecomment-644180265.
// - These are duplicated here rather than being on a helper type due to current limitations around generic inlining.

[MethodImpl(MethodImplOptions.AggressiveInlining)] // compiles to a single comparison or method call
private static bool LessThan(ref TKey left, ref TKey right)
{
if (typeof(TKey) == typeof(byte)) return (byte)(object)left < (byte)(object)right ? true : false;
if (typeof(TKey) == typeof(sbyte)) return (sbyte)(object)left < (sbyte)(object)right ? true : false;
if (typeof(TKey) == typeof(ushort)) return (ushort)(object)left < (ushort)(object)right ? true : false;
if (typeof(TKey) == typeof(short)) return (short)(object)left < (short)(object)right ? true : false;
if (typeof(TKey) == typeof(uint)) return (uint)(object)left < (uint)(object)right ? true : false;
if (typeof(TKey) == typeof(int)) return (int)(object)left < (int)(object)right ? true : false;
if (typeof(TKey) == typeof(ulong)) return (ulong)(object)left < (ulong)(object)right ? true : false;
if (typeof(TKey) == typeof(long)) return (long)(object)left < (long)(object)right ? true : false;
if (typeof(TKey) == typeof(nuint)) return (nuint)(object)left < (nuint)(object)right ? true : false;
if (typeof(TKey) == typeof(nint)) return (nint)(object)left < (nint)(object)right ? true : false;
if (typeof(TKey) == typeof(float)) return (float)(object)left < (float)(object)right ? true : false;
if (typeof(TKey) == typeof(double)) return (double)(object)left < (double)(object)right ? true : false;
if (typeof(TKey) == typeof(Half)) return (Half)(object)left < (Half)(object)right ? true : false;
if (typeof(TKey) == typeof(byte)) return (byte)(object)left < (byte)(object)right;
if (typeof(TKey) == typeof(sbyte)) return (sbyte)(object)left < (sbyte)(object)right;
if (typeof(TKey) == typeof(ushort)) return (ushort)(object)left < (ushort)(object)right;
if (typeof(TKey) == typeof(short)) return (short)(object)left < (short)(object)right;
if (typeof(TKey) == typeof(uint)) return (uint)(object)left < (uint)(object)right;
if (typeof(TKey) == typeof(int)) return (int)(object)left < (int)(object)right;
if (typeof(TKey) == typeof(ulong)) return (ulong)(object)left < (ulong)(object)right;
if (typeof(TKey) == typeof(long)) return (long)(object)left < (long)(object)right;
if (typeof(TKey) == typeof(nuint)) return (nuint)(object)left < (nuint)(object)right;
if (typeof(TKey) == typeof(nint)) return (nint)(object)left < (nint)(object)right;
if (typeof(TKey) == typeof(float)) return (float)(object)left < (float)(object)right;
if (typeof(TKey) == typeof(double)) return (double)(object)left < (double)(object)right;
if (typeof(TKey) == typeof(Half)) return (Half)(object)left < (Half)(object)right;
return left.CompareTo(right) < 0 ? true : false;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)] // compiles to a single comparison or method call
private static bool GreaterThan(ref TKey left, ref TKey right)
{
if (typeof(TKey) == typeof(byte)) return (byte)(object)left > (byte)(object)right ? true : false;
if (typeof(TKey) == typeof(sbyte)) return (sbyte)(object)left > (sbyte)(object)right ? true : false;
if (typeof(TKey) == typeof(ushort)) return (ushort)(object)left > (ushort)(object)right ? true : false;
if (typeof(TKey) == typeof(short)) return (short)(object)left > (short)(object)right ? true : false;
if (typeof(TKey) == typeof(uint)) return (uint)(object)left > (uint)(object)right ? true : false;
if (typeof(TKey) == typeof(int)) return (int)(object)left > (int)(object)right ? true : false;
if (typeof(TKey) == typeof(ulong)) return (ulong)(object)left > (ulong)(object)right ? true : false;
if (typeof(TKey) == typeof(long)) return (long)(object)left > (long)(object)right ? true : false;
if (typeof(TKey) == typeof(nuint)) return (nuint)(object)left > (nuint)(object)right ? true : false;
if (typeof(TKey) == typeof(nint)) return (nint)(object)left > (nint)(object)right ? true : false;
if (typeof(TKey) == typeof(float)) return (float)(object)left > (float)(object)right ? true : false;
if (typeof(TKey) == typeof(double)) return (double)(object)left > (double)(object)right ? true : false;
if (typeof(TKey) == typeof(Half)) return (Half)(object)left > (Half)(object)right ? true : false;
if (typeof(TKey) == typeof(byte)) return (byte)(object)left > (byte)(object)right;
if (typeof(TKey) == typeof(sbyte)) return (sbyte)(object)left > (sbyte)(object)right;
if (typeof(TKey) == typeof(ushort)) return (ushort)(object)left > (ushort)(object)right;
if (typeof(TKey) == typeof(short)) return (short)(object)left > (short)(object)right;
if (typeof(TKey) == typeof(uint)) return (uint)(object)left > (uint)(object)right;
if (typeof(TKey) == typeof(int)) return (int)(object)left > (int)(object)right;
if (typeof(TKey) == typeof(ulong)) return (ulong)(object)left > (ulong)(object)right;
if (typeof(TKey) == typeof(long)) return (long)(object)left > (long)(object)right;
if (typeof(TKey) == typeof(nuint)) return (nuint)(object)left > (nuint)(object)right;
if (typeof(TKey) == typeof(nint)) return (nint)(object)left > (nint)(object)right;
if (typeof(TKey) == typeof(float)) return (float)(object)left > (float)(object)right;
if (typeof(TKey) == typeof(double)) return (double)(object)left > (double)(object)right;
if (typeof(TKey) == typeof(Half)) return (Half)(object)left > (Half)(object)right;
return left.CompareTo(right) > 0 ? true : false;
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/libraries/System.Private.CoreLib/src/System/DateTime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1085,8 +1085,7 @@ public static bool IsLeapYear(int year)
}
if ((year & 3) != 0) return false;
if ((year & 15) == 0) return true;
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (uint)year % 25 != 0 ? true : false;
return (uint)year % 25 != 0;
}

// Constructs a DateTime from a string. The string must specify a
Expand Down
8 changes: 2 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/Delegate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,31 +93,27 @@ public abstract partial class Delegate : ICloneable, ISerializable
return newDelegate;
}

// Force inline as the true/false ternary takes it above ALWAYS_INLINE size even though the asm ends up smaller
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(Delegate? d1, Delegate? d2)
{
// Test d2 first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (d2 is null)
{
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (d1 is null) ? true : false;
return d1 is null;
}

return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
}

// Force inline as the true/false ternary takes it above ALWAYS_INLINE size even though the asm ends up smaller
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator !=(Delegate? d1, Delegate? d2)
{
// Test d2 first to allow branch elimination when inlined for not null checks (!= null)
// so it can become a simple test
if (d2 is null)
{
// return true/false not the test result https://github.com/dotnet/runtime/issues/4207
return (d1 is null) ? false : true;
return d1 is not null;
}

return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,7 @@ internal static bool EqualsIgnoreCase(ref char charA, ref char charB, int length
return false; // not exact match, and first input isn't in [A-Za-z]
}

// The ternary operator below seems redundant but helps RyuJIT generate more optimal code.
// See https://github.com/dotnet/runtime/issues/4207.
return (valueA == (valueB | 0x20u)) ? true : false;
return valueA == (valueB | 0x20u);
}

Debug.Assert(length == 0);
Expand Down
Loading