Open bgamari opened 5 months ago
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:
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.
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 calledclash-port-rep
). While the newly-addedREADME
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:Clash.PortRep.TH.genHasPortRep
could likely be cleanergenerics-sop
. Sadly, I have found that in practice this mechanism tends to be a very good trigger ofclash
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.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.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). Thegenerics-sop
deriving mechanism is IMHO much better than TH in this regard.