Add TH TopEntityGeneration #729

blaxill · 2019-08-21T00:55:50Z

This is a work in progress to add TH functions to generate Synthesize annotations from Clash's NamedTypes annotations. It's based on an initial implementation by @thoughtpolice here and here. The difference with this version is that it will recursively attempt to find annotations in data types.

The motivation for me is that top entities with a large number of ports require haskell type + synthesize annotation which are verbose and can get out of sync.

Some drawbacks of this in its current state:

~~Messy implementation (If there is interest in this PR I will work to improve it)~~ Improved somewhat
Extra dependency: I've used the recursion-schemes package simply as it was the method I'm most familiar with
~~Testing is brittle: I don't know of a better way to test the generated template haskell (see test file)~~
Incorrect generation when used incorrectly: This is the same as manually written Synthesize annotations, but we have the possibility to do better as we have access to the types.

An example:

data Simple = Simple ("simple1" ::: Int) ("simple2" ::: Bool)
data Named
  = Named
  { name1 :: "named1" ::: BitVector 3
  , name2 :: "named2" ::: BitVector 5
  }
data Embedded
  = Embedded
  { name3 :: "embedded1" ::: Simple
  , name4 :: "embedded2" ::: Bool
  }

topEntity :: "int"      ::: Signal System Int
           -> "tuple"    ::: ("tup1" ::: Signal System (BitVector 7), "tup2" ::: Signal System (BitVector 9))
           -> "simple"   ::: Signal System Simple
           -> "named"    ::: Signal System Named
           -> "embedded" ::: Signal System Embedded
           -> "out"      ::: Signal System Bool
topEntity = undefined
makeTopEntity 'topEntity
--   ======>
--     {-# ANN topEntity ((Synthesize "topEntity")
--                           [PortName "int",
--                            (PortProduct "tuple") [PortName "tup1", PortName "tup2"],
--                            (PortProduct "simple") [PortName "simple1", PortName "simple2"],
--                            (PortProduct "named") [PortName "named1", PortName "named2"],
--                            (PortProduct "embedded")
--                              [(PortProduct "embedded1")
--                                 [PortName "simple1", PortName "simple2"],
--                               PortName "embedded2"]])
--                          (PortName "out") #-}

Let me know what you think/any ideas to make this better

blaxill · 2019-08-21T00:58:41Z

It's likely the tests I've written is also version specific due to hardcoding template haskell splices, but I'm not sure on a better way?

leonschoorl · 2019-08-21T15:21:02Z

If you split out the code that generates the TopEntity into a function that generates that as a TH expression (createTopEntity :: Name -> Maybe String -> ExpQ, or maybe even :: .. -> TExpQ TopEntity)
Then in the test you can splice in the TopEntity as a term and compare it with some expected TopEntity written manually.

The test could be something like:

expectedTopEntity1 =
  Synthesize "topEntity1")
    [PortName "in1", PortName "in2"])
    (PortName "out")

