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

Removed JIT (uint) cast workaround to elide bound-checks in CoreLib #67448

Merged
merged 13 commits into from
Jul 12, 2022
Merged

Removed JIT (uint) cast workaround to elide bound-checks in CoreLib #67448

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cf4ccd4
Removed (uint) cast workaround in CoreLib, annotated missing places w…
gfoidl Apr 1, 2022
af9b2d6
Fixed the bug I introduced in TimeSpanParse.TimeSpanTokenizer
gfoidl Apr 1, 2022
19b8e00
Use uint-cast on both sided for clarity
gfoidl Apr 2, 2022
33cd7a1
Added missing namespace (oops)
gfoidl Apr 2, 2022
20842e6
Use uint-division in BitHelper as the JIT can produce more efficient …
gfoidl Apr 4, 2022
da31c0b
Don't have struct local in ValueStringBuilder due to hitting JIT opti…
gfoidl Apr 4, 2022
7e816a5
BitHelper with uint division and without Math.DivRem for even better …
gfoidl Apr 4, 2022
d9f4e01
BitHelper needs local Span due to regression in jit-diff otherwise
gfoidl Apr 4, 2022
034dd89
Fixed dumb copy & paste failure in BitHelper.IsMarked
gfoidl Apr 5, 2022
76d6d2a
Merge branch 'main' into remove-bound-check-hack
gfoidl May 23, 2022
186fb58
Added Debug.Asserts to BitHelper's method that take bitPosition
gfoidl Jul 11, 2022
6afa6c2
BitHelper: comment for workaround to tracking issue
gfoidl Jul 12, 2022
26f459f
Update src/libraries/System.Private.CoreLib/src/System/Collections/Ge…
gfoidl Jul 12, 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
24 changes: 18 additions & 6 deletions src/libraries/Common/src/System/Collections/Generic/BitHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;

