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

Remove KnowDomain #2589

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from
Draft

Remove KnowDomain #2589

wants to merge 27 commits into from

Conversation

leonschoorl
Copy link
Member

@leonschoorl leonschoorl commented Oct 16, 2023

Removing the need for KnownDomain constraints

This an implementation of the idea from #978, removing (most of the need for) the KnownDomain constraint.
The core change of this PR is to hide the KnownDomain constraint inside the Clock and Reset types.

--- a/clash-prelude/src/Clash/Signal/Internal.hs
+++ b/clash-prelude/src/Clash/Signal/Internal.hs
@@ -931,7 +931,7 @@ enableGen :: Enable dom
 enableGen = toEnable (pure True)

 -- | A clock signal belonging to a domain named /dom/.
-data Clock (dom :: Domain) = Clock
+data Clock (dom :: Domain) = KnownDomain dom => Clock
   { -- | Domain associated with the clock
     clockTag :: SSymbol dom

@@ -1171,7 +1171,8 @@ resetGenN n =
 -- | A reset signal belonging to a domain called /dom/.
 --
 -- The underlying representation of resets is 'Bool'.
-data Reset (dom :: Domain) = Reset (Signal dom Bool)
+data Reset (dom :: Domain) where
+  Reset :: KnownDomain dom => Signal dom Bool -> Reset dom

The allows us to drop the KnownDomain constraint from every function take takes either a Clock or a Reset, which is most of them.

The only things that still require a KnownDomain constraint are functions that create Clocks or Resets:

  • clock and reset generators: noReset, clockGen, resetGen and their variants
  • simulation helpers: simulate, sample and all their variants 1
  • conversion functions creating resets from Bool signals: unsafeFromActive[High,Low], unsafeToReset
  • PLLs (and other hardware clock generators): altpll, alteraPll, clockWizard, clockWizardDifferential

And some tracing functions:

  • traceSignal, traceVecSignal

When using any of these functions that still requires a KnownDom there a couple of ways of handling it:

  1. Don't do anything, just propagate the constraint up to your type signature, so your caller will have to solve it.

  2. Make your code monomorphic.
    By specializing to a specific domain, GHC will automatically solve KnownDomain constraints for you.

  3. Use the new provideKnownDomainFrom

    Click here for its definition
    class HasKnownDomain a where
      provideKnownDomainFrom :: a dom -> (KnownDomain dom => r) -> r
    
    instance HasKnownDomain Clock where ...
    instance HasKnownDomain DiffClock where ...
    instance HasKnownDomain Reset where ...
    -- foo :: Clock dom -> Signal dom Bool -> Reset dom
    -- foo clk bools = provideKnownDomainFrom clk (unsafeFromActiveHigh bools)
    
    invertReset :: Reset dom -> Reset dom
    invertReset r = provideKnownDomainFrom r unsafeToReset . fmap not . unsafeFromReset $ r
    

    This helper function take something containing a KnownDomain contraint as first argument, extracts it and provides it to the second argument.

  4. Use the new ExtractClockDom and ExtractResetDom PatternSynonyms to extract the KnownDomain for you

    Click here for its definition
    pattern ExtractResetDom
    :: ()              -- constraint required to match the pattern
    => KnownDomain dom -- constraint provided by matching the pattern
    => Reset dom
    pattern ExtractResetDom <- Reset {}
    -- and the same for ExtractClockDom
    {-# COMPLETE ExtractResetDom #-}
    invertReset :: Reset dom -> Reset dom
    invertReset r@ExtractResetDom = unsafeToReset . fmap not . unsafeFromReset $ r

    ExtractResetDom is just a synonyms for the pattern Reset{}. But limited in such a way that it is safe to use, and won't generate broken (V)HDL, and therefore is safe to export from Clash.Prelude etc.

  5. Change or make variants of (some of) these that take an additional Clock dom argument.

    I'm think this could be convenient for: noReset, traceSignal, traceVecSignal and maybe unsafeFromActive[High,Low].

Options 0. and 1. are nothing new, they were already possible before this PR.
Options 2. and 3. are new options added in this PR, but I'm unsure if it makes sense to add both, or if that would just clutter up the API unnecessarily.
Also I'm not happy with the current names, they could do with some bike shedding.
Option 4. I haven't implemented, could be convenient, but would be more incompatible API changes, or add more slightly different variants of existing functions.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

Footnotes

  1. Except the Explicit variants of simulate, simulateB, sample, sampleN, sample_lazy and sampleN_lazy.

This changes the error when using non-existant arguments in blackboxes 
from:

  Blackbox required at least 32 arguments, but only 8 were passed.

to the new error:

  Blackbox used "~ISSYNC[31]" , but only 8 arguments were passed.
So it'll be easier to keep the temp directory
leonschoorl added a commit that referenced this pull request Oct 16, 2023
In preparation for #2589 "Remove KnownDomain".
This is the only constraint addition in clash-prelude in for the KnownDomain removal,
by adding it here for 1.8 we can do the KnownDomain removal later,
with less backwards compatibility issues.

This commit also had to add a KnownDomain to invertReset,
because it used unsafeToReset, but this will be removed again by #2589.
It now correctly "escapes" square brackets in the right places.
And correctly brackets the second argument of ~SIGD
  Utility that can convert blackboxes from clash-lib .primitives.yaml files
  to InlineYamlPrimitive ANNotations.
You can now use ~PERIOD, ~ISSYNC, ~ISINITDEFINED and ~ACTIVEEDGE
on arguments of type Clock,Reset,Enable,ClockN and DiffClock.
This allows the removal of the KnownDomain constraint on
functions which take a Clock and/or a Reset/
Which is most of them.

The only functions that still need a KnownDomain constraint
should be the ones creating Clocks and/or Resets.
 * ZKnownDomain is used only blackboxes, it used to replace the KnownDomain there,
   so we can postpone the renumbering of arguments
For the more complex ones I've replaced KnownDomain
with the a dummy ZKnownDomain constraint,
so the argument numbers stay the same for now.
For some functions we add explicit foralls, so the order of the type 
arguments stays the same.
You don't need to provide a KnownDomain anymore
The conversion was done automatically by running:
  cabal run -- prim-yaml-to-inlineyaml
Clash.Explicit.BlockRam.blockRamU# Clash.Explicit.BlockRam.blockRam1#
Clash.Explicit.BlockRam.blockRam#
Clash.Explicit.BlockRam.Blob.blockRamBlob#
Clash.Explicit.BlockRam.File.blockRamFile# Clash.Explicit.RAM.asyncRam#
Clash.Explicit.ROM.rom# Clash.Explicit.ROM.Blob.romBlob#
Clash.Explicit.ROM.File.romFile# Clash.Explicit.Testbench.assert
Clash.Explicit.Testbench.assertBitVector Clash.Signal.Internal.delay#
Clash.Signal.Internal.register# Clash.Signal.Internal.asyncRegister#

Then the annotations were manually moved to be next to their haskell
definitions.
Current doctest(-parallel) can't pickup such defines automatically, so 
we have to do it manually.

And also define CLASH_OPAQUE, to prevent a ton of warnings.
leonschoorl added a commit that referenced this pull request Oct 25, 2023
In preparation for #2589 "Remove KnownDomain".
This is the only constraint addition in clash-prelude in for the KnownDomain removal,
by adding it here for 1.8 we can do the KnownDomain removal later,
with less backwards compatibility issues.

This commit also had to add a KnownDomain to invertReset,
because it used unsafeToReset, but this will be removed again by #2589.
leonschoorl added a commit that referenced this pull request Oct 25, 2023
In preparation for #2589 "Remove KnownDomain".
This is the only constraint addition in clash-prelude in for the KnownDomain removal,
by adding it here for 1.8 we can do the KnownDomain removal later,
with less backwards compatibility issues.

This commit also had to add a KnownDomain to invertReset,
because it used unsafeToReset, but this will be removed again by #2589.
leonschoorl added a commit that referenced this pull request Nov 2, 2023
In preparation for #2589 "Remove KnownDomain".
This is the only constraint addition in clash-prelude in for the KnownDomain removal,
by adding it here for 1.8 we can do the KnownDomain removal later,
with less backwards compatibility issues.

This commit also had to add a KnownDomain to invertReset,
because it used unsafeToReset, but this will be removed again by #2589.
@martijnbastiaan
Copy link
Member

Overall this looks good, although it will take me more time to review it properly. Concerning the bike shedding, here are my thoughts:

  1. Don't do anything. From the looks of it, we have quite a few "normal" functions in clash-prelude that want to extract KnownDomains. "Normal" in the sense that these are functions our users would probably write. Its probably best to dogfood a solution and couple them to the changes. Still, this solution doesn't necessarily worsen anything. (I.e., a proposal still propagating KnownDomain in some places is still a strict improvement.)

  2. Make your code monomorphic Similar reasoning to (0), with the addition that you can't really monomorphize library functions.

  3. Use the new provideKnownDomainFrom. I like this. It is explicit, and our users are already exposed to similar functions. I'd prefer the name withKnownDomain/withKnownDomainFrom to make it fit in with the IP functions.

  4. Use the new ExtractClockDom and ExtractResetDom PatternSynonyms to extract the KnownDomain for you. In general I think pattern synonyms should only be applied whenever the act like a proper constructor (and you never need to access actual constructors). E.g., the way Seq does it. I think this, because for any other use it's yet another Haskell concept for users to grasp, which syntactically looks exactly like (de)construction potentially making it very confusing.

  5. Change or make variants of (some of) these that take an additional Clock dom argument. I've already found it confusing in practice for unsafeSynchronizer, so I'd rather not do this. Plus, like you said, this would hard break the current API. (The latter point weighing the most.)

An alternative take on (3) would be to wrap existing types (Clock/Reset/Enable) and make them carry the KnownDomain constraint. Their constructors could be safely exported. Though this also feels kinda dirty..

My preference would be (2), (0), (3).

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

Successfully merging this pull request may close these issues.

Remove the need for KnownDomain constraints throughout Clash designs
2 participants