Skip to content

Fix string.Split regression #130438

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

Open
hamarb123 wants to merge 15 commits into
base: main
Choose a base branch
from
+39 −25
Open

Fix string.Split regression #130438

Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4301275
Update String.Manipulation.cs
hamarb123 Jul 9, 2026
41dcd92
Optimize handling of last vector's worth on arm64
hamarb123 Jul 9, 2026
180c80d
Revert previous change
hamarb123 Jul 9, 2026
4e8d707
Add same opt to V256/V512 & fix it on xarch
hamarb123 Jul 9, 2026
9c9b01e
Fix (shouldn't affect benchmark run meaningfully)
hamarb123 Jul 9, 2026
4647df7
Update String.Manipulation.cs
hamarb123 Jul 9, 2026
d692d36
Test to see if using V64 on arm conditionally would improve perf further
hamarb123 Jul 9, 2026
39f111a
Fixes to V64 path
hamarb123 Jul 9, 2026
b855b40
Revert V64 logic
hamarb123 Jul 10, 2026
930d7f6
Manually elide bounds check that made its way in when we converted to…
hamarb123 Jul 10, 2026
7fd8a6e
Update String.Manipulation.cs
hamarb123 Jul 10, 2026
358b43d
Add comments & test out alternative fixup sequence for arm64
hamarb123 Jul 10, 2026
56a317d
Update String.Manipulation.cs
hamarb123 Jul 10, 2026
655680a
thanks github editor
hamarb123 Jul 10, 2026
c3f656f
Revert alternative enumeration test
hamarb123 Jul 10, 2026
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
64 changes: 39 additions & 25 deletions src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2171,7 +2171,7 @@ private static void MakeSeparatorListVectorized(ReadOnlySpan<char> sourceSpan, r
}
}

