Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add maybeResize (and friends) #2780

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+57 −0
Open

Add maybeResize (and friends) #2780

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
388a499
Add maybeResize (and friends)
jvnknvlgl Aug 2, 2024
c5b9bef
Fix incorrect text in Haddock
jvnknvlgl Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions clash-prelude/src/Clash/Class/Resize.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ module Clash.Class.Resize
, checkedResize
, checkedFromIntegral
, checkedTruncateB
, maybeResize
, maybeFromIntegral
, maybeTruncateB
) where

import Data.Kind (Type)
Expand Down Expand Up @@ -117,3 +120,57 @@ checkedTruncateB ::
f (a + b) -> f a
checkedTruncateB v =
checkIntegral (Proxy @(f a)) v `seq` truncateB v

-- | Helper function of 'maybeFromIntegral', 'maybeResize' and 'maybeTruncateB'
maybeIntegral ::
forall a b.
(Integral a, Integral b, Bounded b) =>
Proxy b ->
a -> Bool
maybeIntegral Proxy v =
toInteger v < toInteger (maxBound @b)
&& toInteger v > toInteger (minBound @b)

-- | Like 'fromIntegral', but returns 'Nothing' if /a/ is out of bounds for /b/.
-- Useful when you do not know /a/ can be out of bounds, and would like to have
-- your assumptions checked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Haddock for maybeFromIntegral is not generated, and that's probably because of this blank line. This blank line means that what follows the blank line is a regular Haskell comment rather than a Haddock comment. I would have expected the Haddock to still be associated with the function, but apparently it's cut off and lost?

TL;DR: Remove the blank line :-). And always check the Haddock output :-).

To view the Haddock, you can click on the Details button next to the ci/gitlab/gitlab.com CI test result, and then click on haddock (in the column test), and then click the big Browse button in the rightmost column of that webpage.

Or just generate Haddock on your own system of course, before you run CI.

-- * __NB__: 'fromIntegral' is not well suited for Clash as it will go through
-- 'Integer' which is arbitrarily bounded in HDL. Instead use
-- 'Clash.Class.BitPack.bitCoerce' and the 'Resize' class.
maybeFromIntegral ::
forall a b.
(Integral a, Integral b, Bounded b) =>
a -> Maybe b
maybeFromIntegral v =
if maybeIntegral (Proxy @b) v
then Just (fromIntegral v)
else Nothing

-- | Like 'resize', but returns 'Nothing' if /f a/ is out of bounds for /f b/.
-- Useful when you do not know /f a/ can be out of bounds, and would like to
-- have your assumptions checked.
Comment on lines +151 to +152
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all these maybe variants can be described much shorter:

Like resize, but returns Nothing if f a is out of bounds for f b.

The rest seems unnecessary, and also it mentions assumptions, but we don't go in with assumptions. This function is total; the assumption with the checked variant was that it would fit, but we want to error when that assumption is wrong. This is different.

Also, Martijn and I discussed that we'd best rephrase all these functions a bit, but that can be done in a followup PR. Or I can tack a commit onto this PR...

maybeResize ::
Copy link
Member

@rowanG077 rowanG077 Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break for anything larger than native word size (64-bit usually) in hardware. At least mention this limitation in the doc.

edit: I think it would be possible to rewrite this using only Ord, Bounded, Resize and BitPack. resize to Max (BitSize a) (BitSize b) and compare to maxBound/minBound of b. That won't have this limitation. It will also sidestep the notorious clash Integer warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh in fact you can do a short circuit on the type level if BitSize a >= BitSize b which means this function will be free in that case in hardware.

Copy link
Member

@martijnbastiaan martijnbastiaan Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this shouldn't go through dynamically sized constructs.

Something along the lines of:

case cmpNat @a @b Proxy Proxy of
  LTI | v > resize (maxBound @(f b)) -> Nothing
  EQI -> Just v -- optional, for performance reasons?
  _ -> Just (resize v)

forall a b f.
( Resize f
, KnownNat a, Integral (f a)
, KnownNat b, Integral (f b), Bounded (f b) ) =>
f a -> Maybe (f b)
maybeResize v =
if maybeIntegral (Proxy @(f b)) v
then Just (resize v)
else Nothing

-- | Like 'truncateB', but returns 'Nothing' if /f (a + b)/ is out of bounds for
-- /f a/. Useful when you do not know /f (a + b)/ can be out of bounds, and
-- would like to have your assumptions checked.
maybeTruncateB ::
forall a b f.
( Resize f
, KnownNat b, Integral (f (a + b))
, KnownNat a, Integral (f a), Bounded (f a) ) =>
f (a + b) -> Maybe (f a)
maybeTruncateB v =
if maybeIntegral (Proxy @(f a)) v
then Just (truncateB v)
else Nothing