Remove BMI2 intrinsics from ASCII and UTF-8 transcoding logic #31904
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
GrabYourPitchforks added NO-MERGE 
Member
| I'll run against both of my AMD boxes (1800X and 3900X) later today and will share back results. |
Member
| Numbers for the Ryzen 9 3900X Baseline, BMI Enabled Baseline, BMI Disabled This PR |
Member
| I'll run 3900X results tonight and post, as that box my personal one at home. |
Member Author
| The fuzzing run is now in progress. The fuzzer is running over the combination of this PR with #32036 cherry-picked. For reference, the fuzzer target application is https://github.com/GrabYourPitchforks/utf8fuzz. There are two runs going in parallel: one with full intrinsic enabled, and one with SSE4.1+ disabled. |
Member
| Numbers for the Ryzen 9 3900X Baseline, BMI Enabled Baseline, BMI Disabled This PR |
Member
| Also updated the 1800X numbers above to include Baseline, BMI Disabled |
GrabYourPitchforks removed the NO-MERGE Member Author
| Removed no-merge label as fuzzing run has been going all weekend with no problem. |
src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Helpers.cs Show resolved Hide resolved
tannergooding approved these changes Feb 11, 2020
Member
tannergooding left a comment
tannergooding left a comment There was a problem hiding this comment.
Overall LGTM.
I don't think we need the assert that we are little endian on x86 and the intrinsic code could likely benefit from comments in a few places for abitrary users reading the code.
GrabYourPitchforks added a commit to GrabYourPitchforks/coreclr that referenced this pull request Feb 11, 2020
Remove BMI2 from ASCII and UTF-16 processing hot paths, as not all processors have optimized implementations of pext/pdep
Anipik pushed a commit to dotnet/coreclr that referenced this pull request Feb 18, 2020
Remove BMI2 from ASCII and UTF-16 processing hot paths, as not all processors have optimized implementations of pext/pdep
ghost locked as resolved and limited conversation to collaborators Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Contributes to #2251. For the most part, BMI2 instructions have been replaced with SIMD instructions where possible. Where the replacement is not easily doable the BMI2 instruction was removed entirely. Performance chart follows.
Most of the performance characteristics are approximately the same. There's a small perf gain in the Alice in Wonderland text (11-0.txt) for transcoding each direction. There's a small perf loss in the CJK sample (25249-0.txt).
Marked as no-merge because I haven't finished fuzzing it yet. It'll take a few days for the run to finish.
Note to reviewers: hiding whitespace changes will make this easier to review.