-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Light up String.Manipulation APIs with Vector512 codepath #93043
Light up String.Manipulation APIs with Vector512 codepath #93043
Conversation
…r oldChar, char newChar)
@tannergooding Just sending out a reminder to review this PR. |
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsOptimizing the following String APIs
PERF on ICXBelow tables show a result comparison output by ResultComparer in the performance repo. Base = No changes 1. SplitA Vector128 code path already exists for this API. We are adding a similar Vector256 and Vector512 code path. base =
base =
This is one of the issues where Avx512 is not that performance because of the issue with using multiple As you can see, for each iteration, 2. Replace_CharA Vector128 code path already exists for this API. We are just adding a single iteration of Vector512 or Vector256. base =
base =
|
@tannergooding just sending out a reminder for this pending review. |
CC. @stephentoub, @GrabYourPitchforks, @adamsitnik Could one of you give this a secondary review. |
@stephentoub @GrabYourPitchforks @adamsitnik sending a reminder for review. Can you please review this PR. |
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.
Thanks
@tannergooding @kunalspathak can you please help with merging this PR? It has been approved for quite some time now. |
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.
LGTM
CI seems red, so I will kick off another round once again to make sure there is nothing related to this PR. /azp run runtime |
Optimizing the following String APIs
PERF on ICX
Below tables show a result comparison output by ResultComparer in the performance repo.
Base = No changes
Diff = With the PR changes
1. Split
A Vector128 code path already exists for this API. We are adding a similar Vector256 and Vector512 code path.
base =
Diff Vector256 code path
vs diff =Diff Vector512 code path
base =
Base Vector128 code path
vs diff =Diff Vector256 code path
This is one of the issues where Avx512 is not that performance because of the issue with using multiple
Vector512.Equals()
. I ran a couple of iterations using StopWatch method and below are the results.As you can see, for each iteration,
Vector512
is almost the same asVector256
. Let me know if there are any suggestions for further optimizingVector512
code path. We have to decide whether this can ne merged or not since there are alreadyVector128
code path for both the APIs. Also, the Vector256 and Vector512 code path provide a significant speed up over Vector128 code path.2. Replace_Char
A Vector128 code path already exists for this API. We are just adding a single iteration of Vector512 or Vector256.
base =
Diff, Vector256 code path
vs diff =Diff Vector512 code path
base =
Base Vector128 code path
vs diff =Diff Vector256 code path