Skip to content

Conversation

@EdsterG
Copy link
Contributor

@EdsterG EdsterG commented Sep 7, 2025

Following up on the discussion in #141 (comment), this PR defines:

bitmaskconvert(UInt, SIMD.Vec)
bitmaskconvert(SIMD.Vec, UInt)

This function replaces bitmask and provide a general interface for converting from a vector of booleans to a uint and vice versa.

There are specific cases where it's very convenient to avoid specifying the output type, for those cases tobitmask(SIMD.Vec) and frombitmask(UInt) were added. However, these are mostly "convenience" functions so they aren't strictly necessary.

@KristofferC
Copy link
Collaborator

KristofferC commented Sep 10, 2025

The functionality is good but the names sext and zext are kind of obscure imo. Maybe frombitmask or bitmask_to_vec or something?

Also, test + README update needed.

@KristofferC
Copy link
Collaborator

(I also don't think the sext version is really needed?)

@eschnett
Copy link
Owner

This function should probably have the same semantics as a regular Bool -> Int conversion, i.e. it should probably use the zext semantics. If you need sext(x) then you could do -zext(x), assuming that LLVM is clever enough to create efficient code.

@EdsterG EdsterG force-pushed the int_to_vec branch 5 times, most recently from 1f05bb5 to f78692f Compare September 19, 2025 19:35
@EdsterG
Copy link
Contributor Author

EdsterG commented Sep 19, 2025

This function should probably have the same semantics as a regular Bool -> Int conversion

I replaced bitmask with bitmaskconvert which has the same semantics as a regular Bool -> Int conversion (tests and README have been updated).

I also really like the convenience of not having to specify the output type in specific cases, so I added tobitmask/frombitmask.

If you need sext(x) then you could do -zext(x), assuming that LLVM is clever enough to create efficient code.

Looks like LLVM is indeed clever enough to create efficient code with -zext(x)!

From my end this PR is ready for another review.

@EdsterG
Copy link
Contributor Author

EdsterG commented Sep 25, 2025

@eschnett @KristofferC would you take another look at this PR?

@KristofferC
Copy link
Collaborator

Imo, the bitmaskconvert is not needed; the previous version with only the two separate functions was better.

@EdsterG
Copy link
Contributor Author

EdsterG commented Sep 25, 2025

Here's my reasoning for bitmaskconvert. The previous version of bitmask(Vec) converted an arbitrary length vector to an integer mask. If the vector was not a power of 2 it would automatically round up to the nearest power of 2 and zero extend. Since the inverse operation has no way of knowing the original vector length, the interface had be asymmetric. The user must specify the output vector in frombitmask(Vec,UInt).

In other words:

  • bitmask handled the case where N < sizeof(UInt) * 8 and N == sizeof(UInt) * 8
  • frombitmask handled all three cases N < sizeof(UInt) * 8, N == sizeof(UInt) * 8 and N > sizeof(UInt) * 8

The omission of the case N > sizeof(UInt) * 8 from bitmask seemed quite arbitrary so I added a bitmaskconvert intrinsic to cleanly handle all the cases in both directions. IMO the symmetry of bitmaskconvert makes it a cleaner interface, and has the added benefit of removing the weird "gotcha" for the 1 missing case described above.

@EdsterG
Copy link
Contributor Author

EdsterG commented Oct 1, 2025

@KristofferC thanks for the feedback. Would you please provide a little more detail behind your opinion? Are you strictly opposed to the name? If so, here are some alternatives.

  • Option 1: rename bitmaskconvert to bitmask(UInt, SIMD.Vec) and bitmask(SIMD.Vec, UInt).
  • Option 2: rename bitmaskconvert to tobitmask(UInt, SIMD.Vec) and frombitmask(SIMD.Vec, UInt).
  • Option 3: if nether option is satisfactory please suggest an alternative.

If you're opposed to something other than the name, I'd love to hear your reasoning and possible options to get this feature in.

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