Bitmask conversion trait

[calebzulawski]

Another approach that fixes #223, as an alternative to #238.

This adds the ToBitMask trait, which is implemented on a vector for each bitmask type it supports. This includes all unsigned integers with enough bits to contain it. The byte array variant has been separated out for now into #246 and still requires generic_const_exprs, but the integer variants no longer require it and can make it to nightly.

[thomcc]

I don't really see why this needs to be a generic trait. It feels like a degree of freedom that gives very little and will make understanding the API more complex than it needs to be.

It seems like it mostly is this way to save a as or .into() call on the returned integer, which doesn't really feel like it's worth the indirection and API surface.

Concretely: is there a reason Bitmask can't just be an associated type on ToBitmask, which just is implemented once per vector with bitmask being the unsigned integer that fits all the bits? (at least for the current vector sizes)

[thomcc]

I also kind of still oppose using [u8; N] for bitmasks for various reasons: byte-order, it seems to have confused most people who came across the existing API, and it won't easily support a lot of natural operations you want to perform on the resulting bitmask (like counting leading/trailing zeros/one and such)...

Is there a reason we want that? I also don't think it would be the end of the world if Simd<T, 256> had to use [u8; N] if we can't come up with a better approach by then (although there are for sure other options there too), but until then we still have u128.

[calebzulawski]

Well, we've had [u8; N] bitmasks for quite a while, this PR doesn't add them. Yeah, the reason is so that you can use them for vectors wider than 64 lanes (There is u128, but we've had repeated issues with it so I'm not counting on it. What about 512+ lanes?)

Regarding complexity, I'm not sure ToBitMask<T> is really any more complicated than ToBitMask<BitMask=T>? The benefit of always offering arrays is being able to use bitmasks in functions generic over lane count.

[calebzulawski]

I did just consider, however, that we could offer two separate traits, one for conversion to integer bitmasks, and one to arrays. Then it would look something like ToBitMask<BitMask=u64> and ToBitMaskArray<const BYTES = 8>.

[thomcc]

I actually think that has a good chance of being less confusing, honestly.

(I do still think that [u8; N] would be very inconvenient to use in very many cases though...)

[thomcc]

At the risk of regretting saying it, I suspect that BitMask<const N: usize> would be a lot better for code generic over lane count.

Like, I pretty much always want to figure out stuff like the leading/trailing zeros/ones, in order to find the index of highest/lowest lane (in whatever produced the mask) for which the predicate that generated thet BitMask returned true/false... (And a dozen other things that just suck to do with [u8; N]).

