Uh oh!
There was an error while loading. Please reload this page.
try to port ASCIIUtility.WidenAsciiToUtf16 to x-plat intrinsics - #73055
Conversation
ghost
commented
Jul 29, 2022
Tagging subscribers to this area: @dotnet/area-system-text-encoding Issue Detailsx64There was a major regression, but I was able to solve it by enforcing the inlining of DetailsBenchmarkDotNet=v0.13.1.1828-nightly, OS=Windows 11 (10.0.22000.795/21H2)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=7.0.100-preview.6.22352.1
[Host] : .NET 7.0.0 (7.0.22.32404), X64 RyuJIT AVX2
Job-IVMYPN : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-BJNCCX : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX2
ARM64There is a major perf regression: 40-50%. I know that it's not caused by the I am trying to update my Surface Pro X to Win 11, which will allow me to install VS 2022 (ARM64), build dotnet/runtime and try the new internal MS profiler. But I can't promise anything. DetailsBenchmarkDotNet=v0.13.1.1828-nightly, OS=ubuntu 20.04
Unknown processor
.NET SDK=7.0.100-rc.1.22378.8
[Host] : .NET 7.0.0 (7.0.22.37802), Arm64 RyuJIT AdvSIMD
Job-VUTCOY : .NET 7.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
Job-LKXRPH : .NET 7.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
contributes to #64451 cc @tannergooding@GrabYourPitchforks
|
…ciiDataSeenInInnerLoop was performed only when containsNonAsciiBytes was true)
9d653d5 to
26c0c9dCompare26c0c9d to
764d038Compare| /// <param name="source">The vector whose elements are to be widened.</param> | ||
| /// <returns>A pair of vectors that contain the widened lower and upper halves of <paramref name="source" />.</returns> | ||
| [CLSCompliant(false)] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
We should add this to all the Widen APIs and to the same on Vector64.
There was a problem hiding this comment.
Can the JIT use named intrinsic and treat all methods in Vector###<T> as candidates for aggressive inlining?
There was a problem hiding this comment.
@tannergooding I've synced my fork with upstream and verified that it's not needed anymore. So we don't need to backport anything to 7.0
Uh oh!
There was an error while loading. Please reload this page.
adamsitnik
commented
Jul 29, 2022
I added I replaced I updated the results posted above. |
| // Can we at least widen the first part of the vector? | ||
| if (!containsNonAsciiBytes) |
There was a problem hiding this comment.
I've removed this code, as it was impossible to satisfy this requirement as the jump was performed only when the flag was set to true:
runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs
Lines 1823 to 1827 in 85d638b
| pCurrentWriteAddress += (nuint)Vector128<byte>.Count; | ||
| } while (currentOffset <= finalOffsetWhereCanRunLoop); | ||
| // Calculate how many elements we wrote in order to get pOutputBuffer to its next alignment |
There was a problem hiding this comment.
based on the benchmarking that I've done this was not improving perf in noticeable way, but increasing the code complexity. I've removed it and added Vector256 code path (less code = less code to duplicate)
Edit: it turned out to be a R2R bug: #74253 OK, I have no idea what is going on and I need an extra pair of eyes. The tests are failing with The callstack is quite long, but it shows that failure starts from This method has 3 debug asserts: runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs Lines 1753 to 1757 in 20a2f23 And the only place where it's called from has exactly the same guards: runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs Lines 1581 to 1584 in 20a2f23 So the asserts mentioned above should definitely not fail. I was able to reproduce the failure locally. It's gone when I remove those 3 asserts! What am I missing? |
danmoseley
commented
Aug 21, 2022
Http test crash. Worth cracking the dump to check it's not related to this change? |
davidwrighton
commented
Aug 23, 2022
@adamsitnik please, don't just comment that assert out. The generated code will not reliably behave correctly, you need to follow the instructions I put in #74253 |
adamsitnik
commented
Aug 23, 2022
@davidwrighton thank you! btw based on the perf results I decided to simply inline these two helpers Details
|
adamsitnik
commented
Aug 23, 2022
Updated perf numbers: For x64 AVX2There is a 10-30% boost for large inputs. It's caused by adding the Small inputs also work faster, partially because DetailsBenchmarkDotNet=v0.13.1.20220823-develop, OS=Windows 11 (10.0.22000.856/21H2)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=7.0.100-preview.7.22377.5
[Host] : .NET 7.0.0 (7.0.22.37506), X64 RyuJIT AVX2
Job-SJSFRM : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QAHUTQ : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
LaunchCount=9 MemoryRandomization=True
ARM64 AdvSIMDThere is a small (4-8%) gain for all test cases. DetailsBenchmarkDotNet=v0.13.1.1845-nightly, OS=Windows 11 (10.0.22622.575)
Microsoft SQ1 3.0 GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=7.0.100-rc.2.22422.7
[Host] : .NET 7.0.0 (7.0.22.41112), Arm64 RyuJIT AdvSIMD
Job-AHYKGY : .NET 8.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
Job-THHHUV : .NET 8.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
LaunchCount=9 MemoryRandomization=True
|
| { | ||
| Vector256<byte> asciiVector = Vector256.Load(pAsciiBuffer + currentOffset); | ||
| if (asciiVector.ExtractMostSignificantBits() != 0) |
There was a problem hiding this comment.
https://github.com/dotnet/runtime/pull/73055/files?diff=split&w=1#diff-66bbe89271f826c9232bd146abb678844754515dc027f70ad0ce36f751da46ebR1378 suggests that Sse41.TestZ is faster than ExtractMostSignificantBits for 128 bits. Does the same not hold for Avx.TestZ for 256 bits?
There was a problem hiding this comment.
It does not:
Details
usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;usingSystem.Reflection.Metadata;usingSystem.Runtime.InteropServices;usingSystem.Runtime.Intrinsics;usingSystem.Runtime.Intrinsics.X86;namespaceVectorBenchmarks{internalclassProgram{staticvoidMain(string[]args)=>BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);}publicunsafeclassContainsNonAscii{privateconstintSize=1024;privatebyte*_bytes;[GlobalSetup]publicvoidSetup(){_bytes=(byte*)NativeMemory.AlignedAlloc(newUIntPtr(Size),newUIntPtr(32));newSpan<byte>(_bytes,Size).Clear();}[GlobalCleanup]publicvoidFree()=>NativeMemory.AlignedFree(_bytes);[Benchmark]publicboolExtractMostSignificantBits(){refbytesearchSpace=ref*_bytes;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector256<byte>.Count;do{Vector256<byte>asciiVector=Vector256.LoadUnsafe(refsearchSpace,currentOffset);if(asciiVector.ExtractMostSignificantBits()!=0){returntrue;}currentOffset+=(nuint)Vector256<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);returnfalse;}[Benchmark]publicboolTestZ(){refbytesearchSpace=ref*_bytes;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector256<byte>.Count;do{Vector256<byte>asciiVector=Vector256.LoadUnsafe(refsearchSpace,currentOffset);if(!Avx.TestZ(asciiVector,Vector256.Create((byte)0x80))){returntrue;}currentOffset+=(nuint)Vector256<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);returnfalse;}}}BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.856/21H2)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=7.0.100-rc.1.22423.16
[Host] : .NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2
DefaultJob : .NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2| Method | Mean | Error | StdDev | Code Size |
|---|---|---|---|---|
| ExtractMostSignificantBits | 11.78 ns | 0.042 ns | 0.040 ns | 57 B |
| TestZ | 14.76 ns | 0.320 ns | 0.416 ns | 68 B |
.NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2
; VectorBenchmarks.ContainsNonAscii.ExtractMostSignificantBits()vzerouppermovrax,[rcx+8]xoredx,edxnop dword ptr [rax]M00_L00: vmovdqu ymm0,ymmword ptr [rax+rdx] vpmovmskb ecx,ymm0testecx,ecxjne short M00_L01addrdx,20cmprdx,3E0jbe short M00_L00xoreax,eaxvzeroupperretM00_L01:moveax,1vzeroupperret; Total bytes of code 57.NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2
; VectorBenchmarks.ContainsNonAscii.TestZ()vzerouppermovrax,[rcx+8]xoredx,edxvmovupdymm0,[7FF9E3D94D60]nop dword ptr [rax]nop dword ptr [rax]M00_L00:vptestymm0,ymmword ptr [rax+rdx]jne short M00_L01addrdx,20cmprdx,3E0jbe short M00_L00xoreax,eaxvzeroupperretM00_L01:moveax,1vzeroupperret; Total bytes of code 68There was a problem hiding this comment.
It does not:
That's not what I see on my machine.
| Method | Mean |
|---|---|
| ExtractMostSignificantBits_128 | 31.77 ns |
| TestZ_128 | 25.58 ns |
| ExtractMostSignificantBits_256 | 15.58 ns |
| TestZ_256 | 11.66 ns |
Details
usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;usingSystem;usingSystem.Runtime.InteropServices;usingSystem.Runtime.Intrinsics.X86;usingSystem.Runtime.Intrinsics;[HideColumns("Error","StdDev","Median","RatioSD")]publicunsafepartialclassProgram{staticvoidMain(string[]args)=>BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);privateconstintSize=1024;privatebyte*_bytes;[GlobalSetup]publicvoidSetup(){_bytes=(byte*)NativeMemory.AlignedAlloc(newUIntPtr(Size),newUIntPtr(32));newSpan<byte>(_bytes,Size).Clear();}[GlobalCleanup]publicvoidFree()=>NativeMemory.AlignedFree(_bytes);[Benchmark]publicboolExtractMostSignificantBits_128(){refbytesearchSpace=ref*_bytes;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector128<byte>.Count;do{Vector128<byte>asciiVector=Vector128.LoadUnsafe(refsearchSpace,currentOffset);if(asciiVector.ExtractMostSignificantBits()!=0){returntrue;}currentOffset+=(nuint)Vector128<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);returnfalse;}[Benchmark]publicboolTestZ_128(){refbytesearchSpace=ref*_bytes;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector128<byte>.Count;do{Vector128<byte>asciiVector=Vector128.LoadUnsafe(refsearchSpace,currentOffset);if(!Sse41.TestZ(asciiVector,Vector128.Create((byte)0x80))){returntrue;}currentOffset+=(nuint)Vector128<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);returnfalse;}[Benchmark]publicboolExtractMostSignificantBits_256(){refbytesearchSpace=ref*_bytes;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector256<byte>.Count;do{Vector256<byte>asciiVector=Vector256.LoadUnsafe(refsearchSpace,currentOffset);if(asciiVector.ExtractMostSignificantBits()!=0){returntrue;}currentOffset+=(nuint)Vector256<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);returnfalse;}[Benchmark]publicboolTestZ_256(){refbytesearchSpace=ref*_bytes;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector256<byte>.Count;do{Vector256<byte>asciiVector=Vector256.LoadUnsafe(refsearchSpace,currentOffset);if(!Avx.TestZ(asciiVector,Vector256.Create((byte)0x80))){returntrue;}currentOffset+=(nuint)Vector256<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);returnfalse;}}There was a problem hiding this comment.
@stephentoub I think it depends on CPU, I even had to revert TestZ from Vector.Equals because it produced regressions #67902
There was a problem hiding this comment.
I think it depends on CPU, I even had to revert TestZ from Vector.Equals because it produced regressions #67902
That change reverted it from both the 256-bit and 128-bit code paths. This PR uses TestZ for 128-bit. Why is that ok?
I'm questioning the non-symmetrical usage.
There was a problem hiding this comment.
There are some helpful comments in these SO links (and related) https://stackoverflow.com/questions/60446759/sse2-test-xmm-bitmask-directly-without-using-pmovmskb
There was a problem hiding this comment.
Thanks, but I'm not seeing the answer there to my question.
I'll restate:
This PR is adding additional code to prefer using TestZ with Vector128:
https://github.com/dotnet/runtime/pull/73055/files#diff-66bbe89271f826c9232bd146abb678844754515dc027f70ad0ce36f751da46ebR1379-R1391
Your #67902 reverted other changes that preferred using TestZ, not just on 256-bit but also on 128-bit vectors.
Does it still make sense for this PR to be adding additional code to use TestZ with Vector128?
(Part of why I'm pushing on this is with a goal of avoiding needing to drop down to direct instrinsics as much as possible. I'd hope we can get to a point where the obvious code to write is the best code to write in as many situations as possible.)
There was a problem hiding this comment.
Right, I am not saying the current non-symmetrical usage is correct, I'd probably change both to use ExtractMostSignificantBits
C++ compilers also do different things here, e.g. LLVM folds even direct MoveMask usage to testz: https://godbolt.org/z/MobvxvzGK
There was a problem hiding this comment.
What is "better" is going to depend on a few factors.
On x86/x64, ExtractMostSignificantBits is likely faster because it can always emit exactly as movmsk and there are some CPUs where TestZ can be slower, particularly for "small inputs" where the match is sooner. When the match is later, TestZ typically wins out regardless.
On Arm64, doing the early comparison against == Zero is likely better because it is a single instruction vs the multi-instruction sequence required to emulate x64's movmsk.
I think the best choice here is to use == Zero (and therefore TestZ) as I believe it will, on average, produce the best/most consistent code. The cases where it might be a bit slower will typically be for smaller inputs where we're already returning quickly and the extra couple nanoseconds won't really matter.
| currentOffset += (nuint)Vector256<byte>.Count; | ||
| pCurrentWriteAddress += (nuint)Vector256<byte>.Count; | ||
| } while (currentOffset <= finalOffsetWhereCanRunLoop); |
There was a problem hiding this comment.
On every iteration of the loop we're bumping currentOffset and then also adding that to pAsciiBuffer. Would it be faster to instead compute the upper bound as an address, just bump the current pointer in the loop, and then after the loop compute the currentOffset if needed based on the ending/starting difference?
There was a problem hiding this comment.
It produces better codegen, but the reported time difference is within the range of error (0-0.3ns gain or loss)
Details
publicunsafeclassWiden{privateconstintSize=1024;privatebyte*_bytes;privatechar*_chars;[GlobalSetup]publicvoidSetup(){_bytes=(byte*)NativeMemory.AlignedAlloc(newUIntPtr(Size),newUIntPtr(32));newSpan<byte>(_bytes,Size).Fill((byte)'a');_chars=(char*)NativeMemory.AlignedAlloc(newUIntPtr(Size*sizeof(char)),newUIntPtr(32));}[GlobalCleanup]publicvoidFree(){NativeMemory.AlignedFree(_bytes);NativeMemory.AlignedFree(_chars);}[Benchmark]publicvoidCurrent(){refbytesearchSpace=ref*_bytes;ushort*pCurrentWriteAddress=(ushort*)_chars;nuintcurrentOffset=0;nuintfinalOffsetWhereCanRunLoop=Size-(uint)Vector256<byte>.Count;do{Vector256<byte>asciiVector=Vector256.Load(_bytes+currentOffset);if(asciiVector.ExtractMostSignificantBits()!=0){break;}(Vector256<ushort>low,Vector256<ushort>upper)=Vector256.Widen(asciiVector);low.Store(pCurrentWriteAddress);upper.Store(pCurrentWriteAddress+Vector256<ushort>.Count);currentOffset+=(nuint)Vector256<byte>.Count;pCurrentWriteAddress+=(nuint)Vector256<byte>.Count;}while(currentOffset<=finalOffsetWhereCanRunLoop);}[Benchmark]publicvoidSuggested(){refbytecurrentSearchSpace=ref*_bytes;refushortcurrentWriteAddress=refUnsafe.As<char,ushort>(ref*_chars);refbyteoneVectorAwayFromEnd=refUnsafe.Add(refcurrentSearchSpace,Size-Vector256<byte>.Count);do{Vector256<byte>asciiVector=Vector256.LoadUnsafe(refcurrentSearchSpace);if(asciiVector.ExtractMostSignificantBits()!=0){break;}(Vector256<ushort>low,Vector256<ushort>upper)=Vector256.Widen(asciiVector);low.StoreUnsafe(refcurrentWriteAddress);upper.StoreUnsafe(refcurrentWriteAddress,(nuint)Vector256<ushort>.Count);currentSearchSpace=refUnsafe.Add(refcurrentSearchSpace,Vector256<byte>.Count);currentWriteAddress=refUnsafe.Add(refcurrentWriteAddress,Vector256<byte>.Count);}while(!Unsafe.IsAddressGreaterThan(refcurrentSearchSpace,refoneVectorAwayFromEnd));}}BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.856/21H2)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=7.0.100-rc.1.22423.16
[Host] : .NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2
DefaultJob : .NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2| Method | Mean | Error | StdDev | Code Size |
|---|---|---|---|---|
| Current | 44.29 ns | 0.171 ns | 0.152 ns | 81 B |
| Suggested | 44.54 ns | 0.042 ns | 0.032 ns | 77 B |
.NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2
; VectorBenchmarks.Widen.Current()vzerouppermoveax,[rcx+8]movrax,[rcx+10]xoredx,edxM00_L00:movr8,[rcx+8] vmovdqu ymm0,ymmword ptr [r8+rdx] vpmovmskb r8d,ymm0testr8d,r8djne short M00_L01 vmovaps ymm1,ymm0 vpmovzxbw ymm1,xmm1vextractf128xmm0,ymm0,1 vpmovzxbw ymm0,xmm0 vmovdqu ymmword ptr [rax],ymm1 vmovdqu ymmword ptr [rax+20],ymm0addrdx,20addrax,40cmprdx,3E0jbe short M00_L00M00_L01:vzeroupperret; Total bytes of code 81.NET 7.0.0 (7.0.22.42223), X64 RyuJIT AVX2
; VectorBenchmarks.Widen.Suggested()vzerouppermovrax,[rcx+8]movrdx,[rcx+10]learcx,[rax+3E0]M00_L00: vmovdqu ymm0,ymmword ptr [rax] vpmovmskb r8d,ymm0testr8d,r8djne short M00_L01 vmovaps ymm1,ymm0 vpmovzxbw ymm1,xmm1vextractf128xmm0,ymm0,1 vpmovzxbw ymm0,xmm0 vmovdqu ymmword ptr [rdx],ymm1 vmovdqu ymmword ptr [rdx+20],ymm0addrax,20addrdx,40cmprax,rcxjbe short M00_L00M00_L01:vzeroupperret; Total bytes of code 77If you don't mind I am going to merge it as it is and apply your suggestion in my next PR.
| break; | ||
| } | ||
| // Vector128.Widen is not used here as it less performant on ARM64 |
There was a problem hiding this comment.
Do we know why? Naively I'd expect the JIT to be able to produce the same code for both.
There was a problem hiding this comment.
I am sorry, but I don't. It's not that I did not try to find out, it's the arm64 tooling that makes it hard for me.
There was a problem hiding this comment.
If this is by design, ok. But if it's something we can/should be fixing in the JIT, I want to make sure we're not sweeping such issues under the rug. Ideally the obvious code is also the best performing code.
There was a problem hiding this comment.
Codegen looks good to me:
(add could be contained but it's unrelated here)
There was a problem hiding this comment.
I'd need a comparison between the two code paths to see where the difference is.
I would expect these to be identical except for a case where the original code was making some assumption (based on knowing the inputs were restricted to a subset of all possible values) and therefore skipping an otherwise "required" step that would be necessary to ensure deterministic results for "any input".
| } while (currentOffset <= finalOffsetWhereCanRunLoop); | ||
| } | ||
| } | ||
| else if (Vector.IsHardwareAccelerated) |
There was a problem hiding this comment.
Why is the Vector<T> path still needed?
There was a problem hiding this comment.
Why is the Vector path still needed?
Some Mono variants don't support Vector128 for all configs yet
There was a problem hiding this comment.
Some Mono variants don't support Vector128 for all configs yet
Which support Vector and not Vector128?
Just the presence of these paths are keeping the methods from being R2R'd it seems.
There was a problem hiding this comment.
Mono-LLVM supports both, Mono without LLVM (e.g. default Mono jit mode or AOT) supports only Vector<>
There was a problem hiding this comment.
Mono-LLVM supports both, Mono without LLVM (e.g. default Mono jit mode or AOT) supports only Vector<>
Is that getting fixed?
We now have multiple vectorized implementations that don't have a Vector<T> code path. Why is this one special that it still needs one?
EDIT: for updated perf numbers please go to #73055 (comment)
x64
Initially, there was a major regression, but I was able to solve it by enforcing the inlining of
Vector128.Widen(Vector128<byte>). After porting everything the new implementation was on par:Details
With some additional optimizations and adding
Vector256code path it's now on par or up to 20% faster, depending on input (the more characters are ascii, the better).Details
ARM64
Initially, after just mapping the code there was a major regression of 40-50%:
Details
After I replaced
Vector128.WidenwithVector128.WidenLowerandVector128.WidenUpper, I was able to lower the regression to 10-16%Details
I am trying to update my Surface Pro X to Win 11, which will allow me to install VS 2022 (ARM64), build dotnet/runtime and try the new internal MS profiler. But I can't promise anything.
contributes to #64451
cc @tannergooding@GrabYourPitchforks