Add `Clash.Class.Convert` #2915

martijnbastiaan · 2025-03-30T19:04:22Z

Utilities for safely converting between various Clash number types.

To discuss / investigate (but maybe not for this PR?)

The instances add 1 <= n constraints to every result where the result is Index n
- This makes sense from a perspective of: Index 0 has no inhabitants, so we cannot create it. On the other hand, we can of course just call errorX "foo". I'm not sure whether I would say this violates the "guaranteed to succeed" rule.
The instances add 1 <= n constraints to every argument where the argument is Index n
- This is similar to the first point, but differs in that we shove the responsibility for creating these values to the caller.
convert and maybeConvert will always return XException if given an XException.
- Does this make sense for converting BitVector 0 to Index/Unsigned/Signed? The HDL will happily do it, why wouldn't Clash simulation? Similarly, any BitVector n could be repacked to be partially undefined if converted to another BitVector m where m > n.

I'd push for kicking these questions to after this PR, because:

We can always relax constraints (but not introduce them)
We can always relax XException behavior (but not introduce it).
Having a safe way of converting between Clash types is way more important than dropping a minor constraint.
It's a fairly delicate thing I don't think we have a proper/consistent view on yet across clash-prelude.

Still TODO:

Write a changelog entry (see changelog/README.md)
Check copyright notices are up to date in edited files
Decide whether to call it Clash.Class.Convert or Clash.Class.Convertible. The latter has some buy-in (?) from the community, given that covertible already exists. On the other hand, this might make users think it is convertible, but it isn't.

DigitalBrains1 · 2025-03-30T20:26:48Z

I'd vote against naming it Convertible, since I think GHC will by default not qualify the class name in an error message of the form No instance for ‘Convertible A B’, which might be very misleading.

_{(I really want to make the silly pun No instance for ‘Convertible Coupé Roadster’ but it seems confusing... X-D)}

Utilities for safely converting between various Clash number types.

martijnbastiaan · 2025-04-02T15:42:48Z

clash-prelude/src/Clash/Class/Convert.hs

+@clash-convertible@ is similar to the @convertible@ package in that it aims to
+facilitate conversions between different number types. It has two key differences:


To change because it isn't a separate package

