Uh oh!
There was an error while loading. Please reload this page.
port SpanHelpers.IndexOfAny(ref byte, byte, byte, int) to Vector128/256 - #73556
port SpanHelpers.IndexOfAny(ref byte, byte, byte, int) to Vector128/256#73556adamsitnik wants to merge 4 commits into
Conversation
ghost
commented
Aug 8, 2022
Tagging subscribers to this area: @dotnet/area-system-memory Issue DetailsIt's #73384 again I've tried to repro #73474 locally and I've failed. Now I want to use the CI to verify if the issue is gone or not. ./build.sh --os browser -c Release
make -C src/mono/wasm provision-wasm
export EMSDK_PATH=/home/adam/projects/runtime/src/mono/wasm/emsdk
./dotnet.sh build /t:Test /p:TargetOS=Browser /p:TargetArchitecture=wasm /p:RunAOTCompilation=true /p:EnableAggressiveTrimming=true -c Release ./src/libraries/System.Runtime/tests
|
adamsitnik
commented
Aug 8, 2022
@jkotas the |
jkotas
commented
Aug 8, 2022
What fixed the underlying issue? The crash was caused by reading beyond end of the buffer. The CI may be green by luck - the memory after the end of the buffer might have been initialized just right to avoid the crash in the given run. @vargaz or @lambdageek should inspect the IL to confirm that the problem is gone. Also, we should find the change that fixed it. |
jkotas
commented
Aug 8, 2022
Ok, #73493 fixed the out of bound read, replaced it by rejecting the whole method with BadImageFormatException. Do we still have a functional issue at runtime? |
adamsitnik
commented
Aug 8, 2022
System.Memory tests exercise this code path and they are passing on machine with AVX2 |
jkotas
commented
Aug 8, 2022
Do the tests exercise the AOT code of method, or are they falling back to the interpreter silently? If it is the latter, we are likely going to see perf regressions. |
adamsitnik
commented
Aug 8, 2022
I am using instructions provided by @lambdageek in #73474 (comment) to run these tests /p:RunAOTCompilation=true /p:EnableAggressiveTrimming=true I would expect that they exercise the AOT code of method. How can I check if they don't fall back to interpreter silently? |
jkotas
commented
Aug 8, 2022
I don’t know how to check that. Somebody from the Mono team needs to help you with at that. |
I guess |
adamsitnik
commented
Aug 8, 2022
I've tried to benchmark WASM AOT but I don't think that it's currently possible: dotnet/BenchmarkDotNet#1818 (comment) |
lambdageek
commented
Aug 8, 2022
/azp run runtime-wasm |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I'm trying the System.Memory tests locally. I would expect any call to IndexOfAny to throw BIFE on wasm AOT. there shouldn't be a fallback to the interpreter, AFAIK. Perhaps we're skipping some of these tests already for other reasons |
Ah interesting. So this PR is good; i'm working, separately, on adding a fallthru check to the interpreter |
jkotas
commented
Aug 8, 2022
Do you agree that this PR is going to introduce perf regression since the method won’t be AOT compiled anymore? |
lambdageek
commented
Aug 8, 2022
Yes, I should qualify "good" - this PR wont' crash the AOT compiler or the runtime anymore. The perf won't be good. |
lambdageek
commented
Aug 8, 2022
actually dynamically this code is unreachable |
adamsitnik
commented
Aug 8, 2022
@jkotas@lambdageek@vitek-karas I am using similar patterns in few other PRs and aiming to port all But why? Is it because I am using |
jkotas
commented
Aug 8, 2022
Yes, something in the refactored methods exposed a bug in the IL linker. Native AOT does not have the bug. It is not sharing the relevant part of the linker. |
lambdageek
commented
Aug 8, 2022
I'm not sure, but I think I have a workaround: if I change the last else{Debug.Assert(Vector.IsHardwareAccelerated);then I can also delete Debug.Fail("Unreachable");gotoNotFound;and then in the IL that comes out of the linker, I don't see a conditional jump at the end of the method anymore. (I'm not sure if this has any undesired impact on CoreCLR performance) |
jkotas
commented
Aug 8, 2022
My guess is that the bug is triggered by certain shape of the method flowgraph. It is not about types used. |
vitek-karas
commented
Aug 8, 2022
Sorry - I didn't get to this yet (first on my list after emails/management)... Early1:Early2:if(something){gotoEarly1;}else{gotoEarly2;}Basically the method ends with an if/else where both branches go backwards. In that case it's invalid to remove the "false" branch, because there's nothing left in the method - and even though that branch is technically not reachable, it needs to exist for validity. |
EgorBo
commented
Aug 8, 2022
Can it still remove it and fold |
vitek-karas
commented
Aug 8, 2022
I'll have to go through the code - but in general the code in the linker is very simplistic and tries to avoid changing the number of instructions (it's problematic to move code around in that setup) - so it tries to keep the same number of instructions. It's also relatively problematic to fully remove the ldc.0 and so on - if the condition is over a call then removing the entire call is relatively complicated - and we don't do that. Which means we need to "pop" somehow. I'm considering either disabling the optimization in this case or to force insert a dummy ret at the end of the method. |
jkotas
commented
Aug 8, 2022
ldnull + throw may be better. Ret requires exactly zero or one items on the evaluation stack. |
adamsitnik
commented
Aug 9, 2022
@jkotas@lambdageek@vitek-karas Thanks for the explanation! I've removed all the gotos, similified the code and ensured that there are no regressions for configs that I can benchmark (CLR x64 AVX2 AVX & arm64 AdvSIMD). The perf has not regressed, it's on par. When reading the AVX2 (x64)DetailsBenchmarkDotNet=v0.13.1.1845-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.7.22377.5
[Host] : .NET 7.0.0 (7.0.22.37506), X64 RyuJIT AVX2
Job-FHASLE : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-JRHZCS : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX2
LaunchCount=9
Code gen before: Details; System.SpanHelpers.IndexOfAny(Byte ByRef, Byte, Byte, Int32)pushrsivzerouppermovzxedx,dlmoveax,edxmovzxr8d,r8bmovr10d,r8dxorr11d,r11dmovesi,r9dmovsxdr9,r9daddr9,0FFFFFFFFFFFFFFF0js near ptr M00_L01movrsi,r9jmp near ptr M00_L13M00_L00:addrsi,0FFFFFFFFFFFFFFF8movzxedx,byte ptr [rcx+r11]cmpeax,edxje near ptr M00_L05cmpr10d,edxje near ptr M00_L05movzxedx,byte ptr [rcx+r11+1]cmpeax,edxje near ptr M00_L06cmpr10d,edxje near ptr M00_L06movzxedx,byte ptr [rcx+r11+2]cmpeax,edxje near ptr M00_L07cmpr10d,edxje near ptr M00_L07movzxedx,byte ptr [rcx+r11+3]cmpeax,edxje near ptr M00_L08cmpr10d,edxje near ptr M00_L08movzxedx,byte ptr [rcx+r11+4]cmpeax,edxje near ptr M00_L09cmpr10d,edxje near ptr M00_L09movzxedx,byte ptr [rcx+r11+5]cmpeax,edxje near ptr M00_L10cmpr10d,edxje near ptr M00_L10movzxedx,byte ptr [rcx+r11+6]cmpeax,edxje near ptr M00_L11cmpr10d,edxje near ptr M00_L11movzxedx,byte ptr [rcx+r11+7]cmpeax,edxje near ptr M00_L12cmpr10d,edxje near ptr M00_L12addr11,8M00_L01:cmprsi,8jae near ptr M00_L00cmprsi,4jb short M00_L02addrsi,0FFFFFFFFFFFFFFFCmovzxedx,byte ptr [rcx+r11]cmpeax,edxje short M00_L05cmpr10d,edxje short M00_L05movzxedx,byte ptr [rcx+r11+1]cmpeax,edxje short M00_L06cmpr10d,edxje short M00_L06movzxedx,byte ptr [rcx+r11+2]cmpeax,edxje short M00_L07cmpr10d,edxje short M00_L07movzxedx,byte ptr [rcx+r11+3]cmpeax,edxje short M00_L08cmpr10d,edxje short M00_L08addr11,4M00_L02:testrsi,rsije short M00_L04M00_L03:movzxedx,byte ptr [rcx+r11]cmpeax,edxje short M00_L05cmpr10d,edxje short M00_L05incr11decrsijne short M00_L03M00_L04:moveax,0FFFFFFFFvzeroupperpoprsiretM00_L05:moveax,r11djmp near ptr M00_L20M00_L06:leaeax,[r11+1]jmp near ptr M00_L20M00_L07:leaeax,[r11+2]jmp near ptr M00_L20M00_L08:leaeax,[r11+3]jmp near ptr M00_L20M00_L09:leaeax,[r11+4]jmp near ptr M00_L20M00_L10:leaeax,[r11+5]jmp near ptr M00_L20M00_L11:leaeax,[r11+6]jmp near ptr M00_L20M00_L12:leaeax,[r11+7]jmp near ptr M00_L20M00_L13:cmprsi,10jb short M00_L16 vmovd xmm0,edxvpbroadcastbymm0,xmm0 vmovd xmm1,r8dvpbroadcastbymm1,xmm1addrsi,0FFFFFFFFFFFFFFF0je short M00_L15M00_L14:vmovupdymm2,[rcx+r11] vpcmpeqb ymm3,ymm0,ymm2 vpcmpeqb ymm2,ymm1,ymm2vporymm2,ymm3,ymm2 vpmovmskb eax,ymm2testeax,eaxjne near ptr M00_L19addr11,20cmprsi,r11ja short M00_L14M00_L15:vmovupdymm2,[rcx+rsi]movr11,rsi vpcmpeqb ymm0,ymm0,ymm2 vpcmpeqb ymm1,ymm1,ymm2vporymm0,ymm0,ymm1 vpmovmskb eax,ymm0testeax,eaxjne short M00_L19jmp near ptr M00_L04M00_L16: vmovd xmm0,edxvpbroadcastbxmm0,xmm0 vmovd xmm1,r8dvpbroadcastbxmm1,xmm1testrsi,rsije short M00_L18M00_L17:vmovupdxmm2,[rcx+r11] vpcmpeqb xmm3,xmm0,xmm2 vpcmpeqb xmm2,xmm1,xmm2vporxmm2,xmm3,xmm2 vpmovmskb eax,xmm2testeax,eaxjne short M00_L19addr11,10cmprsi,r11ja short M00_L17M00_L18:vmovupdxmm2,[rcx+rsi]movr11,rsi vpcmpeqb xmm0,xmm0,xmm2 vpcmpeqb xmm1,xmm1,xmm2vporxmm0,xmm0,xmm1 vpmovmskb eax,xmm0testeax,eaxje near ptr M00_L04M00_L19:tzcnteax,eaxaddr11,raxjmp near ptr M00_L05M00_L20:vzeroupperpoprsiretint3int3int3int3int3int3int3int3int3int3int3int3int3int3int3int3int3int3int3int3sbb[rcx],eaxadd[rax],eaxadd[rax],esp; Total bytes of code 663Codegen after: Details.NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX2; System.SpanHelpers.IndexOfAny(Byte ByRef, Byte, Byte, Int32)vzeroupperxorr10d,r10dcmpr9d,20jl short M00_L02nop dword ptr [rax]moveax,r9dandeax,0FFFFFFE0movzxr11d,dl vmovd xmm0,r11dvpbroadcastbymm0,xmm0movzxr11d,r8b vmovd xmm1,r11dvpbroadcastbymm1,xmm1M00_L00: vmovdqu ymm2,ymmword ptr [rcx+r10] vpcmpeqb ymm3,ymm0,ymm2 vpcmpeqb ymm2,ymm1,ymm2vporymm2,ymm3,ymm2vptestymm2,ymm2jne short M00_L01addr10,20cmpr10,raxjb short M00_L00jmp short M00_L05nop dword ptr [rax]M00_L01: vpmovmskb r9d,ymm2xoreax,eaxtzcnteax,r9daddeax,r10djmp near ptr M00_L09M00_L02:cmpr9d,10jl short M00_L05moveax,r9dandeax,0FFFFFFF0movzxr11d,dl vmovd xmm0,r11dvpbroadcastbxmm0,xmm0movzxr11d,r8b vmovd xmm1,r11dvpbroadcastbxmm1,xmm1M00_L03: vmovdqu xmm2,xmmword ptr [rcx+r10] vpcmpeqb xmm3,xmm0,xmm2 vpcmpeqb xmm2,xmm1,xmm2vporxmm2,xmm3,xmm2vptestxmm2,xmm2jne short M00_L04addr10,10cmpr10,raxjb short M00_L03jmp short M00_L05xchgax,axM00_L04: vpmovmskb edx,xmm2xoreax,eaxtzcnteax,edxaddeax,r10djmp short M00_L09M00_L05:movzxeax,dlmovzxedx,r8bmovr8d,r9dcmpr10,r8jae short M00_L07M00_L06:movzxr9d,byte ptr [rcx+r10]cmpeax,r9dje short M00_L08cmpedx,r9dje short M00_L08incr10cmpr10,r8jb short M00_L06M00_L07:moveax,0FFFFFFFFvzeroupperretM00_L08:moveax,r10dM00_L09:vzeroupperretadd[rcx],bladd[rax],aladd[rax],aladd[rax],aladd[rax],aladd[rax],aladd[rax],aladd[rax],aladd[rax-55C021C],bhjg short M00_L10M00_L10:add[rbp+48],dl (bad); Total bytes of code 291AVX (x64)DetailsBenchmarkDotNet=v0.13.1.1845-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.7.22377.5
[Host] : .NET 7.0.0 (7.0.22.37506), X64 RyuJIT AVX2
Job-YLMNVS : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX
Job-DFRNLR : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT AVX
EnvironmentVariables=COMPlus_EnableAVX2=0 LaunchCount=9
AdvSIMD(arm64)DetailsBenchmarkDotNet=v0.13.1.1845-nightly, OS=ubuntu 20.04
Unknown processor
.NET SDK=7.0.100-rc.1.22408.5
[Host] : .NET 7.0.0 (7.0.22.40308), Arm64 RyuJIT AdvSIMD
Job-NZXMHK : .NET 7.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
Job-LQCIXE : .NET 7.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
LaunchCount=9
|
vitek-karas
commented
Aug 9, 2022
I think I have a tentative fix for the linker bug (with the gotos - I couldn't find any other way to get to that state 😉 ). I'll clean it up and if possible we should try it on your previous change... anyway, thanks a lot for finding it! |
SwapnilGaikwad
commented
Aug 11, 2022
Hi @adamsitnik, I'm looking at the char equivalent of IndexOfAny. It's half done (blocked by #73777) but can add as a draft PR if you are going to find it useful. |
SwapnilGaikwad
commented
Aug 11, 2022
|
adamsitnik
commented
Aug 17, 2022
We don't need it anymore due to #73556 |
It's #73384 again
I've tried to repro #73474 locally and I've failed. Now I want to use the CI to verify if the issue is gone or not.