Skip to content
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

Add plain Vector128 support to IndexOfAnyAsciiSearcher #92680

Closed
wants to merge 1 commit into from

Conversation

MihaZupan
Copy link
Member

Contributes to dotnet/perf-autofiling-issues#21818

I believe this should be an okay implementation correctness-wise, but it's up to Mono implementations how efficiently they can handle the Vector128 paths (e.g. efficiently handing Vector128.Shuffle with non-const indices).

@fanyang-mono could you please check how this impacts Mono perf?
We shouldn't merge this without good validation as it will affect most uses of SearchValues.

@MihaZupan MihaZupan added this to the 9.0.0 milestone Sep 27, 2023
@MihaZupan MihaZupan self-assigned this Sep 27, 2023
@ghost
Copy link

ghost commented Sep 27, 2023

Tagging subscribers to this area: @dotnet/area-system-buffers
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to dotnet/perf-autofiling-issues#21818

I believe this should be an okay implementation correctness-wise, but it's up to Mono implementations how efficiently they can handle the Vector128 paths (e.g. efficiently handing Vector128.Shuffle with non-const indices).

@fanyang-mono could you please check how this impacts Mono perf?
We shouldn't merge this without good validation as it will affect most uses of SearchValues.

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Buffers

Milestone: 9.0.0

@fanyang-mono
Copy link
Member

For initial validation, please run affected microbenchmarks from dotnet/perf-autofiling-issues#21818 using this script https://github.com/dotnet/performance/blob/main/scripts/benchmarks_local.py

You should run them against both Mono JIT and Mono interpreter. It could be achieved by passing MonoJIT or MonoInterpreter to the switch --run-type

@MihaZupan
Copy link
Member Author

@MihuBot

@@ -42,7 +42,7 @@ internal override int IndexOfAny(ReadOnlySpan<char> span)

// We check whether the first character is ASCII before calling into IndexOfAnyAsciiSearcher
// in order to minimize the overhead this fast-path has on non-ASCII texts.
if (IndexOfAnyAsciiSearcher.IsVectorizationSupported && span.Length >= Vector128<short>.Count && char.IsAscii(span[0]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep IsSupported checks to ensure trimming works as expected on platforms without SIMD support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that needed even though all its ctor uses are guarded behind it?

Copy link
Member

@tannergooding tannergooding Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2R has some special rules for various ISA checks and I'm pretty sure we expect them to be used even when all callers are expected to guard a given path.

CC. @davidwrighton

@ghost ghost closed this Oct 29, 2023
@ghost
Copy link

ghost commented Oct 29, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants