Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Intrinsicify SequenceCompareTo(char) #22876

Closed
wants to merge 3 commits into from

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Feb 27, 2019

From #22187

Based on #22875 which is first commit

Resolves https://github.com/dotnet/coreclr/issues/22763

@benaadams
Copy link
Member Author

@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x64 Checked jitnox86hwintrinsic

@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x86 Checked jitnox86hwintrinsic

@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Ubuntu x64 Checked jitnox86hwintrinsic

@stephentoub
Copy link
Member

Perf numbers, best and worst cases?

@stephentoub
Copy link
Member

cc: @tannergooding, @GrabYourPitchforks

@tannergooding
Copy link
Member

Going to wait to review this until after #22816. It looks to be many of the comments are going to be the same overall.

@benaadams
Copy link
Member Author

Will follow up after #22505

@stephentoub
Copy link
Member

@benaadams, what's your plan for this series of PRs? Should they be closed for now?

@benaadams
Copy link
Member Author

Still planning on them; some urgency was lost when 3.0 moved to tell mode

@stephentoub
Copy link
Member

Still planning on them; some urgency was lost when 3.0 moved to tell mode

Thanks. Do you plan to get to them in the next week and a half?
https://github.com/dotnet/coreclr/issues/27549

@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your coreclr repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

@benaadams benaadams force-pushed the SequenceCompareTo-char branch 2 times, most recently from 005c990 to b0686c5 Compare November 9, 2019 18:41
@benaadams benaadams force-pushed the SequenceCompareTo-char branch from b0686c5 to f24e9d6 Compare November 9, 2019 19:20
@benaadams
Copy link
Member Author

Rebased ready for review /cc @tannergooding @CarolEidt @GrabYourPitchforks

Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM but I'd suggest additional comments

{
if (Vector.IsHardwareAccelerated && (byte*)minLength >= (byte*)Vector<ushort>.Count)
if (lengthToExamine >= Vector256<ushort>.Count)

Choose a reason for hiding this comment

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

IMO this could use some additional comments to explain how this handles non-multiples of vector size (i.e. that the last compare may overlap the previous, but it's still correct).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added. Realized I could delete the additional Sse2 block in the Avx2 when adding the comment as it can fall through to the existing Sse2 one.

Choose a reason for hiding this comment

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

Excellent!

@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.

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.

Use Intrinsics.X86 for SpanHelpers.SequenceCompareTo(char)
6 participants