-
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
remove redundant OR #88993
remove redundant OR #88993
Conversation
both inputs are equal here so if one of them is non ASCII, then the other one is too
Tagging subscribers to this area: @dotnet/area-system-text-encoding Issue Detailsboth inputs are equal here so if one of them is non ASCII, then the other one is too (idea taken from @BrennanConroy) ASM diff: https://www.diffchecker.com/hdGIxueW #87141 have not introduced the regression reported in #88670, but it has changed... the code alignment. This PR improves the perf for the mentioned benchmark by 5%. The other benchmark is back to it's previous state: So it should be safe to assume that this PR is going to fix #88670 dotnet run -c Release -f net8.0 -- --filter "*Perf_Ascii.Equals_*Chars*6*" --envVars DOTNET_EnableAVX2:0 --memoryRandomization --launchCount 6 --corerun $removedForBrevity BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22621.1848)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=8.0.100-preview.4.23259.14
[Host] : .NET 8.0.0 (8.0.23.25905), X64 RyuJIT AVX2
Job-VOGWHW : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX
Job-AYOMRP : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX
|
Presumably, you can use the same trick in the main loop of these functions that currently looks like this: -if (!AllCharsInVectorAreAscii(leftValues | rightValues))
-{
- return false;
-}
Vector128<TRight> notEquals = ~Vector128.Equals(leftValues, rightValues);
if (notEquals != Vector128<TRight>.Zero)
{
+ if (!AllCharsInVectorAreAscii(leftValues | rightValues))
+ return false;
// not exact match
leftValues |= loweringMask;
rightValues |= loweringMask;
if (Vector128.GreaterThanAny((leftValues - vecA) & notEquals, vecZMinusA) || leftValues != rightValues)
{
return false; // first input isn't in [A-Za-z], and not exact match of lowered
}
}
+else if (!AllCharsInVectorAreAscii(leftValues))
+ return false; but I'm not sure if it's profitable - |
The failures are unrelated (#88996), merging. |
both inputs are equal here so if one of them is non ASCII, then the other one is too (idea taken from @BrennanConroy)
ASM diff: https://www.diffchecker.com/hdGIxueW
#87141 have not introduced the regression reported in #88670, but it has changed... the code alignment.
This PR improves the perf for the mentioned benchmark by 5%.
The other benchmark is back to it's previous state:
So it should be safe to assume that this PR is going to fix #88670