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

Fix bitmask vector bit order #380

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Fix bitmask vector bit order #380

merged 1 commit into from
Jan 29, 2024

Conversation

calebzulawski
Copy link
Member

Fixes #379

@calebzulawski
Copy link
Member Author

@RalfJung looks good?

@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2023

I honestly can't tell, I don't really understand the code in this library. We'll see if this makes the Miri tests pass -- once it propagates into the rustc repository.

I also have no idea what should happen for cases like N == 12. Should the first or the last element of the array be the one that is only partially filled? I thought it'd have to be the first element, but then I looked at an example: on little endian, mask 0b001101001001 is the same as [0b01001001, 0b0011], and on big endian the same mask is written 0b100100101100 as a single integer and [0b10010010, 0b1100] as an array. Looks like indeed converting the arrays works by reversing the bits in each element, and then fixing up the last element as that is always the one that is "partial". But the order of array elements is the same in both cases. I don't quite understand why...

@calebzulawski
Copy link
Member Author

Yeah, I'm not sure what makes the most sense either. Fwiw size 12 simply doesn't work yet, something is wrong with the intrinsic. When I fix the intrinsic I expect the last element to be the one needing fixing, because LLVM simply specifies the vector element ordering to be dependent on endianness (not just bitmasks, also concatenating vectors into a single integer). An alternative implementation could reverse the vector rather than the bitmask, if that makes it any clearer.

@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2023

I'm fine with whatever as long as it is consistent between simd_bitmask and simd_select_bitmask, and documented properly. :)

Miri anyway only supports powers of 2 currently.

@workingjubilee
Copy link
Member

We should explain why this is the correct behavior before we commit it, which means expanding our written spec for simd_select_bitmask and simd_bitmask.

@calebzulawski
Copy link
Member Author

Perhaps we should expose the intrinsics in something like core::intrinsics::simd and use that documentation as the actual semantics

@calebzulawski calebzulawski merged commit f55ca30 into master Jan 29, 2024
66 checks passed
@RalfJung
Copy link
Member

RalfJung commented Feb 17, 2024

Any estimate when this will be propagated to the rustc repo? :)

@calebzulawski
Copy link
Member Author

I just opened rust-lang/rust#121269

bors added a commit to rust-lang/miri that referenced this pull request Feb 19, 2024
enable from_bitmask_vector test on little-endian targets

Blocked on rust-lang/portable-simd#380 propagating to the rustc repo
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Feb 25, 2024
…Jung

enable from_bitmask_vector test on little-endian targets

Blocked on rust-lang/portable-simd#380 propagating to the rustc repo
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.

from_bitmask_vector on big-endian calls simd_select_bitmask with the mask at the wrong end of the byte
3 participants