Add bitmask i{N <8} -> u8 impls #250
Conversation
crates/core_simd/src/intrinsics.rs
Outdated
| // | ||
| // The bit order of the result depends on the byte endianness, LSB-first for little | ||
| // endian and MSB-first for big endian. | ||
| // Not intended to be called on a vector with values other than 0 and -1. |
There was a problem hiding this comment.
Is "Not intended to be called" any different from "it is UB to be called that way"?
If 0 and -1 are the only possible values, the "returns the most significant bit (MSB)" part seems unnecessarily specific -- basically this returns whether each lane is logically true or false, right?
There was a problem hiding this comment.
The commentary before that is a barely-trimmed copy of the comments from the implementation.
So at the current moment, it's implemented in such a way that it is not UB to do that.
"Not intended to be called" is me adding an ominous suggestion that I am reconsidering whether it should be well-defined or not.
There was a problem hiding this comment.
I think there's an unwritten contract that the mask arguments must always be valid masks. We should probably just say "don't do it". It could certainly end up being UB in another future backend. All of the intrinsics are poorly documented but I don't think we want to give the impression you can pass weird inputs to them without it being UB.
There was a problem hiding this comment.
Well, specifically when implementing them in Miri one needs to know what is and is not UB. :)
There was a problem hiding this comment.
I think so, yeah.
As far as I know Rust is currently against partially init scalar data, so a u8 is either u8 or is MaybeUninit<u8>. I could see us getting uint<4> and then not worrying about the next 4 bits and making simd_select_bitmask legal on that, but as far as I understand the current Rust model for uninit data, the entire vector risks becoming uninit if we ever passed a single actually uninit bit into the mask for simd_select_bitmask, and simd_bitmask is implicitly wedded to the fate of that.
There was a problem hiding this comment.
I checked and we're currently the only actual users of this intrinsic so yeah, we can just define it as UB. Bam.
...and copy the notes for why they're legal.
workingjubilee
force-pushed
the
smol-bitmasks
branch
from
4d0397d to
d411ef5
Compare
...and copy the notes for why they're legal.
...and copy the notes for why they're legal.
I think this should be fine to Just Do but I want to make sure that if this is wrong or problematic that someone pokes me about it soooooooooooooooo I guess I am poking everyone today:
cc @calebzulawski @programmerjake @RalfJung