tests :: TestTree
tests =
  testGroup
    "topEntityGen"
    [ testCase "topEntity1" $
      $(createTopEntity 'topEntity1 Nothing) @?= expectedTopEntity1
    , ...
    ]

Much more readable and the test is TH version independent.

blaxill · 2019-08-21T15:22:58Z

Makes sense! I'll get to it in a day or two.

blaxill · 2019-08-26T12:37:29Z

hey @leonschoorl, any other thoughts on this ?

thoughtpolice · 2019-08-27T17:04:59Z

Thanks for this, it's wonderful and much closer to what I wanted to (eventually) build!

Here's a bikeshed: what about the module name? There is some precedence for naming things .TH, e.g. lens puts all TH helpers under one module, and there is already a .TH module for Clash.Promoted.Nat.TH. So maybe we should just call it Clash.Annotations.TH or something, which could contain future APIs as well.

For example, I could see this same basic design being used for an implementation of e.g. #248 if (when) I implement it, so you could easily write blackbox modules and have all the crap auto-generated from the declaration you specify, just like here. In that case, we could also add a makeBlackBox to Clash.Annotations.TH as well.

blaxill · 2019-09-01T17:51:24Z

hey @martijnbastiaan any thoughts on this PR ?

martijnbastiaan · 2019-09-01T19:35:17Z

I like it! As the definition of ::: is

type (name :: k) ::: a = a

I wonder if we could use something like:

data AutoName

and

topEntity :: "int"      ::: Signal System Int
           -> "tuple"   ::: ("tup1" ::: Signal System (BitVector 7), "tup2" ::: Signal System (BitVector 9))
           -> "simple"  ::: Signal System Simple
           -> "named"   ::: Signal System Named
           -> AutoName  ::: Signal System Embedded
           -> "out"     ::: Signal System Bool

to autogenerate field names based on the record's field names. Anyway, that might be something to consider in the future. I'll give it a spin tomorrow and report back.

For the meantime, could you format you code according to the style guide? It's mostly:

buildPorts :: Type                                        -- Type to split at
           -> TypeF (Type, Q [PortName]) -> Q [PortName]  -- Fold step

to

buildPorts
  :: Type                                        
  -- ^ Type to split at
  -> TypeF (Type, Q [PortName]) -> Q [PortName]  
  -- ^ Fold step

Thanks!

blaxill · 2019-09-11T10:49:45Z

@martijnbastiaan I've made some style fixes and rebased. I'm not sure what the failure for 8.8.1 is. Any other changes you can think of for this PR?

martijnbastiaan · 2019-09-11T12:07:38Z

@blaxill Looks good! I'd be happy to include it. I'll look into the GHC 8.8.1 error - I suspect it's another issue with version bounds on dependencies. I've rebased it again and will run it with our own CI. If that works I'll force push to this PR.

christiaanb · 2019-09-11T12:26:58Z

@blaxill what happens if I don't annotate an argument or a result with a name, e.g

topEntity :: "int"      ::: Signal System Int
           -> "tuple"   ::: ("tup1" ::: Signal System (BitVector 7), "tup2" ::: Signal System (BitVector 9))
           -> "simple"  ::: Signal System Simple
           -> "named"   ::: Signal System Named
           -> Signal System Embedded
           -> "out"     ::: Signal System Bool

leonschoorl · 2019-09-11T12:47:43Z

clash-prelude/clash-prelude.cabal

@@ -317,14 +319,16 @@ test-suite unittests
      base,
      hint          >= 0.7      && < 0.10,
      tasty         >= 1.2      && < 1.3,
-      tasty-hunit
+      tasty-hunit ,
+      template-haskell  >= 2.9.0.0 && < 2.14


This should be template-haskell >= 2.9.0.0 && < 2.16 to work with GHC-8.8

Actually you could just remove the bounds here and cabal will use the bounds places on the Library

blaxill · 2019-09-11T12:55:18Z

@christiaanb

topEntity :: "int"      ::: Signal System Int
           -> "tuple"   ::: ("tup1" ::: Signal System (BitVector 7), "tup2" ::: Signal System (BitVector 9))
           -> "simple"  ::: Signal System Simple
           -> "named"   ::: Signal System Named
           -> Signal System Embedded
           -> "out"     ::: Signal System Bool

That will fail with:

tests/Clash/Tests/TopEntityGeneration.hs:90:1: error:
    Failed to get a single name for argument 4
 a.k.a. AppT (AppT (ConT Clash.Signal.Internal.Signal) (ConT Clash.Signal.Internal.System)) (ConT Clash.Tests.TopEntityGeneration.Embedded)
 got name [PortProduct "embedded1" [PortName "simple1",PortName "simple2"],PortName "embedded2"]

Additionally:

topEntity :: "int"      ::: Signal System Int
           -> "tuple"   ::: ("tup1" ::: Signal System (BitVector 7), "tup2" ::: Signal System (BitVector 9))
           -> "simple"  ::: Signal System Simple
           -> "named"   ::: Signal System Named
           -> Signal System Int
           -> "out"     ::: Signal System Bool

Will also fail:

tests/Clash/Tests/TopEntityGeneration.hs:100:1: error:
    Failed to get a single name for argument 4
 a.k.a. AppT (AppT (ConT Clash.Signal.Internal.Signal) (ConT Clash.Signal.Internal.System)) (ConT GHC.Types.Int)
 got name []

Whereas this will work:

data SimpleNamed = SimpleNamed ("simpleNamed" ::: Int)
topEntity :: "int"      ::: Signal System Int
           -> "tuple"   ::: ("tup1" ::: Signal System (BitVector 7), "tup2" ::: Signal System (BitVector 9))
           -> "simple"  ::: Signal System Simple
           -> "named"   ::: Signal System Named
           -> Signal System SimpleNamed
           -> "out"     ::: Signal System Bool
  ======>
    {-# ANN topEntity Synthesize
                        {t_name = "topEntity",
                         t_inputs = [PortName "int",
                                     (PortProduct "tuple") [PortName "tup1", PortName "tup2"],
                                     (PortProduct "simple")
                                       [PortName "simple1", PortName "simple2"],
                                     (PortProduct "named") [PortName "named1", PortName "named2"],
                                     PortName "simpleNamed"],
                         t_output = PortName "out"} #-}

Some other failure cases might exist when using naming annotations on a subset of parameters in constructors. (E.g. note the PortProduct for "Ops" only gets one PortName child instead of two):

data Ops = Ops ("one":::Int) (Bool)

topEntity :: "int"      ::: Signal System Int
           -> "simple"  ::: (Signal System Ops)
           -> "out"     ::: Signal System Bool
makeTopEntity 'topEntity

  ======>
    {-# ANN topEntity Synthesize
                        {t_name = "topEntity",
                         t_inputs = [PortName "int",
                                     (PortProduct "simple") [PortName "one"]],
                         t_output = PortName "out"} #-}

I think it will work correctly if the unnamed child references a type that has annotation though.

I'll add a test case for your example. For my failure case its a bit more tricky and the function buildPorts would need to be extended to handle this.

martijnbastiaan · 2019-09-11T13:34:30Z

I believe

a.k.a. AppT (AppT (ConT Clash.Signal.Internal.Signal) (ConT Clash.Signal.Internal.System)) (ConT Clash.Tests.TopEntityGeneration.Embedded)

would be more readable after a pprint (click).

martijnbastiaan · 2019-09-11T14:51:28Z

Looking good @blaxill! I'll merge it after the CI is done testing.

blaxill · 2019-09-11T14:53:14Z

Looking good @blaxill! I'll merge it after the CI is done testing.

Hold on one second, I've just realised newtypes aren't supported, but it will only take me a second or two to add support and tests...

martijnbastiaan · 2019-09-11T14:56:36Z

Np! CI won't be finished anytime soon I'm afraid..

blaxill · 2019-09-11T18:41:02Z

CI beat me to it :) I'm adding support for basic type/data families, eta tomorrow

Added support for: - newtypes - One step resolution of type/data family instances - all constructor types

blaxill · 2019-09-12T03:15:50Z

Ok, this has support for one-step (non-recursive) type families, extra tests and better error handling. Assuming you don't want any changes from the latest commit this is good to go for me.

martijnbastiaan · 2019-09-12T06:34:03Z

Haddock generation fails with the new patch. If I had to guess it would be:

'clash-prelude/tests/Clash/Tests/TopEntityGeneration.hs'

causing parse errors (as single quotes are usually used to reference to functions). I think replacing it with:

"Clash.Tests.TopEntityGeneration"

would be good enough.

martijnbastiaan · 2019-09-12T07:26:03Z

@blaxill I've pushed it to our local CI too to speed it up. If you could give me a heads up when you think it's ready, that'd be great, then I'll squash & merge it.

blaxill · 2019-09-12T07:35:59Z

Ok, I've added all I can think of. Works locally for me, but I can't promise I've missed another incompatibility with differing base/template haskell versions until CI has finished.

Edit:

Looks like haddock still might be failing.

If you can skip CI on the previous pushes and only do the last that would be preferable. My intermediate commits have bogged down the CI sorry.

martijnbastiaan · 2019-09-12T08:00:24Z

If you can skip CI on the previous pushes and only do the last that would be preferable. My intermediate commits have bogged down the CI sorry.

Done! We should be able to move everything to GitLab CI soon (we've been using it for our own branches/prs for a while now), which should improve the experience considerably.

blaxill · 2019-09-12T10:37:39Z

@martijnbastiaan Looks like its ready to go!

martijnbastiaan · 2019-09-12T11:41:54Z

Great, I've squashed it in #795. Looks like the CI approved there as well. Thanks for your work @blaxill !

blaxill changed the title ~~WIP: Add TH TopEntityGen~~ Add TH TopEntityGeneration Sep 1, 2019

blaxill added 9 commits September 8, 2019 16:26

Initial TopEntityGen

e27da3e

Add no-warn-orphans option to TopEntityGen.hs

8eb3b81

Refactor TopEntityGen.hs

7ee61f7

Add equality instances for TopEntity and PortName

b5357dc

Proper TopEntityGen tests

e8ddcdb

Add some doc to TopEntityGen

427a670

TopEntityGen export comments

50cf753

Better module names

f6e15e7

Some style fixes

365541f

blaxill force-pushed the topentitygen branch from 6c96ee9 to 365541f Compare September 8, 2019 09:29

leonschoorl reviewed Sep 11, 2019

View reviewed changes

Remove template-haskell bounds

cf5d657

blaxill added 2 commits September 11, 2019 21:26

Use pprint in failure message

3133909

Add failure cases

1d624b1

Broaden type support and tests

903dd5e

Added support for: - newtypes - One step resolution of type/data family instances - all constructor types

blaxill added 4 commits September 12, 2019 14:04

Doc fix

dbeeea5

Semigroup fix

d26d3d1

Template haskell fix for 2.15+

bd11b87

Pushed too soon

48b19ab

Work with concrete types with outer constraint

5eb4d11

More haddock fixes

b1df7b9

martijnbastiaan pushed a commit that referenced this pull request Sep 12, 2019

Add tools to generate top entity annotations (#729)

d1044e4

martijnbastiaan mentioned this pull request Sep 12, 2019

Add tools to generate top entity annotations (#729) #795

Merged

martijnbastiaan closed this Sep 12, 2019

martijnbastiaan pushed a commit that referenced this pull request Sep 12, 2019

Add tools to generate top entity annotations (#729)

0afb65c

leonschoorl mentioned this pull request Nov 14, 2019

PortProduct prefixing, auto-generating notation #914

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TH TopEntityGeneration #729

Add TH TopEntityGeneration #729

blaxill commented Aug 21, 2019 •

edited

Loading

blaxill commented Aug 21, 2019

leonschoorl commented Aug 21, 2019

blaxill commented Aug 21, 2019

blaxill commented Aug 26, 2019

thoughtpolice commented Aug 27, 2019

blaxill commented Sep 1, 2019

martijnbastiaan commented Sep 1, 2019

blaxill commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

christiaanb commented Sep 11, 2019

leonschoorl Sep 11, 2019

leonschoorl Sep 11, 2019

blaxill commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

blaxill commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

blaxill commented Sep 11, 2019

blaxill commented Sep 12, 2019

martijnbastiaan commented Sep 12, 2019

martijnbastiaan commented Sep 12, 2019

blaxill commented Sep 12, 2019 •

edited

Loading

martijnbastiaan commented Sep 12, 2019

blaxill commented Sep 12, 2019

martijnbastiaan commented Sep 12, 2019

Add TH TopEntityGeneration #729

Add TH TopEntityGeneration #729

Conversation

blaxill commented Aug 21, 2019 • edited Loading

blaxill commented Aug 21, 2019

leonschoorl commented Aug 21, 2019

blaxill commented Aug 21, 2019

blaxill commented Aug 26, 2019

thoughtpolice commented Aug 27, 2019

blaxill commented Sep 1, 2019

martijnbastiaan commented Sep 1, 2019

blaxill commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

christiaanb commented Sep 11, 2019

leonschoorl Sep 11, 2019

Choose a reason for hiding this comment

leonschoorl Sep 11, 2019

Choose a reason for hiding this comment

blaxill commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

blaxill commented Sep 11, 2019

martijnbastiaan commented Sep 11, 2019

blaxill commented Sep 11, 2019

blaxill commented Sep 12, 2019

martijnbastiaan commented Sep 12, 2019

martijnbastiaan commented Sep 12, 2019

blaxill commented Sep 12, 2019 • edited Loading

martijnbastiaan commented Sep 12, 2019

blaxill commented Sep 12, 2019

martijnbastiaan commented Sep 12, 2019

blaxill commented Aug 21, 2019 •

edited

Loading

blaxill commented Sep 12, 2019 •

edited

Loading