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

AdvSimd support for System.Text.Unicode.Utf8Utility.GetPointerToFirstInvalidByte #38653

Merged
merged 19 commits into from
Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
38e528b
AdvSimd support for System.Text.Unicode.Utf8Utility.GetPointerToFirst…
carlossanlop Jul 1, 2020
db7a4b1
Move comment to the top, add shims.
carlossanlop Jul 1, 2020
3daf5cc
Little endian checks
carlossanlop Jul 1, 2020
3fb9b55
Use custom MoveMask method for AdvSimd
carlossanlop Jul 10, 2020
3a340b7
Address suggestions to improve the AdvSimdMoveMask method
carlossanlop Jul 10, 2020
45bb8dd
Define initialMask outside MoveMask method
carlossanlop Jul 10, 2020
d4e5497
UInt64 in Arm64MoveMask
carlossanlop Jul 13, 2020
d761124
Add unit test case to verify intrinsics improvement
carlossanlop Jul 14, 2020
af42e59
Avoid casting to smaller integer type
carlossanlop Jul 14, 2020
bb07819
Typo and comment
carlossanlop Jul 14, 2020
55dd236
Use ShiftRightArithmetic instead of CompareEqual + And.
carlossanlop Jul 14, 2020
46bbf26
Use AddPairwise version of GetNotAsciiBytes
carlossanlop Jul 14, 2020
6f4cca9
Add missing shims causing Linux build to fail
carlossanlop Jul 15, 2020
b2d3705
Simplify GetNonAsciiBytes to only one AddPairwise call, shorter bitmask
carlossanlop Jul 16, 2020
8536a8d
Respect data type returned by masking method
carlossanlop Jul 16, 2020
a45fe16
Address suggestions - assert trailingzerocount and bring back uint mask
carlossanlop Jul 17, 2020
b09e92c
Trailing zeroes in AdvSimd need to be divided by 4, and total number …
carlossanlop Jul 17, 2020
5c3cee2
Avoid declaring static field which causes PNSE in Utf8String.Experime…
carlossanlop Jul 17, 2020
d9dd878
Prefer using nuint for BitConverter.TrailingZeroCount
carlossanlop Jul 20, 2020
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 @@ -4,6 +4,8 @@
using System.Diagnostics;
using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;
using System.Runtime.Intrinsics.X86;

#if SYSTEM_PRIVATE_CORELIB
Expand Down Expand Up @@ -117,22 +119,35 @@ internal static unsafe partial class Utf8Utility
// the alignment check consumes at most a single DWORD.)

byte* pInputBufferFinalPosAtWhichCanSafelyLoop = pFinalPosWhereCanReadDWordFromInputBuffer - 3 * sizeof(uint); // can safely read 4 DWORDs here
uint mask;
nuint trailingZeroCount;

Vector128<byte> bitMask128 = BitConverter.IsLittleEndian ?
Vector128.Create((ushort)0x1001).AsByte() :
Vector128.Create((ushort)0x0110).AsByte();