martijnbastiaan · 2025-04-02T15:44:12Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+{- | Conversions that are, based on their types, guaranteed to succeed.
+
+== __Laws__
+A conversion is safe and total if a round trip conversion is guaranteed to be


"safe and total" is not terminology introduced earlier

martijnbastiaan · 2025-04-02T15:46:15Z

clash-prelude/src/Clash/Class/Convert/Internal/MaybeConvert.hs

+{- | Conversions that may fail for some values.
+
+== __Laws__
+A conversion is safe if a round trip conversion does not produce errors (also


I don't like the word "safe" here.

kleinreact

LGTM overall.

I'm going to accept this, because I cannot see anything really controversial in the sense that it would be hard to fix it later on.

Regarding the Convert vs Convertible question: I don't like either name, as they both suggest much freedom in terms of which types could be converted with the classes. Your documentation clearly restricts the setting to number types, which is in contrast to Convertible for example, which is also open for conversion between custom non-number types. So maybe we should keep the users of that package happy by not producing any name clash at this point.

A name like changeNumType would make it more clear to me what the purpose of this function is. Convert, Convertible, and convert are just super generic names!

I still find it strange to consider BitVector a number type. If we are going to have a discussion about this at some point, then I'd suggest to keep it out here for the time being. Adding it later is easy and the user still can define the instance locally, if necessary.

I don't like the fact that we need to have two classes in the end, but I get the point that the need for individual selection of constraints per instance introduces that requirement.

kleinreact · 2025-04-07T18:40:22Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+A conversion is safe and total if a round trip conversion is guaranteed to be
+lossless. I.e.,
+
+> Just x == maybeConvert (convert @a @b x)


Is there a particular reason for mixing maybeConvert and convert in a single law? Wouldn't it make more sense to have a separate law per class, as a type not necessarily needs to be an instance of both?

kleinreact · 2025-04-07T18:43:21Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+
+> L.any isNothing (L.map (maybeConvert @a @b) [minBound ..])
+
+Additionally, any implementation should be translatable to synthesizable RTL.


You mention synthesizable HDL in Clash.Class.Convert. I'd suggest to keep the wording consistent.

kleinreact · 2025-04-07T18:45:36Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+-}
+class Convert a b where
+  {- | Convert a supplied value of type @a@ to a value of type @b@. The conversion
+  is guaranteed to succeed.


The classical wording in legal specification documents usually is "must be guaranteed to succeed", as it is a user requirement, in case you like to stick with that.

kleinreact · 2025-04-07T19:03:06Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+instance (Convert (Unsigned 8) a) => Convert Word8 a where
+  convert = convert . bitCoerce @_ @(Unsigned 8)
+
+instance (Convert (Signed 64) a) => Convert Int a where


I would either not add any instances for Word and Int, as their sizes are machine dependent, or the source of the respective conversions should be restricted to at most 32 bit, as this is what base fixes independently from the underlying machine.

kleinreact · 2025-04-07T19:13:02Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

+instance (KnownNat n, KnownNat m, n <= m) => Convert (BitVector n) (BitVector m) where
+  convert = resize
+
+instance (Convert (Unsigned 64) a) => Convert Word a where


The RHS parenthesis are redundant here.

Just noting this, as hlint won't complain about that, in case you prefer them being removed.

kleinreact · 2025-04-07T19:17:20Z

clash-prelude/src/Clash/Class/Convert/Internal/Convert.hs

As you have instances covering BitVector, it might be also desirable to have instances converting from Bit (Bool). Something like a Bit to Word8 conversion sounds reasonable to me.

On the other hand, we should ask ourselves whether BitVector should be considered a number type? I'm still struggling with conceptual separation here, as if BitVector is a number type, what distinguishes it from Unsigned then in the end?

kleinreact · 2025-04-07T19:33:48Z

clash-prelude/src/Clash/Class/Convert/Internal/MaybeConvert.hs

+  For the time being, if the input is an @XException@, then the output is too.
+  This property might be relaxed in the future.
+  -}
+  maybeConvert :: a -> Maybe b


I personally prefer the convention to call this function convertMaybe (similar to readMaybe in base), as the name then denotes the order of operation: "first convert and then put the potential result into the Maybe container". maybeConvert suggests to me that the function converts from a Maybe as the input.

Another advantage of convertMaybe is that TAB completion in the REPL will display both: convert and convertMaybe, as soon as you started typing c o n ... .

martijnbastiaan · 2025-04-10T08:18:02Z

Thanks for the comments @kleinreact, I'll get to them, though I'm fairly time constrained at the moment :(.

I think your comment about the name convert makes a lot of sense. Maybe convertNum? You're right that the class really constrains it to just number types. (Maybe that is an issue?)

DigitalBrains1 · 2025-04-10T10:34:35Z

Somehow I feel NumConvert feels more natural than ConvertNum for the class name... But not so for the function name.

Also, we could rewrite the laws such that the class can also be used for non-numbers. I think that would work?

This comment was marked as resolved.

Sign in to view

DigitalBrains1 self-requested a review March 30, 2025 19:19

This comment was marked as resolved.

Sign in to view

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch from 6d3840f to d8f851c Compare March 30, 2025 21:21

Add Clash.Class.Convert

25c7757

Utilities for safely converting between various Clash number types.

martijnbastiaan force-pushed the martijn/add-clash-class-convert branch from d8f851c to 25c7757 Compare April 1, 2025 20:43

martijnbastiaan requested a review from kleinreact April 2, 2025 07:10

martijnbastiaan added the needs-review label Apr 2, 2025

martijnbastiaan commented Apr 2, 2025

View reviewed changes

kleinreact approved these changes Apr 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Clash.Class.Convert` #2915

Add `Clash.Class.Convert` #2915

martijnbastiaan commented Mar 30, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

DigitalBrains1 commented Mar 30, 2025 •

edited

Loading

martijnbastiaan Apr 2, 2025

martijnbastiaan Apr 2, 2025

martijnbastiaan Apr 2, 2025

kleinreact left a comment

kleinreact Apr 7, 2025

kleinreact Apr 7, 2025

kleinreact Apr 7, 2025

kleinreact Apr 7, 2025

kleinreact Apr 7, 2025

kleinreact Apr 7, 2025

kleinreact Apr 7, 2025

martijnbastiaan commented Apr 10, 2025

DigitalBrains1 commented Apr 10, 2025

		@clash-convertible@ is similar to the @convertible@ package in that it aims to
		facilitate conversions between different number types. It has two key differences:


		> L.any isNothing (L.map (maybeConvert @a @b) [minBound ..])

		Additionally, any implementation should be translatable to synthesizable RTL.

Add Clash.Class.Convert #2915

Are you sure you want to change the base?

Add Clash.Class.Convert #2915

Conversation

martijnbastiaan commented Mar 30, 2025 • edited Loading

To discuss / investigate (but maybe not for this PR?)

Still TODO:

This comment was marked as resolved.

This comment was marked as resolved.

DigitalBrains1 commented Mar 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleinreact left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnbastiaan commented Apr 10, 2025

DigitalBrains1 commented Apr 10, 2025

Add `Clash.Class.Convert` #2915

Add `Clash.Class.Convert` #2915

martijnbastiaan commented Mar 30, 2025 •

edited

Loading

DigitalBrains1 commented Mar 30, 2025 •

edited

Loading