search

clash-lang / clash-compiler

Haskell to VHDL/Verilog/SystemVerilog compiler
https://clash-lang.org/
Other
1.42k stars 150 forks source link

Expose `Clash.Class.Counter` methods #2534

Closed gergoerdi closed 5 months ago

gergoerdi commented 1 year ago

Currently, functions like countMin and countSuccOverflow are marked in the documentation as internal implementation details that shouldn't be used by client code. But they (or something similar) would be very useful in a lot of cases where we only want to count "once" and then do something else.

I realize overflow detection can be achieved "on the outside" by adding one more bit to the counter, without access to these functions by doing something like:

let ((bitCoerce @(Index 2) -> overflow), x') = countSucc (0, x)
in x' <$ guard (not overflow)

but this is not really readable (it could be improved a bit if we had a Counter Bool instance). And I'm not sure what similar trick could apply to countMin.

gergoerdi commented 1 year ago

I think the API I would prefer is one that consists of countMin, and countSuccChecked :: (Counter a) => a -> Maybe a. Then both countSucc and countSuccOverflow are trivially recoverable:

countSucc = fromMaybe countMin . countSuccChecked
countSuccOverflow = maybe (True, countMin) (False,) . countSuccChecked
martijnbastiaan commented 1 year ago

The nice thing about (Bool, a) is that you don't need a branch to determine the overflowed number. While this is a bit of a micro-optimization, I do feel it's the right thing to do.

How about simply moving the internal functions public? I don't think we discovered any downsides to the private API so far, so as far as I'm concerned it's fine as it is.

gergoerdi commented 10 months ago

Are there any plans to implement this?

martijnbastiaan commented 10 months ago

Would simply exposing the internal functions be okay? In that case, it's easily done.

leonschoorl commented 10 months ago

We could also add an instance for Counter a => Maybe a And then you can get your interface with:

countSuccChecked :: Counter a => a -> Maybe a
countSuccChecked = countSucc . Just
leonschoorl commented 10 months ago

Also I think we should probably add instances for Bool and Bit

martijnbastiaan commented 6 months ago

@gergoerdi I'd be happy to move this forward; could you comment on my replies?

gergoerdi commented 6 months ago

I'm happy with the suggested ways of doing thidls, because the Maybe instance would give me what I need.

martijnbastiaan commented 6 months ago

See https://github.com/clash-lang/clash-compiler/pull/2692