namespace System.Collections.Generic
{
internal ref struct BitHelper
Expand All @@ -19,19 +21,29 @@ internal BitHelper(Span<int> span, bool clear)

internal void MarkBit(int bitPosition)
{
Copy link
Member

@tannergooding tannergooding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we add a Debug.Assert(bitPosition >= 0)? Same for the other two methods here.

The previous logic, would've done say -1 / 32 == 0 and now will do uint.MaxValue / 32 = 134217727

Copy link
Member

Choose a reason for hiding this comment

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

It might even be better to just keep this one "as is" since its really doing the same thing, just with more casts now (that is comparing uint to uint)

int bitArrayIndex = bitPosition / IntSize;
if ((uint)bitArrayIndex < (uint)_span.Length)
Debug.Assert(bitPosition >= 0);

uint bitArrayIndex = (uint)bitPosition / IntSize;

// Workaround for https://github.com/dotnet/runtime/issues/72004
Span<int> span = _span;
if (bitArrayIndex < (uint)span.Length)
{
_span[bitArrayIndex] |= (1 << (bitPosition % IntSize));
span[(int)bitArrayIndex] |= (1 << (int)((uint)bitPosition % IntSize));
}
}

internal bool IsMarked(int bitPosition)
{
int bitArrayIndex = bitPosition / IntSize;
Debug.Assert(bitPosition >= 0);

uint bitArrayIndex = (uint)bitPosition / IntSize;
Copy link
Member

@tannergooding tannergooding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Same for this one, its doing the same thing but via "more code" now. I'd also expect the JIT was already hoisting the span since its a field of a struct.

Copy link
Member

Choose a reason for hiding this comment

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

It is not the same thing. Unsigned division is a simple shift. Signed division is more complicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd also expect the JIT was already hoisting the span since its a field of a struct.

Unfortunately that's not the case*. Codegen was validated for this change.
* (at least when the PR was created, at the moment I don't have enough time to check again)

Copy link
Member

Choose a reason for hiding this comment

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

The operation is "the same" even if the codegen is different. If we're doing this specifically to take advantage of the codegen, there should be a comment on it.

More generally, we also need some assert that the input is definitely positive -or- should be taking the input as uint and expecting the caller handle the validation/assertions. The current usages all look to be but its also always the output of GetIndex so without the corresponding outer checks, it'd be pretty easy for someone to accidentally pass in -1 here from some refactoring.

If we're going to be making the method less readable, we should include a comment explaining why these tricks are being used (just as all the current cases have a small // uint cast, per https://github.com/dotnet/runtime/issues/10596 or similar comment). Ideally the issue tracks ways to improve the JIT handling where we can't improve it ourselves via signature changes, locals, or other tweaks.


// Workaround for https://github.com/dotnet/runtime/issues/72004
Span<int> span = _span;
Copy link
Member

Choose a reason for hiding this comment

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

This one can use a tracking issue. The JIT should be smart enough to make it unnecessary to cache the Span here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #72004 for this.

return
(uint)bitArrayIndex < (uint)_span.Length &&
(_span[bitArrayIndex] & (1 << (bitPosition % IntSize))) != 0;
bitArrayIndex < (uint)span.Length &&
(span[(int)bitArrayIndex] & (1 << ((int)((uint)bitPosition % IntSize)))) != 0;
}

/// <summary>How many ints must be allocated to represent n bits. Returns (n+31)/32, but avoids overflow.</summary>
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Boolean.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public bool TryFormat(Span<char> destination, out int charsWritten)
{
if (m_value)
{
if ((uint)destination.Length > 3) // uint cast, per https://github.com/dotnet/runtime/issues/10596
if (destination.Length > 3)
{
ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
MemoryMarshal.Write<ulong>(MemoryMarshal.AsBytes(destination), ref true_val);
Expand All @@ -109,7 +109,7 @@ public bool TryFormat(Span<char> destination, out int charsWritten)
}
else
{
if ((uint)destination.Length > 4)
if (destination.Length > 4)
{
ulong fals_val = BitConverter.IsLittleEndian ? 0x73006C00610046ul : 0x460061006C0073ul; // "Fals"
MemoryMarshal.Write<ulong>(MemoryMarshal.AsBytes(destination), ref fals_val);
Expand Down
3 changes: 1 addition & 2 deletions src/libraries/System.Private.CoreLib/src/System/Buffers/StandardFormat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,7 @@ internal int Format(Span<char> destination)
int count = 0;
char symbol = Symbol;

if (symbol != default &&
(uint)destination.Length == FormatStringLength) // to eliminate bounds checks
if (symbol != default && destination.Length == FormatStringLength)
{
destination[0] = symbol;
count = 1;
Expand Down
4 changes: 2 additions & 2 deletions ...ies/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Boolean.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public static bool TryFormat(bool value, Span<byte> destination, out int bytesWr
{
// This check can't be performed earlier because we need to throw if an invalid symbol is
// provided, even if the buffer is too small.
if ((uint)4 >= (uint)destination.Length)
if (destination.Length <= 4)
{
goto BufferTooSmall;
}
Expand All @@ -75,7 +75,7 @@ public static bool TryFormat(bool value, Span<byte> destination, out int bytesWr
}
else if (symbol == 'l')
{
if ((uint)4 >= (uint)destination.Length)
if (destination.Length <= 4)
{
goto BufferTooSmall;
}
Expand Down
4 changes: 1 addition & 3 deletions ...ries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Date.L.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ public static partial class Utf8Formatter
//
private static bool TryFormatDateTimeL(DateTime value, Span<byte> destination, out int bytesWritten)
{
// Writing the check in this fashion elides all bounds checks on 'buffer'
// for the remainder of the method.
if ((uint)28 >= (uint)destination.Length)
if (destination.Length <= 28)
{
bytesWritten = 0;
return false;
Expand Down
4 changes: 1 addition & 3 deletions ...ries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Date.R.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ public static partial class Utf8Formatter
//
private static bool TryFormatDateTimeR(DateTime value, Span<byte> destination, out int bytesWritten)
{
// Writing the check in this fashion elides all bounds checks on 'buffer'
// for the remainder of the method.
if ((uint)28 >= (uint)destination.Length)
if (destination.Length <= 28)
{
bytesWritten = 0;
return false;
Expand Down
35 changes: 17 additions & 18 deletions src/libraries/System.Private.CoreLib/src/System/Char.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ public static bool IsControl(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
Copy link
Member

Choose a reason for hiding this comment

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

These ones seem to be touching code "stylistically", we probably should exclude it.

Copy link
Member Author

Choose a reason for hiding this comment

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

W/o the superfluos paranthesis it's easier to read, thus I changed it.

Copy link
Member

@tannergooding tannergooding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't disagree, but its also a "stylistic" change and so makes it harder to review the thing we actually care about. The typical PR policy is to not include such things and leave them to a separate PR, especially when its the only or primary change to a given file.

some operand swapping, etc. was done to get proper codegen. By reverting these to only have a mechanical remove of the uint-casts, we give up some (runtime) benefits.

We should have issues tracking these. We really shouldn't be getting codegen differences between x < y and y > x, especially for the cases where one input is constant.

{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -606,7 +606,7 @@ public static bool IsDigit(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -626,7 +626,7 @@ public static bool IsLetter(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -647,7 +647,7 @@ public static bool IsLetterOrDigit(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -667,8 +667,7 @@ public static bool IsLower(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}

if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -710,7 +709,7 @@ public static bool IsNumber(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -743,7 +742,7 @@ public static bool IsPunctuation(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -788,7 +787,7 @@ public static bool IsSeparator(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -813,7 +812,7 @@ public static bool IsSurrogate(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -845,7 +844,7 @@ public static bool IsSymbol(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -865,7 +864,7 @@ public static bool IsUpper(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -885,7 +884,7 @@ public static bool IsWhiteSpace(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -911,7 +910,7 @@ public static UnicodeCategory GetUnicodeCategory(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -935,7 +934,7 @@ public static double GetNumericValue(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -957,7 +956,7 @@ public static bool IsHighSurrogate(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -979,7 +978,7 @@ public static bool IsLowSurrogate(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -996,7 +995,7 @@ public static bool IsSurrogatePair(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down
5 changes: 3 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ public ref T this[int index]
public void Append(T item)
{
int pos = _pos;
if ((uint)pos < (uint)_span.Length)
Span<T> span = _span;
gfoidl marked this conversation as resolved.
Show resolved Hide resolved
if ((uint)pos < (uint)span.Length)
{
_span[pos] = item;
span[pos] = item;
_pos = pos + 1;
}
else
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Decimal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ public static int[] GetBits(decimal d)
/// <exception cref="ArgumentException">The destination span was not long enough to store the binary representation.</exception>
public static int GetBits(decimal d, Span<int> destination)
{
if ((uint)destination.Length <= 3)
if (destination.Length <= 3)
{
ThrowHelper.ThrowArgumentException_DestinationTooShort();
}
Expand All @@ -623,7 +623,7 @@ public static int GetBits(decimal d, Span<int> destination)
/// <returns>true if the decimal's binary representation was written to the destination; false if the destination wasn't long enough.</returns>
public static bool TryGetBits(decimal d, Span<int> destination, out int valuesWritten)
{
if ((uint)destination.Length <= 3)
if (destination.Length <= 3)
{
valuesWritten = 0;
return false;
Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeParse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4668,7 +4668,7 @@ private static bool ParseFormatR(ReadOnlySpan<char> source, ref ParsingInfo pars
// Tue, 03 Jan 2017 08:08:05 GMT

// The format is exactly 29 characters.
if ((uint)source.Length != 29)
if (source.Length != 29)
{
result.SetBadDateTimeFailure();
return false;
Expand Down Expand Up @@ -4865,7 +4865,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
// 2017-06-12T05:30:45.7680000-7:00 (special-case of one-digit offset hour)
// 2017-06-12T05:30:45.7680000-07:00

if ((uint)source.Length < 27 ||
if (source.Length < 27 ||
source[4] != '-' ||
source[7] != '-' ||
source[10] != 'T' ||
Expand Down Expand Up @@ -4986,7 +4986,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
return false;
}

if ((uint)source.Length > 27)
if (source.Length > 27)
{
char offsetChar = source[27];
switch (offsetChar)
Expand All @@ -5004,7 +5004,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
case '-':
int offsetHours, colonIndex;

if ((uint)source.Length == 33)
if (source.Length == 33)
{
uint oh1 = (uint)(source[28] - '0'), oh2 = (uint)(source[29] - '0');

Expand All @@ -5017,7 +5017,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
offsetHours = (int)(oh1 * 10 + oh2);
colonIndex = 30;
}
else if ((uint)source.Length == 32) // special-case allowed for compat: only one offset hour digit
else if (source.Length == 32) // special-case allowed for compat: only one offset hour digit
{
offsetHours = source[28] - '0';

Expand Down
Loading