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.

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

Clash.Sized.Fixed: fromInteger and fromRational don't saturate correctly #374

Closed
leonschoorl opened this issue Oct 9, 2018 · 2 comments · Fixed by #1015
Closed

Clash.Sized.Fixed: fromInteger and fromRational don't saturate correctly #374

leonschoorl opened this issue Oct 9, 2018 · 2 comments · Fixed by #1015
Assignees
Milestone

Comments

@leonschoorl
Copy link
Member

While looking at Ben's on fix for signum Fixed: c2d064f
I noticed that even with that fix:

Clash.Prelude> signum (42 :: SFixed 4 2) == signum (42.0 :: SFixed 4 2)
False

It's because fromInteger(used for the literal 42) and fromRational(used for the literal 42.0) produce very different results when the input is out of range:

Clash.Prelude> 42.0 :: SFixed 4 2
7.75
Clash.Prelude> 42 :: SFixed 4 2
-6.0

But our documentation states that Fixed saturates, so both of these should produces 3 times the same tuple:

Clash.Prelude> [(minBound,maxBound), (-42.0, 42.0), (-42, 42)] :: [(SFixed 4 2,SFixed 4 2)]
[(-8.0,7.75),(-7.75,7.75),(6.0,-6.0)]
Clash.Prelude> [(minBound,maxBound), (-42.0, 42.0), (-42, 42)] :: [(UFixed 4 2,UFixed 4 2)]
[(0.0,15.75),(0.25,15.75),(6.0,10.0)]
@martijnbastiaan
Copy link
Member

So I guess this is because Signed and Unsigned don't actually saturate? That is:

Clash.Prelude> 100 :: Signed 2
0

and:

  fromInteger i    = let fSH = fromInteger (natVal (Proxy @frac))
                         res = Fixed (fromInteger i `shiftL` fSH)
                     in res

@martijnbastiaan martijnbastiaan self-assigned this Oct 10, 2018
@martijnbastiaan martijnbastiaan changed the title Fixed: fromInteger and fromRational don't saturate (correctly) Clash.Sized.Fixed: fromInteger and fromRational don't saturate correctly Oct 11, 2018
@martijnbastiaan
Copy link
Member

fromInteger does not know anything about its underlying representation, so unless we convert res back to an integer and compare it to the original input I'm not sure how we can fix this.

@christiaanb christiaanb added this to the 1.1 milestone Dec 18, 2018
christiaanb added a commit that referenced this issue Jan 16, 2020
Needs `-XNegativeLiterals` to fully saturate to `minBound`,
because without that extension `-42` is desugared to
`negate (fromInteger 42)`, and `-42.0` is desugared to
`negate (fromRational (42 % 1))`. Which means it's first
saturated to the upper bound and then negated.

Fixes #374
christiaanb added a commit that referenced this issue Jan 17, 2020
Needs `-XNegativeLiterals` to fully saturate to `minBound`,
because without that extension `-42` is desugared to
`negate (fromInteger 42)`, and `-42.0` is desugared to
`negate (fromRational (42 % 1))`. Which means it's first
saturated to the upper bound and then negated.

Fixes #374
christiaanb added a commit that referenced this issue Jan 17, 2020
Needs `-XNegativeLiterals` to fully saturate to `minBound`,
because without that extension `-42` is desugared to
`negate (fromInteger 42)`, and `-42.0` is desugared to
`negate (fromRational (42 % 1))`. Which means it's first
saturated to the upper bound and then negated.

Fixes #374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants