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

Refactor NonAscii bit mask usage #43537

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Refactor NonAscii bit mask usage #43537

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d81cf0f
Refactor NonAscii bit mask usage
eerhardt Oct 16, 2020
eb11e3b
Refactor code so the Vector128 is declared inline.
eerhardt Oct 17, 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
14 changes: 7 additions & 7 deletions src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Validation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,6 @@ internal static unsafe partial class Utf8Utility
byte* pInputBufferFinalPosAtWhichCanSafelyLoop = pFinalPosWhereCanReadDWordFromInputBuffer - 3 * sizeof(uint); // can safely read 4 DWORDs here
nuint trailingZeroCount;

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

do
{
// pInputBuffer is 32-bit aligned but not necessary 128-bit aligned, so we're
Expand All @@ -134,7 +130,7 @@ internal static unsafe partial class Utf8Utility
// within the all-ASCII vectorized code at the entry to this method).
if (AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian)
{
ulong mask = GetNonAsciiBytes(AdvSimd.LoadVector128(pInputBuffer), bitMask128);
ulong mask = GetNonAsciiBytes(AdvSimd.LoadVector128(pInputBuffer));
if (mask != 0)
{
trailingZeroCount = (nuint)BitOperations.TrailingZeroCount(mask) >> 2;
Expand Down Expand Up @@ -736,16 +732,20 @@ internal static unsafe partial class Utf8Utility
return pInputBuffer;
}

private static readonly Vector128<byte> s_NonAsciiBitMask128 = BitConverter.IsLittleEndian ?
Copy link
Member

Choose a reason for hiding this comment

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

I believe this was @carlossanlop's first approach and this was causing a PNSE in some platforms because since it is a static field it is initialized at startup... but I can't remember exactly the error he was seeing.

Copy link
Member

@jkotas jkotas Oct 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Also, this may have less desirable performance characteristics compared to inline initialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using this approach, the whole thing goes away when building for non-arm64 because the linker will remove the single method that uses it and then remove the whole static field.

Other approaches would be:

  • create it every time in the GetNonAsciiBytes method
  • refactor the calling method so the loops are inside the ‘Arm64.IsSupported’ checks, which might need to dupe a bit of code.

Unfortunately I don’t have an Arm64 machine to perf test this myself.

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 was getting the PNSE on .NET Framework x86 CI legs for Utf8String.Experimental, since this file gets compiled into the Utf8String OOB package.

I refactored to use the 2nd approach above. It didn't turn out terrible.

Vector128.Create((ushort)0x1001).AsByte() :
Vector128.Create((ushort)0x0110).AsByte();

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

Vector128<byte> mostSignificantBitIsSet = AdvSimd.ShiftRightArithmetic(value.AsSByte(), 7).AsByte();
Vector128<byte> extractedBits = AdvSimd.And(mostSignificantBitIsSet, bitMask128);
Vector128<byte> extractedBits = AdvSimd.And(mostSignificantBitIsSet, s_NonAsciiBitMask128);
extractedBits = AdvSimd.Arm64.AddPairwise(extractedBits, extractedBits);
return extractedBits.AsUInt64().ToScalar();
}
Expand Down