Skip to content

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

@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

4 participants