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

Assist JIT in eliminating bounds checks #81036

Merged
merged 3 commits into from
May 15, 2023
Merged

Assist JIT in eliminating bounds checks #81036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5aafefc
for \(.+!= [^ ]+\.Length
xtqqczze Jan 23, 2023
ce18ccf
while \(.+!= [^ ]+\.Length
xtqqczze Jan 23, 2023
36d4416
Remove additional bounds check
xtqqczze Mar 3, 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
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/Compiler/ReferenceSource/LinkAttributesParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ static string FormatCustomAttribute(CustomAttribute ca)
continue;

bool match = true;
for (int ii = 0; match && ii != args.Length; ++ii)
for (int ii = 0; match && ii < args.Length; ++ii)
{
//
// No candidates betterness, only exact matches are supported
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/Mono.Linker.Tests/Extensions/NiceIO.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ public bool Equals (NPath? p)
if (p._elements.Length != _elements.Length)
return false;

for (var i = 0; i != _elements.Length; i++)
for (var i = 0; i < _elements.Length; i++)
if (!string.Equals (p._elements[i], _elements[i], PathStringComparison))
return false;

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/BinderHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public static void ValidateBindArgument(DynamicMetaObject[] arguments, string pa
{
if (arguments != null) // null is treated as empty, so not invalid
{
for (int i = 0; i != arguments.Length; ++i)
for (int i = 0; i < arguments.Length; ++i)
{
ValidateBindArgument(arguments[i], $"{paramName}[{i}]");
}
Expand Down
2 changes: 1 addition & 1 deletion ...libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ public int Match(object[] givenParameters, IServiceProviderIsService serviceProv

public object CreateInstance(IServiceProvider provider)
{
for (int index = 0; index != _parameters.Length; index++)
for (int index = 0; index < _parameters.Length; index++)
{
if (_parameterValues[index] == null)
{
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Collections/src/System/Collections/Generic/SortedList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public SortedList(IDictionary<TKey, TValue> dictionary, IComparer<TKey>? compare
{
comparer = Comparer; // obtain default if this is null.
Array.Sort<TKey, TValue>(keys, values, comparer);
for (int i = 1; i != keys.Length; ++i)
for (int i = 1; i < keys.Length; ++i)
{
if (comparer.Compare(keys[i - 1], keys[i]) == 0)
{
Expand Down
2 changes: 1 addition & 1 deletion ...stem.Linq.Parallel/src/System/Linq/Parallel/Merging/AsynchronousChannelMergeEnumerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ private bool MoveNextSlowPath()
int firstChannelIndex = _channelIndex;

int currChannelIndex;
while ((currChannelIndex = _channelIndex) != _channels.Length)
while ((currChannelIndex = _channelIndex) < _channels.Length)
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
AsynchronousChannel<T> current = _channels[currChannelIndex];

Expand Down
2 changes: 1 addition & 1 deletion ...ystem.Linq.Parallel/src/System/Linq/Parallel/Merging/SynchronousChannelMergeEnumerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public override bool MoveNext()
}

// If the index has reached the end, we bail.
while (_channelIndex != _channels.Length)
while (_channelIndex < _channels.Length)
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
SynchronousChannel<T> current = _channels[_channelIndex];
Debug.Assert(current != null);
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ protected override int GetParsedValueLength(string value, int startIndex, object
int? maxAge = null;
bool persist = false;

while (idx != value.Length)
while (idx < value.Length)
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
// Skip OWS before semicolon.
while (idx != value.Length && IsOptionalWhiteSpace(value[idx])) ++idx;
while (idx < value.Length && IsOptionalWhiteSpace(value[idx])) ++idx;

if (idx == value.Length)
{
Expand All @@ -102,7 +102,7 @@ protected override int GetParsedValueLength(string value, int startIndex, object
++idx;

// Skip OWS after semicolon / before value.
while (idx != value.Length && IsOptionalWhiteSpace(value[idx])) ++idx;
while (idx < value.Length && IsOptionalWhiteSpace(value[idx])) ++idx;

// Get the parameter key length.
int tokenLength = HttpRuleParser.GetTokenLength(value, idx);
Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Number.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ private static bool TryParseNumber(ReadOnlySpan<byte> source, ref Number.NumberB

case Utf8Constants.Plus:
srcIndex++;
if (srcIndex == source.Length)
if (srcIndex >= source.Length)
{
bytesConsumed = 0;
return false;
Expand All @@ -61,7 +61,7 @@ private static bool TryParseNumber(ReadOnlySpan<byte> source, ref Number.NumberB
int maxDigitCount = digits.Length - 1;

// Throw away any leading zeroes
while (srcIndex != source.Length)
while (srcIndex < source.Length)
Copy link
Member

Choose a reason for hiding this comment

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

Do these need uint-casts to eliminate the bound checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case casts appear to regress bounds check elimination.

Perhaps this is unnecessary because srcIndex is a local variable that is zero-initialized?

runtime/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Number.cs

Line 34 in ce18ccf

int srcIndex = 0;

Copy link
Member

Choose a reason for hiding this comment

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

In this case casts appear to regress bounds check elimination.

Can you elaborate? I wouldn't expect this to remove the bounds check on source[srcIndex] without casting srcIndex to uint here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already do checks on source.Length before the loop. Adding uint casts seems to cause JIT to "forget" about these.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand what you mean. Can you share a sharplab.io example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's worth noting that SharpLab is .NET 7

@tannergooding For future reference, does the codegen from Compiler Explorer have the same optimizations as a local disassembly using .NET 8 nightly build.

Currently, .e.g. https://csharp.godbolt.org/z/cKhTfjvqd, seems to be using:
// crossgen2 8.0.0-preview.3.23152.99+8d2ddc45e8f6f2a726c2b91c1ba2b9f7208b1f3b

Copy link
Member

Choose a reason for hiding this comment

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

@xtqqczze for godbolt you need to pass -O or better --Ot argument to compiler to enable optimizations. godbolt uses crossgen (R2R) which is, on average, worse than what JIT emits (sharplab). Although, even sharplab's codegen is a bit synthetic since in the real world our JIT is tiered and Tier1 codegen might be different from what you see in sharplab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for godbolt you need to pass -O or better --Ot

@EgorBo Not seeing any differeences in codegen for this example: https://csharp.godbolt.org/z/cKhTfjvqd, is this expected?

Copy link
Member

@tannergooding tannergooding Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. However, its showing the crossgen code and so isn't strictly the same as what sharplab shows or what a user will see for Tier 1 codegen.

You can customize the instruction set targeted and I'd recommend using --instruction-set x86-x64-v3 as that targets AVX2, BMI1, BMI2, LZCNT, MOVBE, and FMA

You can customize the OS: --targetos windows (defaults to linux)
and the architecture: --targetarch arm64 (defaults to x64)

and in general pass in other options supported by crossgen (such as --Ot to optimize for speed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding Great help with your tips, I knew about the parameters but couldn't find a useful reference with a search engine.

{
c = source[srcIndex];
if (c != '0')
Expand All @@ -79,7 +79,7 @@ private static bool TryParseNumber(ReadOnlySpan<byte> source, ref Number.NumberB
int startIndexNonLeadingDigitsBeforeDecimal = srcIndex;

int hasNonZeroTail = 0;
while (srcIndex != source.Length)
while (srcIndex < source.Length)
{
c = source[srcIndex];
int value = (byte)(c - (byte)('0'));
Expand Down Expand Up @@ -134,7 +134,7 @@ private static bool TryParseNumber(ReadOnlySpan<byte> source, ref Number.NumberB
srcIndex++;
int startIndexDigitsAfterDecimal = srcIndex;

while (srcIndex != source.Length)
while (srcIndex < source.Length)
{
c = source[srcIndex];
int value = (byte)(c - (byte)('0'));
Expand Down Expand Up @@ -268,7 +268,7 @@ private static bool TryParseNumber(ReadOnlySpan<byte> source, ref Number.NumberB
// continue eating digits from there
srcIndex += 10;

while (srcIndex != source.Length)
while (srcIndex < source.Length)
{
c = source[srcIndex];
int value = (byte)(c - (byte)('0'));
Expand Down
2 changes: 1 addition & 1 deletion ...ies/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.TimeSpan.BigG.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ private static bool TryParseTimeSpanBigG(ReadOnlySpan<byte> source, out TimeSpan
{
int srcIndex = 0;
byte c = default;
while (srcIndex != source.Length)
while (srcIndex < source.Length)
{
c = source[srcIndex];
if (!(c == ' ' || c == '\t'))
Expand Down
2 changes: 1 addition & 1 deletion ...ibraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.TimeSpan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ private static bool TryParseTimeSpanFraction(ReadOnlySpan<byte> source, out uint
uint fraction = digit;
int digitCount = 1;

while (srcIndex != source.Length)
while (srcIndex < source.Length)
{
digit = source[srcIndex] - 48u; // '0'
if (digit > 9)
Expand Down
2 changes: 1 addition & 1 deletion .../System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.TimeSpanSplitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public bool TrySplitTimeSpan(ReadOnlySpan<byte> source, bool periodUsedToSeparat
byte c = default;

// Unlike many other data types, TimeSpan allow leading whitespace.
while (srcIndex != source.Length)
while (srcIndex < source.Length)
{
c = source[srcIndex];
if (!(c == ' ' || c == '\t'))
Expand Down
4 changes: 2 additions & 2 deletions src/tools/illink/test/Mono.Linker.Tests/Extensions/NiceIO.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ public bool Equals (NPath p)
if (p._elements.Length != _elements.Length)
return false;

for (var i = 0; i != _elements.Length; i++)
for (var i = 0; i < _elements.Length; i++)
if (!string.Equals (p._elements[i], _elements[i], PathStringComparison))
return false;

Expand Down Expand Up @@ -870,4 +870,4 @@ public enum DeleteMode
Normal,
Soft
}
}
}