Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement vpmaxq_u8 on aarch64 #4193

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Feb 14, 2025

This is documented in https://developer.arm.com/architectures/instruction-sets/intrinsics/#q=vpmaxq_u8

This makes all memchr tests suite pass when I remove #[cfg(not(miri))] from tests.

Fixes #4191

@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Feb 16, 2025
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 21, 2025

Turns out the memchr test failures were because I was accidentally doing signed rather than unsigned comparisons. All tests are passing now.

@bjorn3 bjorn3 marked this pull request as ready for review February 21, 2025 11:22
@bjorn3 bjorn3 added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Feb 21, 2025
@RalfJung
Copy link
Member

Please add a test; you can use the x86 intrinsic tests as template. In particular there should be a Miri test directly checking the signed vs unsigned issue.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Feb 21, 2025
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 21, 2025

Added tests.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Feb 21, 2025
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

LGTM, just a comment nit.

}

// Used to implement the vpmaxq_u8 function.
// Folding maximum of adjacent pairs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Folding maximum of adjacent pairs.
// Computes the maximum of adjacent pairs; the first half of the output is produced from the first
// input, the second half of the output from the second input.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support memchr on arm64 too
3 participants