while ((uint)remaining.Length >= (uint)Vector512<ushort>.Count)
while ((uint)remaining.Length > (uint)Vector512<ushort>.Count)
{
Vector512<ushort> vector = Vector512.Create(remaining);
Vector512<byte> cmp = Vector512.Equals(vector, v1).AsByte() | Vector512.Equals(vector, v2).AsByte() | Vector512.Equals(vector, v3).AsByte();
Expand All @@ -2194,17 +2194,21 @@ private static void MakeSeparatorListVectorized(ReadOnlySpan<char> sourceSpan, r

// Handle the last chunk in a vectorized way also.
// We do a whole vector's worth again, but just mask out the bits we've already handled.
if (remaining.Length > 0)
// Note: when Avx512BW.IsSupported is false, we never enter the first block, and thus remaining.Length is always > 0.
if (!Avx512BW.IsSupported || remaining.Length > 0)
{
Vector512<ushort> vector = Vector512.Create(sourceSpanUInt16.Slice(sourceSpanUInt16.Length - Vector512<ushort>.Count));
Vector512<byte> cmp = Vector512.Equals(vector, v1).AsByte() | Vector512.Equals(vector, v2).AsByte() | Vector512.Equals(vector, v3).AsByte();
int finalIndex = sourceSpanUInt16.Length - Vector512<ushort>.Count;
ulong mask = cmp.ExtractMostSignificantBits() & 0x5555555555555555 & ~((1UL << (Vector512<byte>.Count - remaining.Length * sizeof(char))) - 1);
while (mask != 0)
if (cmp != Vector512<byte>.Zero)
{
uint bitPos = (uint)BitOperations.TrailingZeroCount(mask) / sizeof(char);
sepListBuilder.Append(finalIndex + (int)bitPos);
mask = BitOperations.ResetLowestSetBit(mask);
int finalIndex = sourceSpanUInt16.Length - Vector512<ushort>.Count;
ulong mask = cmp.ExtractMostSignificantBits() & 0x5555555555555555 & ~((1UL << (Vector512<byte>.Count - remaining.Length * sizeof(char))) - 1);
while (mask != 0)
{
uint bitPos = (uint)BitOperations.TrailingZeroCount(mask) / sizeof(char);
sepListBuilder.Append(finalIndex + (int)bitPos);
mask = BitOperations.ResetLowestSetBit(mask);
}
}
}
return;
Expand Down Expand Up @@ -2266,7 +2270,7 @@ private static void MakeSeparatorListVectorized(ReadOnlySpan<char> sourceSpan, r
}
}

while ((uint)remaining.Length >= (uint)Vector256<ushort>.Count)
while ((uint)remaining.Length > (uint)Vector256<ushort>.Count)
{
Vector256<ushort> vector = Vector256.Create(remaining);
Vector256<byte> cmp = Vector256.Equals(vector, v1).AsByte() | Vector256.Equals(vector, v2).AsByte() | Vector256.Equals(vector, v3).AsByte();
Expand All @@ -2289,17 +2293,21 @@ private static void MakeSeparatorListVectorized(ReadOnlySpan<char> sourceSpan, r

// Handle the last chunk in a vectorized way also.
// We do a whole vector's worth again, but just mask out the bits we've already handled.
if (remaining.Length > 0)
// Note: when Avx2.IsSupported is false, we never enter the first block, and thus remaining.Length is always > 0.
if (!Avx2.IsSupported || remaining.Length > 0)
{
Vector256<ushort> vector = Vector256.Create(sourceSpanUInt16.Slice(sourceSpanUInt16.Length - Vector256<ushort>.Count));
Vector256<byte> cmp = Vector256.Equals(vector, v1).AsByte() | Vector256.Equals(vector, v2).AsByte() | Vector256.Equals(vector, v3).AsByte();
int finalIndex = sourceSpanUInt16.Length - Vector256<ushort>.Count;
uint mask = cmp.ExtractMostSignificantBits() & 0x55555555 & ~((1u << (Vector256<byte>.Count - remaining.Length * sizeof(char))) - 1);
while (mask != 0)
if (cmp != Vector256<byte>.Zero)
{
uint bitPos = (uint)BitOperations.TrailingZeroCount(mask) / sizeof(char);
sepListBuilder.Append(finalIndex + (int)bitPos);
mask = BitOperations.ResetLowestSetBit(mask);
int finalIndex = sourceSpanUInt16.Length - Vector256<ushort>.Count;
uint mask = cmp.ExtractMostSignificantBits() & 0x55555555 & ~((1u << (Vector256<byte>.Count - remaining.Length * sizeof(char))) - 1);
while (mask != 0)
{
uint bitPos = (uint)BitOperations.TrailingZeroCount(mask) / sizeof(char);
sepListBuilder.Append(finalIndex + (int)bitPos);
mask = BitOperations.ResetLowestSetBit(mask);
}
}
}
return;
Expand Down Expand Up @@ -2361,7 +2369,7 @@ private static void MakeSeparatorListVectorized(ReadOnlySpan<char> sourceSpan, r
}
}

while ((uint)remaining.Length >= (uint)Vector128<ushort>.Count)
while ((uint)remaining.Length > (uint)Vector128<ushort>.Count)
{
Vector128<ushort> vector = Vector128.Create(remaining);
Vector128<byte> cmp = Vector128.Equals(vector, v1).AsByte() | Vector128.Equals(vector, v2).AsByte() | Vector128.Equals(vector, v3).AsByte();
Expand All @@ -2384,17 +2392,23 @@ private static void MakeSeparatorListVectorized(ReadOnlySpan<char> sourceSpan, r

// Handle the last chunk in a vectorized way also.
// We do a whole vector's worth again, but just mask out the bits we've already handled.
if (remaining.Length > 0)
// Note: when Sse.IsSupported is false, we never enter the first block, and thus remaining.Length is always > 0.
if (!Sse.IsSupported || remaining.Length > 0)
{
Vector128<ushort> vector = Vector128.Create(sourceSpanUInt16.Slice(sourceSpanUInt16.Length - Vector128<ushort>.Count));
Debug.Assert(sourceSpanUInt16.Length >= Vector128<ushort>.Count);
Vector128<ushort> vector = Vector128.LoadUnsafe(in MemoryMarshal.GetReference(sourceSpanUInt16), (uint)(sourceSpanUInt16.Length - Vector128<ushort>.Count));

@EgorBo EgorBo Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please don't replace safe code with unsafe

@hamarb123 hamarb123 Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was unsafe before the PR that introduced the regression. Part of the regression is likely the bounds check that exists here now.

@hamarb123 hamarb123 Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That PR removed a lot more unsafe than just this one spot, and we only need to add this one spot back to get no bounds checks - surely that is fine?

Vector128<byte> cmp = Vector128.Equals(vector, v1).AsByte() | Vector128.Equals(vector, v2).AsByte() | Vector128.Equals(vector, v3).AsByte();
int finalIndex = sourceSpanUInt16.Length - Vector128<ushort>.Count;
uint mask = cmp.ExtractMostSignificantBits() & 0x5555 & ~((1u << (Vector128<byte>.Count - remaining.Length * sizeof(char))) - 1);
while (mask != 0)
if (cmp != Vector128<byte>.Zero)
{
uint bitPos = (uint)BitOperations.TrailingZeroCount(mask) / sizeof(char);
sepListBuilder.Append(finalIndex + (int)bitPos);
mask = BitOperations.ResetLowestSetBit(mask);
cmp &= Vector128.Create(0x0101010101010101UL).AsByte();
int finalIndex = sourceSpanUInt16.Length - Vector128<ushort>.Count;
uint mask = cmp.ExtractMostSignificantBits() & 0x5555 & ~((1u << (Vector128<byte>.Count - remaining.Length * sizeof(char))) - 1);
while (mask != 0)
{
uint bitPos = (uint)BitOperations.TrailingZeroCount(mask) / sizeof(char);
sepListBuilder.Append(finalIndex + (int)bitPos);
mask = BitOperations.ResetLowestSetBit(mask);
}
}
}
return;
Expand Down
Loading