Add Protocols.MemoryMap.Mask

[martijnbastiaan]

In Clash designs we often use Vec n Bool to track per-bit enables, valid bits, and similar. The default BitPackC representation of Vec n Bool allocates a full byte per bit, which is wasteful when these end up in memory-mapped registers. Switching to a BitVector is denser, but inverts the index correspondence: Vec index 0 maps to the MSB of the bitvector via Clash's standard pack/unpack, so a loop walking 0..n on the Haskell side walks the wrong direction relative to firmware that pokes individual bits.

Mask fixes both problems: it is packed compactly (like BitVector), and its toVec / fromVec / toBitVector / fromBitVector conversions all preserve the Vec-index <-> bit-position correspondence: index 0 is bit 0 (the least significant bit).

Lastly, it introduces a mask-focused interface on the Rust side.

Dear reviewer
I'm currently extending the native types clash-protocols-memmap and friends know about. This is because in the future I'd like to have Masks operate more efficiently for tasks common to them, e.g., testing a single bit. As far as I understand, the complete structure is currently fetched, after which the bit is tested. IMO that's out of scope for this PR though.

Note that the type is backed by Unsigned. I had originally implemented backed by BitVector, but this requires modulo operations in software. @hydrolarus pointed out that this isn't true because you can translate %8 to & 0b111.

AI disclaimer
You bet!

TODO

• Write (regression) test
• Update documentation, including docs/
• Link to existing issue
• Use BitVector-like backend in Rust
• Add Mask![n] marcos

[hydrolarus]

Overall looking good! Nice work. Is the idea to eventually have this type be in clash-prelude?

[martijnbastiaan]

Is the idea to eventually have this type be in clash-prelude?

Oh, I didn't even think of that! I'll open a Discussion.

[hydrolarus]

As discussed off-PR, there's still some changes to be made, but here's still a naming thing I noticed!

[rslawson]

+1 to Lara's comments about test names, otherwise just some cleanup.

[rslawson]

I'd prefer to see these be a little more flexible on the size of the type of other. Maybe something like this?

impl<const M: u8, const N: u8, T, U> From<Unsigned<M, T>> for Mask<N, U>
where
    Unsigned<M, T>: UnsignedSizeCheck,
    Unsigned<N, U>: UnsignedSizeCheck,
    U: FromAs<T>,
{
    fn from(other: Unsigned<M, T>) -> Mask<N, U> {
        let _ = Unsigned::<M, T>::SIZE_CHECK;
        let _ = Unsigned::<N, U>::SIZE_CHECK;
        let _ = const {
            // ensure that M <= N here, else panic
        };
        if const { M == N } {
            Mask::<N, U>::from_unsigned(other)
        } else {
            Mask(Unsigned(other.0))
        }
    }
}

As well as From<Mask<N, U>> for Unsigned<M, T>.

[martijnbastiaan]

        let _ = const {
            // ensure that M <= N here, else panic
        };

That's super interesting Rust lets you do that! I think it means that this will trigger a compile error, right?

[martijnbastiaan]

Could you check whether the current implementation is what you intended?

[rslawson]

        let _ = const {
            // ensure that M <= N here, else panic
        };

That's super interesting Rust lets you do that! I think it means that this will trigger a compile error, right?

Exactly, yep! It'll fail to evaluate the const { ... } block so it'll produce an error at comptime. You can just write something like

let _ = const {
    if M > N {
        panic!("Cannot infallibly convert if the `Unsigned` is larger than the `Mask`!");
    }
};

And then it'll fail to compile at call sites that are invalid.

Could you check whether the current implementation is what you intended?

This is still missing from the current implementation.

[martijnbastiaan]

Sorry I messed up when reverting to the BV implementation, it's monomorphic again..

[hydrolarus]

Much better with the BitVector backing! Still some naming discrepancies and small suggesetions, but it's look good!

[hydrolarus]

Nice! It would be cool if there was also a value-level macro mask!(0b...., n = 32), but if you don't want to add that in this PR I think that would be fine too!

[martijnbastiaan]

I'll open another PR :)

[hydrolarus]

Looking good to me! One more tiny naming nit-pick but it's not a blocker to get this merged IMO

[hydrolarus]

Nice!