-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.TranscodeToUtf8 #39041
AdvSimd support for System.Text.Unicode.Utf8Utility.TranscodeToUtf8 #39041
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to this area: @tannergooding |
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Show resolved
Hide resolved
Add AdvSimd equivalent operation to TestZ.
8a19f17
to
f596eaf
Compare
The following unit tests hit the code added in this PR: System.Runtime:
System.Text.Encoding:
System.Utf8String.Experimental:
|
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Outdated
Show resolved
Hide resolved
…ss and use MaxPairwise instead
Update: I finally got WSL in my ARM device to execute the unit tests. The unit tests that hit these changes are listed here: #39041 (comment) After I ran built everything and ran the unit tests in my other PR, I switch to the branch from this PR, and since both of my branches had the same base commit, I only had to quickly build clr.corelib+libs.pretest, then run the unit tests. The System.Text.Encoding
|
@echesakovMSFT - What should be an equivalent of |
@kunalspathak No What are you trying to do? If you want to store the lower 8 bytes to memory then StoreSelectedScalar(ptr, utf16Data.AsUInt64(), 0); If you need the value in a GP registers then utf16Data.AsUInt64().GetElement(0) or AdvSimd.Extract(utf16Data.AsUInt64(), 0) |
Just an equivalent of that SSE2 instruction. Didn't realize it returns possibleNonAsciiQWord = utf16Data.AsUInt64().GetElement(0);
// or
possibleNonAsciiQWord = utf16Data.AsUInt64().ToScalar(); |
Fixed the unit test failures with the last two commits: System.Runtime
System.Text.Encoding
System.Utf8String.Experimental
|
The Alpine ARM64 leg finally finished and it hit a bunch of UTF8 encoding errors, which I will investigate: |
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Outdated
Show resolved
Hide resolved
@echesakovMSFT the tests are passing after applying your suggestion. Thank you! System.Text.Json
System.Formats.Asn1``` calope@calopearm:~/runtime$ dotnet msbuild /t:Test /p:Configuration=Release /p:TargetArchitecture=arm64 src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj Microsoft (R) Build Engine version 16.7.0-preview-20309-02+d6862eb21 for .NET Copyright (C) Microsoft Corporation. All rights reserved.
|
We had a conversation offline and we decided we will collect the perf data after merging. @kunalspathak @jeffhandley |
|
if (AdvSimd.IsSupported) | ||
{ | ||
Vector64<byte> lower = AdvSimd.ExtractNarrowingSaturateUnsignedLower(utf16Data); | ||
AdvSimd.StoreSelectedScalar((uint*)pOutputBuffer, lower.AsUInt32(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should comment on this line that (uint*)pOutputBuffer
is unaligned.
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Show resolved
Hide resolved
if (AdvSimd.IsSupported) | ||
{ | ||
Vector64<byte> lower = AdvSimd.ExtractNarrowingSaturateUnsignedLower(utf16Data); | ||
AdvSimd.StoreSelectedScalar((uint*)pOutputBuffer, lower.AsUInt32(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment saying (uint*)pOutputBuffer
is unaligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's ok, I can add this to a list of TODOs for the next PR we open for intrinsics changes (there are things that the FC may have to address later). The CI is taking a really long time and I would like to get this merged today. I would like to avoid resetting the CI.
I posted the perf results in this comment: #39050 (comment) |
I think this is ready, @kunalspathak @echesakovMSFT @GrabYourPitchforks I already have two sign-offs. I'll wait one more hour in case you have any final comments, otherwise I'll merge. Note that the CI failure is unrelated to my change. I ran that CI leg multiple times and it's happening consistently and is pre-existing. I also found it in other PRs. Libraries build Windows_NT allConfigurations x64 Release
|
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Transcoding.cs
Show resolved
Hide resolved
…otnet#39041) * AdvSimd support for System.Text.Unicode.Utf8Utility.TranscodeToUtf8 * Readd using to prevent build failure. Add AdvSimd equivalent operation to TestZ. * Inverted condition * Address IsSupported order, improve use ExtractNarrowingSaturated usage * Rename source to result, second argument utf16Data * Improve CompareTest * Add shims causing failures in Linux * Use unsigned version of ExtractNarrowingSaturate, avoid using MinAcross and use MaxPairwise instead * Missing support check for Sse2.X64 * Add missing case for AdvSimd * Use MinPairwise for short
…#39738) * AdvSimd support for System.Text.Unicode.Utf16Utility.GetPointerToFirstInvalidChar (#39050) * AdvSimd support for System.Text.Unicode.Utf16Utility.GetPointerToFirstInvalidChar * Move using directive outside #if. Improve Arm64MoveMask. * Change overloads * UIn64 in Arm64MoveMask * Build error implicit conversion fix * Rename method and use simpler version * Use ShiftRightArithmetic instead of CompareEqual + And. * Remove unnecessary comment * Add missing shims causing Linux build to fail * AdvSimd support for System.Text.Unicode.Utf8Utility.TranscodeToUtf8 (#39041) * AdvSimd support for System.Text.Unicode.Utf8Utility.TranscodeToUtf8 * Readd using to prevent build failure. Add AdvSimd equivalent operation to TestZ. * Inverted condition * Address IsSupported order, improve use ExtractNarrowingSaturated usage * Rename source to result, second argument utf16Data * Improve CompareTest * Add shims causing failures in Linux * Use unsigned version of ExtractNarrowingSaturate, avoid using MinAcross and use MaxPairwise instead * Missing support check for Sse2.X64 * Add missing case for AdvSimd * Use MinPairwise for short * AdvSimd support for System.Text.Unicode.Utf8Utility.GetPointerToFirstInvalidByte (#38653) * AdvSimd support for System.Text.Unicode.Utf8Utility.GetPointerToFirstInvalidByte * Move comment to the top, add shims. * Little endian checks * Use custom MoveMask method for AdvSimd * Address suggestions to improve the AdvSimdMoveMask method * Define initialMask outside MoveMask method * UInt64 in Arm64MoveMask * Add unit test case to verify intrinsics improvement * Avoid casting to smaller integer type * Typo and comment * Use ShiftRightArithmetic instead of CompareEqual + And. Remove test case causing other unit tests to fail. * Use AddPairwise version of GetNotAsciiBytes * Add missing shims causing Linux build to fail * Simplify GetNonAsciiBytes to only one AddPairwise call, shorter bitmask * Respect data type returned by masking method * Address suggestions - assert trailingzerocount and bring back uint mask * Trailing zeroes in AdvSimd need to be divided by 4, and total number should not be larger than 16 * Avoid declaring static field which causes PNSE in Utf8String.Experimental (S.P.Corelib code is used for being NetStandard) * Prefer using nuint for BitConverter.TrailingZeroCount * Fix build failure in net472 debug AdvSimd Utf16Utility (#39652) Co-authored-by: Carlos Sanchez Lopez <[email protected]>
…otnet#39041) * AdvSimd support for System.Text.Unicode.Utf8Utility.TranscodeToUtf8 * Readd using to prevent build failure. Add AdvSimd equivalent operation to TestZ. * Inverted condition * Address IsSupported order, improve use ExtractNarrowingSaturated usage * Rename source to result, second argument utf16Data * Improve CompareTest * Add shims causing failures in Linux * Use unsigned version of ExtractNarrowingSaturate, avoid using MinAcross and use MaxPairwise instead * Missing support check for Sse2.X64 * Add missing case for AdvSimd * Use MinPairwise for short
Contributes to #35035
Adds AdvSimd support for
System.Text.Unicode.Utf8Utility.TranscodeToUtf8()
inside the file
runtime\src\libraries\System.Private.CoreLib\src\System\Text\Unicode\Utf8Utility.Transcoding.cs
I've been having difficulties testing this in my ARM device so I want to analyze the CI results.
I manually executed an additional "Libraries Test Run" pipeline to ensure arm64 is run in all platforms.