-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Light up BitArray APIs with Vector512 code path #91903
Light up BitArray APIs with Vector512 code path #91903
Conversation
…e larger vec size if processed
- merging with main
merging with main
…e larger vec size if processed merging with main
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsNO NEED TO REVIEW RIGHT NOWThis PR is about adding Vector512 support to the existing BitArray library APIs. Upgrading the following APIs
PERF
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One point for better codegen, didn't look into the rest at the moment.
@@ -141,7 +137,7 @@ public unsafe BitArray(bool[] values) | |||
|
|||
if (Vector256.IsHardwareAccelerated) | |||
{ | |||
for (; (i + Vector256ByteCount) <= (uint)values.Length; i += Vector256ByteCount) | |||
for (; (i + Vector256<byte>.Count) <= (uint)values.Length; i += (uint)Vector256<byte>.Count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (; (i + Vector256<byte>.Count) <= (uint)values.Length; i += (uint)Vector256<byte>.Count) | |
for (; (i) <= (uint)values.Length - Vector256<byte>.Count; i += (uint)Vector256<byte>.Count) |
With the suggestion i
can be used from a register directly, same for the length-count.
With the code as is i
needs to be added first, then compared.
See the difference in M1 and M2 from sharplab (silly) example.
Same on other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the suggested change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tannergooding do you wat this to be in the scope of this PR? this PR was about upgrading the APIs mentioned in the description. This is surely a optimization opportunity but just checking if adding this here is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's simply enough to include here. But generally speaking it's nice to separate the two considerations as it helps us more easily track performance differences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohkay. They are already added.
…ested changes so the i in the for loop can use a single register
@DeepakRajendrakumaran @Ruihan-Yin for the review |
src/libraries/System.Collections/src/System/Collections/BitArray.cs
Outdated
Show resolved
Hide resolved
I've asked for a secondary review on this and it should be mergeable after that happens. |
This PR is about adding Vector512 support to the existing BitArray library APIs.
Upgrading the following APIs
PERF
AND
OR
NOT
XOR
CopyTo(bool[] arr)