Skip to content

Commit

Permalink
Reliably handle hidden resets in 'withReset' and siblings
Browse files Browse the repository at this point in the history
Before this commit, 'withReset' would behave very surprisingly in
combination with 'simulate' or 'sample':

    >>> rst0 = fromList [True, True, False, False, True, True]
    >>> rst1 = unsafeFromHighPolarity @System rst0
    >>> sig = withReset rst1 (register 'a' (pure 'b'))
    >>> sampleN @System 6 sig
    "aabbbb"

Even though `rst1` is asserted on the 5th and 6th clock cycle, we never
see the reset value, `a`, after the initial two. But even the first two
`a`s are suspicious. We'd expect to see three `a`s (one powerup, two
reset) instead! It seems that 'sampleN' is generating a default reset
value and using that one. Indeed, when we inspect the type of `sig` we
see it is as we never applied the reset at all:

    >>> :t sig
    (HiddenReset dom, ..) => Signal dom Char

To figure out what's going on, let's inspect the type signature of
'withReset':

    withReset
      :: KnownDomain dom
      => Reset dom
      -> (HiddenReset dom => r)
      -> r

It turns out that GHC is perfectly happy to /only/ resolve `r` when
given the following as its second argument:

    reg :: (HiddenReset dom, ..) => Signal dom Char
    reg = register 'a' (pure 'b')

i.e., `withReset` would actually resolve to something like:

    withReset*
      :: forall dom r
       . KnownDomain dom
      => Reset dom
      -> (HiddenReset dom  => ((HiddenReset dom0, ..) => Signal dom0 Char))
      -> ((HiddenReset dom0, ..) => Signal dom0 Char)

thus completely discarding its given reset, and leaving it up to
`sample` to actually generate the reset. The reason GHC is free to do
this, is that nothing in `r` ties `dom` to the "dom inside `r`" simply
because it's not mentioned in the type signature.

To solve this, this commit introduces a typeclass `HasDomain` that
forces a link between the given `dom` and the "dom inside `r`". The
type signature of 'withReset' changes only slightly:

    withReset
      :: (KnownDomain dom, HasDomain r, GetDomain r ~ dom)
      => Reset dom
      -> (HiddenReset dom => r)
      -> r

With this patch, 'withReset' behaves exactly like we expect it would:

    >>> rst0 = fromList [True, True, False, False, True, True]
    >>> rst1 = unsafeFromHighPolarity rst0
    >>> sig = withReset rst1 (register 'a' (pure 'b'))
    >>> sampleN @System 6 sig
    "aaabaa"

We haven't got a good answer for bundled types yet. _Ideally_
we'd be able to write something like

    instance HasDomain (Unbundled dom a) where
        type GetDomain (Unbundled dom a) = dom

but we can't, as _Unbundled_ in itself is an associated type.

To those wondering why this issue didn't rear its head the moment
we introduced hidden clocks: this issue has only been introduced
a few days ago after merging PR #527 (configurable initial register
values). Before that PR, only a single hidden clock/register could
be used - a fact that GHC allowed to resolve the constraint early.

Co-Authored-By: Leon Schoorl <[email protected]>
  • Loading branch information
martijnbastiaan and leonschoorl committed Jun 28, 2019
1 parent c42931e commit dc53b59
Show file tree
Hide file tree
Showing 4 changed files with 283 additions and 47 deletions.
1 change: 1 addition & 0 deletions clash-prelude/clash-prelude.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ Library

Clash.Class.BitPack
Clash.Class.Exp
Clash.Class.HasDomain
Clash.Class.Num
Clash.Class.Resize

Expand Down
Loading

0 comments on commit dc53b59

Please sign in to comment.