Skip to content

Conversation

@pentp
Copy link
Contributor

@pentp pentp commented Jan 13, 2026

In case of WideningLoader it currently uses Vector128<TLeft>.Count (16) as the minimum length, but 8 bytes/chars is the actual minimum.
Also removed Count128/Count256/Count512 as they are not useful - both vectors must always advance by the same amount anyway.

Copilot AI review requested due to automatic review settings January 13, 2026 01:50
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 13, 2026
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 13, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug in the vectorization of Ascii.Equals where the code incorrectly used Vector128<TLeft>.Count as the minimum length threshold for the WideningLoader case. Since WideningLoader converts bytes to ushorts, TLeft=byte has a count of 16 while TRight=ushort has a count of 8. This caused the code to skip vectorization for byte-to-char comparisons with lengths between 8 and 15 elements, falling back to the slower scalar path unnecessarily.

Changes:

  • Fixed the minimum length check to use Vector128<TRight>.Count instead of Vector128<TLeft>.Count
  • Replaced all TLoader.Count128/256/512 references with Vector<TRight>.Count to ensure both left and right search spaces advance by the same amount
  • Removed the now-unused Count128, Count256, and Count512 properties from the ILoader interface and its implementations
  • Corrected spelling errors: "loose" → "lose"

@EgorBo
Copy link
Member

EgorBo commented Jan 13, 2026

In case of WideningLoader it currently uses Vector128.Count (16) as the minimum length, but 8 bytes/chars is the actual minimum.

is the issue here that vectorization path doesn't kick in when it should or it's a correctness fix?

@pentp
Copy link
Contributor Author

pentp commented Jan 13, 2026

The issue is that vectorization path doesn't kick in, but the result is correct either way.

@stephentoub stephentoub added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 13, 2026
@dotnet-policy-service
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Runtime community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants