google / xls

XLS: Accelerated HW Synthesis
http://google.github.io/xls/
Apache License 2.0
1.21k stars 179 forks source link

Conversion between bool[N] and bits[N] should allow control over ordering #1718

Open mikex-oss opened 1 week ago

mikex-oss commented 1 week ago

What's hard to do? (limit 100 words)

In XLS, we have a distinction between an array of bools bool[N] (i.e. bits[1][N]) and a packed bit vector bits[N]. Oftentimes, you may choose to write one or the other depending on convenience (e.g. arrays support map, bits support and/or reduce). But in these cases, the order of indices is normally fixed regardless of whether we write it as one version or the other.

stdlib provides convert_to_bools_lsb0 and convert_to_bits_msb0 to convert between the two, but it'd be cleaner to have that ordering user-specified.

Current best alternative workaround (limit 100 words)

You can use rev or array_rev before or after the conversion functions, to undo the reversing.

Your view of the "best case XLS enhancement" (limit 100 words)

Provide APIs to convert between the two without extraneous reversing.

cdleary commented 1 week ago

@mikex-oss I don't understand what this issue is asking for concretely -- we have the two stdlib functions that state what they do explicitly. You're saying we should outlaw the cast conversion, in order to make sure users specify explicitly?

mikex-oss commented 1 week ago

It's talking about this ambiguity and that we should provide both options for the user.

So, it's somewhat ambiguous whether "index 0" in the array would become the least significant bit or the most significant bit.

That is, provide

fn convert_to_bits<N: u32>(x: bool[N], set_index0_to_msb: bool) -> uN[N]

or if preferred, provide both convert_to_bits_msb0 and convert_to_bits_lsb0.

Same for the other direction.

As mentioned, there are plenty of cases where the user just wants the bit at index 0 (regardless of whether it's an array or bits) and it doesn't matter if it's backwards "on paper". The workaround is to do stuff like:

rev(std::convert_to_bits_msb0(my_bool_array)) or std::convert_to_bits_msb0(array_rev(my_bool_array)).

cdleary commented 1 week ago

@mikex-oss Ok I see, so you're proposing we switch it from two functions to one function that takes a boolean? I think the boolean is less clear TBH (you always ask "what does true vs false mean for this function") -- an enum seems like it would be fine, but then also the user would need to do std::Index0::IsMsb or something which is maybe longer in the end. No particular preference.

cdleary commented 1 week ago

@mikex-oss Or on further reflection I now realized you're saying "at least offer the full set, even if we keep this naming scheme", that SGTM too! Thanks for explaining.