That said we already have many masks... (Actually, I wonder if those operations are just better on Mask<T, N>, and if they need to convert to bitmask internally to answer that question, they can, but aren't forced to)

I guess much of this came up in the The Great Simd Mask Bikeshed Of 2021...

[calebzulawski]

If that's the specific operation you want, then you can cast the array to [u64; N] etc and run those operations. Another common thing would be to match on specific values, which you could use the byte array directly for. I think the point of providing it directly as a byte array is it's not restrictive about what you can do with it, for the same reason that we return integer bitmasks as integers at all and not just an opaque type.

[workingjubilee]

I would like to see core_simd outmode the use of feature flags rather than require allowing warnings on some feature, since they are of no additional benefit for actual core inclusion.

[calebzulawski]

In retrospect, it was pretty easy removing generic_const_exprs (from the bitmask stuff, to_bytes still needs it). Sometimes takes a few tries...

[calebzulawski]

Of course mips failed again, not just endianness but completely reversed bit order...

[programmerjake]

if you assert, you get a safe function afaict.

Suggested change

	pub unsafe fn to_bitmask_array<const N: usize>(self) -> [u8; N] {
	pub fn to_bitmask_array<const N: usize>(self) -> [u8; N] {
	assert_eq!(N, LaneCount::<LANES>::BITMASK_LEN, "invalid bitmask array size");

[calebzulawski]

I'm not sure the assertion actually makes it safe--the intrinsic would still be (invalidly) lowered. It should cause a monomorphization error, but I didn't want to rely on that.

[programmerjake]

having transmute_copy of a wrong-sized object in a code-path that's never executed is at least just as safe as calling unreachable_unchecked -- perfectly safe imho.

pub fn this_is_safe() {
    assert!(1 == 2);
    unsafe { unreachable_unchecked() }
}

[workingjubilee]

to_bitmask_array feels like a more confusing name than what we had before (i.e. giving a bitmask, but as bytes) since it may imply, since many masks are 1 byte per lane, somehow lane masks, but as a set of bytes...? I think the "bitmask" terminology doesn't adequately distinguish it from the "width = lane" mask, since those are also used in the same ways that "bitmasks" are used elsewhere in programming.

If we are going to have such a verbose method, maybe to_bitmask_as_bytes.

[calebzulawski]

I'm okay with that name too, or any other suggestions.

[calebzulawski]

I'm not going to resolve this whole thread because of the naming issue, but the original concern about the function being safe has been addressed.

[programmerjake]

Of course mips failed again, not just endianness but completely reversed bit order...

icr where I posted the link to the relevant code in llvm, but llvm arbitrarily decided that bit order always follows byte endianness. it might be nice to mention that in the code comments somewhere

[workingjubilee]

I kind of want this, but I also kind of am starting to get the vibe of "did we lose the plot somewhere?" Insofar as whether this is an alternative or exception to the other PR, as I see it, there is no reason this PR cannot be rebased atop the other one, so I am inclined to say we should wave Jorge's through first, collect some user feedback, and work on this some more.

[workingjubilee]

to_bitmask_array feels like a more confusing name than what we had before (i.e. giving a bitmask, but as bytes) since it may imply, since many masks are 1 byte per lane, somehow lane masks, but as a set of bytes...? I think the "bitmask" terminology doesn't adequately distinguish it from the "width = lane" mask, since those are also used in the same ways that "bitmasks" are used elsewhere in programming.

If we are going to have such a verbose method, maybe to_bitmask_as_bytes.

[workingjubilee]

My apologies, I would like to downscale all previous remarks in import and instead observe that this code induces an ICE when I issue the command cargo doc.

[calebzulawski]

I kind of want this, but I also kind of am starting to get the vibe of "did we lose the plot somewhere?" Insofar as whether this is an alternative or exception to the other PR, as I see it, there is no reason this PR cannot be rebased atop the other one, so I am inclined to say we should wave Jorge's through first, collect some user feedback, and work on this some more.

I realized that I forgot the critical component of this--bitmasks now have nothing to do with the magic LaneCount type and are explicitly defined by these two traits.

[calebzulawski]

Actually, I realized I jumped the gun on that because of the limitations of our bitmask-layout masks, but let me explain what I mean:

<LaneCount<LANES> as SupportedLaneCount>::BitMask is not user-friendly, it only exists there so that we can implement bitmask-layout masks with the limitations of today's const generics. With that in mind, I don't think to_bitmask() should return that type. The ToBitMask::BitMask type is what the user can directly reference as the return type, and even when const generics are improved and we can remove anything related to bitmasks from LaneCount, that type will remain.

The difference between this PR and #238 is this one doesn't expose that ugly implementation detail that we'd really like to remove ASAP.

[calebzulawski]

The commits got a little messed up since I removed a broken commit, but I was able to make all of the bitmask functions safe by making ToBitMask an unsafe trait that specifies the required bitmask types, and using that trait to determine correct bitmask types.

[programmerjake]

other than maybe renaming ToBitmask to ToIntBitmask since it only works for a few specific lane counts due to us not having generic integers yet so people will wonder why it doesn't work on f32x4 or similar, this looks good to me!

[programmerjake]

My apologies, I would like to downscale all previous remarks in import and instead observe that this code induces an ICE when I issue the command cargo doc.

maybe we need to add doc testing to our CI?

[workingjubilee]

<LaneCount<LANES> as SupportedLaneCount>::BitMask is not user-friendly, it only exists there so that we can implement bitmask-layout masks with the limitations of today's const generics. [..]

The difference between this PR and #238 is this one doesn't expose that ugly implementation detail that we'd really like to remove ASAP.

Ah! That makes this all make more sense then, yes, if that is the "leading edge" of this.

[workingjubilee]

Mmm, I tried building this on a subtree pull into std and I can confirm it breaks, so we can't land this as-is. It is almost entirely lamenting about the way ToBitMaskArray works. And it's not even getting as far as building and running rustdoc, for note, just in core. Possibly because it has to handle that during bootstrap...

If you split out the ToBitMaskArray impl into another PR and rebase this one accordingly, we can probably push it in, in stages, otherwise not so much.
💭 ( ToBitArray::to_bitarray? )

[calebzulawski]

@workingjubilee I removed bitmask arrays from this PR, to be handled in #246