Vectorize SpanHelpers<T>.IndexOf #60974
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @GrabYourPitchforks, @dotnet/area-system-memory Issue DetailsAdd a vectorized path in Baseline is commit 31c38ef. Benchmark results:
|
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
|
|
||
| internal static bool CanVectorizeIndexOfForType<T>() | ||
| { | ||
| return (typeof(T) == typeof(byte)) || |
There was a problem hiding this comment.
I wonder if there's no "built-in" way to achieve this. Or at least any kind of central place for all the helpers that use vectorized pathes.
There was a problem hiding this comment.
Just realized there is Vector<T>.IsTypeSupported. This should work as a replacement for checks within the new IndexOfValueType<T> method. I'll make the changes and remove this helper method since that will be cleaner.
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Günther Foidl <gue@korporal.at>
| { | ||
| // bool and char will already have been checked before, just do checks for types | ||
| // that are equal to sizeof(int) or sizeof(long) | ||
| if (Unsafe.SizeOf<T>() == sizeof(int)) |
There was a problem hiding this comment.
Won't this break for float and double? This is why we have IsBitwiseEquatable (see https://source.dot.net/System.Private.CoreLib/R/e4188e6833cbc739.html) as a helper API.
There was a problem hiding this comment.
Good point, I'll add a check.
There was a problem hiding this comment.
do we need tests for float and double somewhere -- did anything fail before you fixed this?
There was a problem hiding this comment.
No, nothing failed for me when testing locally using:
.\build.cmd clr+libs+libs.tests -c Checked -test
.\build.cmd clr+libs+libs.tests -c Release -testBut some new tests for float and double would probably be appropriate just to be sure. Do you know the best place to add those?
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
| @@ -225,6 +226,22 @@ public static void Fill<T>(ref T refData, nuint numElements, T value) | |||
| { | |||
| Debug.Assert(length >= 0); | |||
|
|
|||
| if (typeof(T).IsValueType && RuntimeHelpers.IsBitwiseEquatable<T>()) | |||
| { | |||
| // bool and char will already have been checked before, just do checks for types | |||
There was a problem hiding this comment.
Indeed I see byte/bool and char are checked here
https://github.com/danmoseley/runtime/blob/f3ca6f91ba9c758bb246be8ba26bd356d3f9dda6/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs#L298
... why are 1 and 2 byte sizes treated specially there, and 4 and 8 byte sizes treated specially here? why not all in the same place?
There was a problem hiding this comment.
Oh, I think I was referencing a different check for those types, but putting them all near the section you linked would be cleaner and more intuitive. I'll move the checks there instead.
Co-authored-by: Dan Moseley <danmose@microsoft.com>
…sts for Span<T>.Contains
…et-runtime into vectorize-array-indexof
| @@ -193,5 +195,114 @@ public static void ContainsNull_String(string[] spanInput, bool expected) | |||
| Span<string> theStrings = spanInput; | |||
| Assert.Equal(expected, theStrings.Contains(null)); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@danmoseley I've added some tests here since there were not many tests for Span<T>.Contains. I believe the existing tests for Array.IndexOf (link) already have enough coverage. Let me know if I can add/modify anything.
There was a problem hiding this comment.
LGTM, big thanks for your contribution @alexcovington !
The benchmark numbers look great. Is there any chance you could run them for smaller collections as well? Currently we test only 512 elements which ofc is going to be great for the vectorized code path. You could do that by modifying the following Params:
In a following way:
- [Params(Utils.DefaultCollectionSize)]
+ [Params(Utils.DefaultCollectionSize, 7)]
|
Sorry for the delay, @adamsitnik.
Absolutely, I've added the following size parameters to the tests you suggested: [Params(7, 25, Utils.DefaultCollectionSize)]Here's the comparison: So there is a slight regression for smaller buffers that can't be vectorized. It looks like the extra few nanoseconds are due to the extra conditional check that needs to be performed. PerfView shows more samples being collected around one of the checks to see if the type of the buffer is a value type: PerfView screenshots
Once the buffer is large enough to be vectorized, the extra cost of the conditional is made up for by the perf increase of the vectorized path. Please let me know if I can add any additional information or run any other scenarios. |
|
@alexcovington thank you for providing the benchmark numbers. LGTM, |
|
Nice improvements thanks @alexcovington ! Do you plan to do more vectorization type work? |
|
Sorry for the delayed response, @danmoseley.
Yes, we're always interested in potential vector/SIMD optimization opportunities. Let me know if we can assist with similar work! |
ghost
locked as resolved and limited conversation to collaborators
Add a vectorized path in
SpanHelpers.T.csfor value types that performs the same logic asSpanHelpers.IndexOf<T>but will vectorize the operations where possible.Baseline is commit 31c38ef.
Benchmark results: