Uh oh!
There was an error while loading. Please reload this page.
Implement StoreSelectedScalar for Arm64 - #93223
Conversation
ghost
commented
Oct 9, 2023
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
ghost
commented
Oct 9, 2023
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsContribute towards #84510. // AdvSimdpublicstaticunsafevoidStoreSelectedScalar64x2(byte*address,(Vector64<byte>Value1,Vector64<byte>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar64x2(sbyte*address,(Vector64<sbyte>Value1,Vector64<sbyte>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar64x2(short*address,(Vector64<short>Value1,Vector64<short>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar64x2(ushort*address,(Vector64<ushort>Value1,Vector64<ushort>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar64x2(int*address,(Vector64<int>Value1,Vector64<int>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar64x2(uint*address,(Vector64<uint>Value1,Vector64<uint>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar64x2(float*address,(Vector64<float>Value1,Vector64<float>Value2)value,byteindex);// AdvSimd.Arm64publicstaticunsafevoidStoreSelectedScalar128x2(byte*address,(Vector128<byte>Value1,Vector128<byte>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(sbyte*address,(Vector128<sbyte>Value1,Vector128<sbyte>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(short*address,(Vector128<short>Value1,Vector128<short>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(ushort*address,(Vector128<ushort>Value1,Vector128<ushort>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(int*address,(Vector128<int>Value1,Vector128<int>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(uint*address,(Vector128<uint>Value1,Vector128<uint>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(long*address,(Vector128<long>Value1,Vector128<long>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(ulong*address,(Vector128<ulong>Value1,Vector128<ulong>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(float*address,(Vector128<float>Value1,Vector128<float>Value2)value,byteindex);publicstaticunsafevoidStoreSelectedScalar128x2(double*address,(Vector128<double>Value1,Vector128<double>Value2)value,byteindex);
|
4672ea0 to
621dc68CompareSwapnilGaikwad
commented
Oct 9, 2023
This patch would emit the following assembly sequence 1. When the index is a constant valueAdvSimd.StoreSelectedScalar64x2((Int32*)_dataTable.outArrayPtr,(AdvSimd.LoadVector64((Int32*)(_dataTable.inArray1Ptr)),AdvSimd.LoadVector64((Int32*)(_dataTable.inArray2Ptr))),1);2. When index is not a constantAdvSimd.StoreSelectedScalar64x2((Int32*)_dataTable.outArrayPtr,(AdvSimd.LoadVector64((Int32*)(_dataTable.inArray1Ptr)),AdvSimd.LoadVector64((Int32*)(_dataTable.inArray2Ptr))),elemIndex);3. When the index is variable and out of boundsThe current test crashes with an error.Not sure whether it's the desired behaviour. Also, I haven't considered the possibility of creating a ROP gadget with the sequence for Case 2 above. The sequence of instruction where value of an index can be used control the jump target. |
kunalspathak
commented
Oct 9, 2023
As seen in #93197, the API names should be just |
kunalspathak
left a comment
There was a problem hiding this comment.
I will double check about the immediate operand out of range scenario.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
kunalspathak
commented
Oct 10, 2023
Alright, so if you follow #93197, the |
kunalspathak
commented
Oct 10, 2023
They should throw |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
kunalspathak
commented
Nov 1, 2023
Interesting. What is the cause of it? |
SwapnilGaikwad
commented
Nov 1, 2023
Now we get out of range exception for all the invalid scenarios (with constant or variable index) with all the test cases.
This mystery is also resolved now. There was a check for array out of bounds missing. Now added it. Can be confirmed from the assembly for Full assemblyInterestingly, for some tests I got |
kunalspathak
commented
Nov 1, 2023
Seems the indices of few test cases needs adjustment for them to not throw Exception. |
Had to move the range check after recomputing the |
Uh oh!
There was an error while loading. Please reload this page.
SwapnilGaikwad
commented
Nov 6, 2023
Sure! It's tricky to debug, and it isn't obvious why compilation of |
kunalspathak
commented
Nov 6, 2023
From what I remember, these tests were passing earlier and failing with the recent commit and are in arm64, so most likly related. I could be wrong. |
SwapnilGaikwad
commented
Nov 6, 2023
You're right, it seems to be related to the arg check. Not sure how to debug it though. I'll try to see if I can reproduce the failure. If that doesn't progress, should I put the explicit check as before? |
I would say just inspect the code changes and see if the code path for existing |
kunalspathak
commented
Nov 15, 2023
@SwapnilGaikwad - can you resolve the merge conflicts? |
fanyang-mono
left a comment
There was a problem hiding this comment.
The change to Mono codebase looks good to me.
Change-Id: I81d386adbbcc195b777f436329f24438b0cca10c
kunalspathak
commented
Nov 16, 2023
@fanyang-mono - was the change essentially to make sure that mono continue to ignore newer APIs whose 2nd parameter is tuple and only support the existing |
Unfortunately, yes, Mono is not super friendly with handling API with 4 and more input arguments. We need to add additional code to handle those cases, rather than automatically handled by the machinery like Vector64/128. There is an issue to track Mono side of the work related to these new set of API's. |
Contribute towards #84510.