Upstreaming clash-port-name?

[bgamari]

One of the facets of Clash that I have long struggled with is the treatment of naming top-level ports. Ensuring that the port-naming of a top-level entity remains consistent with the entity's actual type is annoying at best and a horrible source of bugs at worst. For this reason, I quickly find myself yearning for better tools for treating the representations of top-level entities' ports as composable, first-class objects.

To this end I have written clash-port-name (which, in hindsight, would probably be better called clash-port-rep). While the newly-added README and Haddocks should hopefully start to explain the "why" and "how", what I would like to discuss here is the future of this package. While I am happy to continue maintaining this as an external package, I suspect that the the problem that it solves is universal enough to potentially warrant eventual upstreaming of it or something like it. I am opening this ticket as a forum to have this conversation.

Things I would do differently

clash-port-name is just one point in the design space. There are various things that I think could be improved in it:

• the "mode" notion of Clash.PortRep.TH.genHasPortRep could likely be cleaner
• the library includes a generic deriving mechanism via generics-sop. Sadly, I have found that in practice this mechanism tends to be a very good trigger of clash compiler bugs and consequently I generally don't rely on it. This is sad because in general I think it is much more composable than the TH-based deriving mechanism.
• while I think having both an explicit dictionary type (Port) and the typeclass is helpful, use of the typeclass is a bit less convenient as a result. Adding a few convenient class-centric accessors (e.g. toPortName :: forall a. HasPortRep a => a -> PortRep a) may be a good idea.
• it can still be awkward to modify the behavior of the derived instances as Port is both (a) an isomorphism between a Haskell type and a generic product, (b) an isomorphism between each of the factors and its external bit encoding, and (c) a naming for the factors. Ideally, you wouldn't need to rewrite (a) in order to affect (b) and (c). The generics-sop deriving mechanism is IMHO much better than TH in this regard.

[DigitalBrains1]

I like the elegant and principled approach here! But I do have one main criticism.

But first, some simple observations. You write

module Lib where

foozle = ...

(foozleTop, foozleAnn) = mkEntity "foozle" foozle

-- N.B. Sadly, we must place the top-level entity and its annotation in a separate
-- module due to GHC's staging restriction.
module Top where
import Lib

{-# ANN top foozleAnn #-}
top = foozleTop

The staging restriction is very annoying, but I do think the usability can be increased a bunch by defining mkEntity to be a TH DecsQ used as such:

module Lib where

foozle = ...

-- N.B. Sadly, we must place the top-level entity and its annotation in a separate
-- module due to GHC's staging restriction.
module Top where
import Lib

mkEntity "top" "foozle" 'foozle

with the following:

module Clash.PortRep.Class.Def where

mkEntity0 :: HasEntityPorts a => String -> a -> (EntityPortRep a, TopEntity)
mkEntity0 name f = ... what mkEntity currently is ...

clashOpaquePragma ::
  Quote m =>
  Name ->
  m Dec
#if MIN_VERSION_template_haskell(2,19,0)
clashOpaquePragma = pure . PragmaD . OpaqueP
#else
clashOpaquePragma name = pragInlD name NoInline FunLike AllPhases
#endif

clashOpaque ::
  Quote m =>
  Name ->
  m [Dec]
clashOpaque = fmap pure . clashOpaquePragma

mkEntity ::
  String ->
  String ->
  Name ->
  DecsQ
mkEntity topFunName0 topName entity =
     funDef
  <> funAnn
  <> clashOpaque topFunName1
 where
  funDef = [d| $(varP topFunName1) = fst (mkEntity0 $topNameE $entityE) |]

  funAnn = fmap pure $
    pragAnnD (valueAnnotation topFunName1)
             [| snd (mkEntity0 $topNameE $entityE) |]

  topFunName1 = mkName topFunName0
  topNameE = litE $ stringL topName
  entityE = varE entity

The functions clashOpaque{Pragma,} should go somewhere else of course, but I need it here :-)

We should annotate everything that Clash might need to look up the name of (primitives, top entities, perhaps more) with OPAQUE, or lacking OPAQUE, at least NOINLINE.

And then hopefully you can also plug in a type declaration for the top function through Template Haskell, because it is not very pretty to have to do OPTIONS_GHC -Wno-missing-signatures on the module with the top entity now.

Now regarding the naming part of clash-port-name, I completely agree that the Synthesize annotation really falls short of a proper solution: matching up the names to the ports is just too brittle and error-prone. But we already have Clash.NamedTypes in combination with makeTopEntity. However, makeTopEntity has many issues that definitely make it such that it is not ready for prime time. But the proper solution in my eyes is to move the functionality into the compiler.

But clash-port-name has two components: naming, and automatic conversion. Unfortunately, I see a problem with the automatic, implicit part of that.

Right now, a top entity will often contain some conversion code to massage the data representations as they are used internally in the circuit into a representation that is suitable for the outside. And often, when you connect stuff to the FPGA pins, you want the first thing connected to the I/O buffer of the pin to be a register. If you're writing ad-hoc conversions, you'll just see where that register end up, and you can trivially put it on the "outside".

But your toPortRep and fromPortRep functions will often introduce LUTs. This is very obvious with the sum-of-products default implementation that makes the representation wide to use the terminology of Clash.Annotations.BitRepresentation.Deriving.FieldsType (the fully qualified name of the Wide constructor being an autological name ;-). But it can happen more insidiously. The encoding of the Maybe type is actually well-defined, though probably underdocumented (the default for any Clash type has ConstructorType Binary and FieldsType OverlapL). Now if you use your maybePort True, this will not add any LUTs as the encoding is merely reshuffling some wires. But with maybePort False, your mkEntity will add a LUT onto the port of the top entity that the user wrote. If the user diligently puts a register directly connected to the port of their top entity, and then use mkEntity to generate the final top entity, nothing is informing them that they just inserted a LUT in their data path.

Therefore, I think it would be better if you would either make the port conversion explicit, or find some other way where the user will not easily make this mistake.