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

shift_elements_{left,right} #441

Merged
merged 6 commits into from
Sep 28, 2024

Conversation

sammysheep
Copy link
Contributor

This implementation was patterned after the pre-existing rotate_elements_{left,right} and is meant to supersede #112 from 2021. If this functionality is buried somewhere else in the API that I missed, please close with a note where I can find it.

The tests pass and the assembly looked reasonable on x86.

@calebzulawski
Copy link
Member

Not to say we shouldn't have this function, but (devil's advocate) slices for example only have rotating elements, not shifting in default. You could technically use our already implemented SIMD rotates, and then another swizzle to insert the default values.

Should this maybe shift in an arbitrary value, rather than Default?

@sammysheep
Copy link
Contributor Author

Ah, this PR came about because I wrote an extension trait to do this for an algorithm that needs to shift by one element to the left and continue on its merry way. The C code I was porting just used a single intrinsic for what amounts to the same thing, so that motivated me to look for it in portable SIMD's API.

Perhaps also, adding an explicit shift_elements function might help users to stop and realize that the Shl / << operator works within each element bitwise and not element-wise. This was an actual mistake I made a few months ago and only realized it after restarting my port project again yesterday. You /can/ use rotate++ as you suggest, but I wrote the extension trait exactly because my doing so would have felt like the wrong level of abstraction.

As to using default(), the previous impl in #112 inspired me that it was an easy and natural way to provide zero-padding. However, I'm not stuck on that. If one were to make the pad value arbitrary, would it just need to be another const parameter I suppose?

If so, would that rule out things that are invalid as a const generic parameter like f64?

Happy to make whatever desired changes based upon your own design goals if you decide to keep this PR.

@programmerjake
Copy link
Member

However, I'm not stuck on that. If one were to make the pad value arbitrary, would it just need to be another const parameter I suppose?

no, it can just be a function argument, no need for it to be const.

@sammysheep
Copy link
Contributor Author

I have changed it to accept a padding argument and updated the tests and docs. This seems equivalent for the zero case on x86_64: https://godbolt.org/z/E8h9onese

crates/core_simd/src/swizzle.rs Outdated Show resolved Hide resolved
crates/core_simd/src/swizzle.rs Outdated Show resolved Hide resolved
crates/core_simd/src/swizzle.rs Outdated Show resolved Hide resolved
@calebzulawski calebzulawski merged commit 158e240 into rust-lang:master Sep 28, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants