Skip to content

Feature/simplify masks #99

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

Merged
merged 4 commits into from
May 1, 2021
Merged

Feature/simplify masks #99

merged 4 commits into from
May 1, 2021

Conversation

calebzulawski
Copy link
Member

Remove explicit bitmasks.

Add ToBitMask trait which allows you to convert masks to u64.

@programmerjake
Copy link
Member

what happens in the future when we get 256-element vectors? are masks just not supported?!

@calebzulawski
Copy link
Member Author

I'm not sure. I'm still thinking about that. Unfortunately there's basically no way to represent that in the rust type system as far as generics go. Related to #50

@programmerjake
Copy link
Member

I'm not sure. I'm still thinking about that. Unfortunately there's basically no way to represent that in the rust type system as far as generics go. Related to #50

An idea for fixing that:
#50 (comment)

@workingjubilee
Copy link
Member

@programmerjake We will absolutely successfully work out a version that does not break on 2048-bit vectors with byte lanes before we stabilize anything (or else over my dead body, I suppose). You can expect to see this API change a lot, please do not read too much into any single version. Right now, we're trying things out a lot and seeing how things work.

"How to handle masks on platforms" is definitely going to be an Unresolved Question in our RFC.

@calebzulawski
Copy link
Member Author

calebzulawski commented Apr 19, 2021

I think there are two separate things here that we need to consider.

One is that masks of any (reasonable) vector length should be supported, regardless of their implementation. That could be solved by using a trait like that (it may be appropriate as part of LanesAtMost32 or whatever we end up calling it).

But I think there is a separate problem of explicitly exposing a mask as an integer bitmask. This is the SSE movemask example, where you use your bitmask as an index to a lookup table or similar. It's possible to have a 256-lane vector, but I don't think it's useful to expose an entire 256-bit "integer". Or, hypothetically, if we support Mask8<65536> in the future, I don't think there's any (portable!) way of exposing that. On AVX-512 is that 2048 k registers placed into an array in memory? I can't think of many (portable!) reasons you'd need to access that as a [u8]. This is what the ToBitMask trait is for, converting masks with up to 64 lanes into u64s, independently of the maximum supported number of mask lanes.

@calebzulawski calebzulawski mentioned this pull request Apr 22, 2021
@calebzulawski calebzulawski force-pushed the feature/simplify-masks branch 2 times, most recently from 2c8c37a to 676956d Compare April 22, 2021 23:46
@calebzulawski calebzulawski marked this pull request as ready for review April 22, 2021 23:48
@calebzulawski
Copy link
Member Author

Anyone know why the MIPS masks seem to be reversed when packed to an integer? Somehow an LLVM bug or is there something peculiar about MIPS?

@programmerjake
Copy link
Member

IDK, but maybe it's related to Ada bit array ordering?

@workingjubilee
Copy link
Member

...Ada bit array?

@programmerjake
Copy link
Member

...Ada bit array?

They describe it some here:
https://en.wikibooks.org/wiki/Ada_Programming/Types/array#With_aliased_elements

@calebzulawski
Copy link
Member Author

I think that leaves me with more questions than I started with. What do Ada arrays have to do with this?

@programmerjake
Copy link
Member

Well, when discussing which order to have bitmasks in SimpleV, lxo (one of the GCC Ada compiler engineers who's helping out with Libre-SOC) mentioned that Ada uses MSB to LSB ordering for bit arrays -- I figured that might be related since it uses the same order as what you apparently noticed in MIPS.

@calebzulawski
Copy link
Member Author

Hmm. I'd hope that's not related to the problem. I would expect LLVM to maintain consistent bit field ordering across architectures.

@workingjubilee
Copy link
Member

workingjubilee commented Apr 28, 2021

Can you rebase this, please?

@calebzulawski calebzulawski force-pushed the feature/simplify-masks branch from 676956d to eec4280 Compare April 28, 2021 22:13
@calebzulawski
Copy link
Member Author

Okay. "Simplify" might be a bit of a misnomer now (but the API is definitely simplified!)

I used a Mask helper trait to mostly fix #50. I didn't use const_evaluatable_unchecked to implement the trait over all masks, but it should be possible in the future, effectively solving that problem.

Bit masks are now implemented as arrays of u8 and can be exposed to the users of stdsimd that way, too. If you want an integer you'll have to do the conversion yourself (not that big of a deal). Due to limitations in the simd_bitmask and simd_select_bitmask intrinsic, Mask has an extra IntBitMask associated type, which is purely used internally. We should be able to remove that after improving the intrinsics to accept arrays of u8 as bitmasks as well.

@workingjubilee workingjubilee force-pushed the feature/simplify-masks branch from 9be0c5c to 589fce0 Compare May 1, 2021 06:22
@workingjubilee
Copy link
Member

This wound up not simplifying things as much as intended internally but it looks much better to me as an external API, really, which is what counts here. Can you add "modify Rust SIMD intrinsics to accept u8-or-something bitmasks" to #46 with a note about all the ones that need to get touched?

@workingjubilee workingjubilee merged commit 9a063bc into master May 1, 2021
@calebzulawski calebzulawski deleted the feature/simplify-masks branch August 7, 2021 18:19
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