do
{
if (Sse2.IsSupported)
// pInputBuffer is 32-bit aligned but not necessary 128-bit aligned, so we're
// going to perform an unaligned load. We don't necessarily care about aligning
// this because we pessimistically assume we'll encounter non-ASCII data at some
// point in the not-too-distant future (otherwise we would've stayed entirely
// within the all-ASCII vectorized code at the entry to this method).
if (AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian)
{
// pInputBuffer is 32-bit aligned but not necessary 128-bit aligned, so we're
// going to perform an unaligned load. We don't necessarily care about aligning
// this because we pessimistically assume we'll encounter non-ASCII data at some
// point in the not-too-distant future (otherwise we would've stayed entirely
// within the all-ASCII vectorized code at the entry to this method).

mask = (uint)Sse2.MoveMask(Sse2.LoadVector128((byte*)pInputBuffer));
ulong mask = GetNonAsciiBytes(AdvSimd.LoadVector128(pInputBuffer), bitMask128);
if (mask != 0)
{
trailingZeroCount = (nuint)BitOperations.TrailingZeroCount(mask) >> 2;
goto LoopTerminatedEarlyDueToNonAsciiData;
}
}
else if (Sse2.IsSupported)
{
uint mask = (uint)Sse2.MoveMask(Sse2.LoadVector128(pInputBuffer));
if (mask != 0)
{
goto Sse2LoopTerminatedEarlyDueToNonAsciiData;
trailingZeroCount = (nuint)BitOperations.TrailingZeroCount(mask);
goto LoopTerminatedEarlyDueToNonAsciiData;
}
}
else
Expand All @@ -153,19 +168,20 @@ internal static unsafe partial class Utf8Utility

continue; // need to perform a bounds check because we might be running out of data

Sse2LoopTerminatedEarlyDueToNonAsciiData:
LoopTerminatedEarlyDueToNonAsciiData:
// x86 can only be little endian, while ARM can be big or little endian
// so if we reached this label we need to check both combinations are supported
Debug.Assert((AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian) || Sse2.IsSupported);

Debug.Assert(BitConverter.IsLittleEndian);
Debug.Assert(Sse2.IsSupported);

// The 'mask' value will have a 0 bit for each ASCII byte we saw and a 1 bit
// for each non-ASCII byte we saw. We can count the number of ASCII bytes,
// for each non-ASCII byte we saw. trailingZeroCount will count the number of ASCII bytes,
// bump our input counter by that amount, and resume processing from the
// "the first byte is no longer ASCII" portion of the main loop.
// We should not expect a total number of zeroes equal or larger than 16.
Copy link
Member

Choose a reason for hiding this comment

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

We should not expect a total number of zeroes equal or larger than 16. [](start = 27, length = 70)

minor: I would re-phrase the comment to "Make sure that pInputBuffer is not advanced by more than 15 positions."

Copy link
Member Author

Choose a reason for hiding this comment

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

If that's ok with you, I would like to address this in another PR so that I don't reset the CI (it's taking a really long time to finish).
I have another similar TODO from the other PR (modifying a comment). We can collect similar requests and address them separately.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. No problem.

Debug.Assert(trailingZeroCount < 16);

Debug.Assert(mask != 0);

pInputBuffer += BitOperations.TrailingZeroCount(mask);
pInputBuffer += trailingZeroCount;
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
if (pInputBuffer > pFinalPosWhereCanReadDWordFromInputBuffer)
{
goto ProcessRemainingBytesSlow;
Expand Down Expand Up @@ -719,5 +735,19 @@ internal static unsafe partial class Utf8Utility
scalarCountAdjustment = tempScalarCountAdjustment;
return pInputBuffer;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static ulong GetNonAsciiBytes(Vector128<byte> value, Vector128<byte> bitMask128)
{
if (!AdvSimd.Arm64.IsSupported || !BitConverter.IsLittleEndian)
{
throw new PlatformNotSupportedException();
}

carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Vector128<byte> mostSignificantBitIsSet = AdvSimd.ShiftRightArithmetic(value.AsSByte(), 7).AsByte();
Vector128<byte> extractedBits = AdvSimd.And(mostSignificantBitIsSet, bitMask128);
extractedBits = AdvSimd.Arm64.AddPairwise(extractedBits, extractedBits);
return extractedBits.AsUInt64().ToScalar();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ internal readonly struct Vector64<T>

internal static class Vector128
{
public static Vector128<long> Create(long value) => throw new PlatformNotSupportedException();
public static Vector128<short> Create(short value) => throw new PlatformNotSupportedException();
public static Vector128<ulong> Create(ulong value) => throw new PlatformNotSupportedException();
public static Vector128<ushort> Create(ushort value) => throw new PlatformNotSupportedException();
public static Vector128<ulong> CreateScalarUnsafe(ulong value) => throw new PlatformNotSupportedException();
public static Vector128<byte> AsByte<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();
public static Vector128<short> AsInt16<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();
public static Vector128<sbyte> AsSByte<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();
public static Vector128<ushort> AsUInt16<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();
public static Vector128<uint> AsUInt32<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();
public static Vector128<ulong> AsUInt64<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();
Expand Down