Set names for ILAs and VIOs by default #2655
Comments
|
I think that in the common case of only a single ILA or VIO, there is no need for explicit names. Vivado's I think we should improve our documentation of the naming process rather than change the API. |
|
That is a general problem with naming. If instances are unique, then names turn obsolete, but as soon as multiple of them are around, having some way to properly name them is the best way to go. I still think that an API change is the better way to go here, because it turns Clash magic into some controllable and well-defined argument. As ILAs and VIOs are blackboxes anyway, it is not much effort to handle the names as part of the blackbox, in the same fashion as we do already for naming the ports. If names are not needed, just offer the user to pass the empty string, which the blackbox then maps to the auto-generated name by default. |
|
There are many cases where you might want to give something a name to be able to find it again in the HDL and stages beyond the HDL. That is why we have |
|
You could argue that I'm going off-topic with this post, since most of it is not about ILA's or VIO's. However, you are suggesting we stop using I think the difference between f = setName @"name" $ g . hand f = setName @"name" g . his that the former sets the name of both component To get back to ILAs, if I try the following, which seems to be a reasonable method to write one's ILAs: module TestIlaNames where
import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.Ila
topEntity ::
Clock System ->
Signal System (Unsigned 8) ->
Signal System (Unsigned 8) ->
Signal System ()
topEntity clk in1 in2 = ila1 clk in1 `hwSeqX` ila2 clk in2 `hwSeqX` pure ()
{-# OPAQUE topEntity #-}
ila1 ::
Clock System ->
Signal System (Unsigned 8) ->
Signal System ()
ila1 clk inp = setName @"foo" ila (ilaConfig $ "a" :> Nil) clk inp
ila2 ::
Clock System ->
Signal System (Unsigned 8) ->
Signal System ()
ila2 clk inp = setName @"bar" $ ila (ilaConfig $ "b" :> Nil) clk inpboth ILAs end up with the plain names I agree that the user experience for module TestIlaNames where
import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.Ila
topEntity ::
Clock System ->
Signal System (Unsigned 8) ->
Signal System ()
topEntity clk inp = setName @"foo" $ ila ilaCfg clk $ delay clk enableGen 0 inp
where
ilaCfg = ilaConfig $ "a" :> Nil
{-# OPAQUE topEntity #-}More broadly considering I did notice another unexpected thing, which corresponds to what Hidde reported on the ILAs in the Bittide project. Once Clash begins disambiguating names, it prefixes the names with prefixes based on the name of the entity. But if the name is already unique, it does not prefix the name. With the following code: module SetName where
import Clash.Explicit.Prelude
topEntity ::
Clock System ->
Enable System ->
Signal System Int ->
Signal System Int
topEntity clk en = f clk en . g clk en
{-# OPAQUE topEntity #-}
f ::
Clock System ->
Enable System ->
Signal System Int ->
Signal System Int
f clk en = setName @"foo" delay clk en 0 . delay clk en 0
{-# OPAQUE f #-}
g ::
Clock System ->
Enable System ->
Signal System Int ->
Signal System Int
g clk en = setName @"bar" $ delay clk en 0 . delay clk en 0
{-# OPAQUE g #-}The flip-flops in
And the flip-flops in
I think we should take a look whether we want that! If you're wondering why Clash came up with |
|
Ah perhaps this is what irks you? I managed to at least foul up the names of the ILAs a bit. Normalisation introduced module Test where
import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.Ila
topEntity :: Clock System -> Signal System ()
topEntity clk =
setName @"foo" ilaInst (ilaConfig ("a" :> Nil))
`hwSeqX`
setName @"bar" ilaInst (ilaConfig ("b" :> Nil))
`hwSeqX`
pure ()
where
ilaInst :: IlaConfig 1 -> Signal System ()
ilaInst cfg = ila cfg clk (pure 0 :: Signal System (Unsigned 1))gives the names The normalisation of the core introduced It is rather misleading that it has a duplicate |
|
Right now, I'm thinking that once a user uses And I think that instead of data CtxName =
NoCtxName
| DefaultCtxName IdentifierText
| ExplicitCtxName IdentifierText
bbCtxName :: CtxNameand then this code getIlaName :: Maybe T.Text -> T.Text
getIlaName Nothing = "ila_inst"
getIlaName (Just "result") = getIlaName Nothing
getIlaName (Just "__VOID_TDECL_NOOP__") = getIlaName Nothing
getIlaName (Just s) = scan become getIlaName :: CtxName -> T.Text
getIlaName (ExplicitCtxName name) = name
getIlaName _ = "ila_inst"In fact, that should be a library function. And it shouldn't be called |
|
Normalization adds That’s why the naming mechanism does not ignore these |
|
Yes, that is a good thing, but maybe not for [edit] |
|
The current behavior is explicitly documented. Perhaps we want a |
|
Maybe if the user comes at the point where they cannot tell apart their Also, explicitly documented is not so explicit? It says
It says anything you annotate with I think a solution with |
|
You can also add a NOINLINE to the functions where you use |
|
I also think it is confusing that sometimes the prefix is not there, i.e., when Clash didn't have to inline any definitions. Compare topEntity :: Clock System -> Signal System Bit -> Signal System ()
topEntity clk inp =
setName @"foo" ilaInst (ilaConfig ("a" :> Nil))
`hwSeqX`
setName @"bar" ilaInst (ilaConfig ("b" :> Nil))
`hwSeqX`
pure ()
where
ilaInst :: IlaConfig 1 -> Signal System ()
ilaInst cfg = ila cfg clk inpwhich names the ILAs topEntity :: Clock System -> Signal System Bit -> Signal System ()
topEntity clk inp = ila1 `hwSeqX` ila2 `hwSeqX` pure ()
where
ila1 :: Signal System ()
ila1 = setName @"foo" ila (ilaConfig ("a" :> Nil)) clk inp
ila2 :: Signal System ()
ila2 = setName @"bar" ila (ilaConfig ("b" :> Nil)) clk inpwhich names the ILAs [edit]
correctly now. I think that, when properly documented, might be the correct solution (I still like my |
|
The reason I consider module Test where
import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.Ila
topEntity :: Vec 3 (Clock System) -> Signal System ()
topEntity clks =
hwSeqX (zipWith ilaInst cfgs clks)
$ pure ()
where
cfgs = map (ilaConfig . singleton) ("a" :> "b" :> "c" :> Nil)
ilaInst :: IlaConfig 1 -> Clock System -> Signal System ()
ilaInst cfg clk = setName @"ila" ila cfg clk (pure 0 :: Signal System (Unsigned 1))which produces a ...
topEntity2_1 ila (.c$pTS0 (c$clks_1));
topEntity2_0 ila_0 (.c$pTS0 (c$clks_0));
topEntity2 ila_1 (.c$pTS0 (c$clks));
...
|
|
Agree Felix. What about a |
|
Can you elaborate a bit more on the interface/usage of |
|
Wow, yeah, that use case is nasty. I generated HDL for that code, and in my case, the first ila was called |
The name of an ILA or VIO can be set using Clash's
setName, which turns out to be useful if data needs to be passed or read from these components in an automated setup. However, the user has to be careful in correctly usingsetName, as for exampleand
sets the name of two different components, the latter being the desired use case for most applications.
Hence, to make the user experience less fragile to this, we shall consider adding a required type level name argument to the ILA and VIO interfaces by default.
The text was updated successfully, but these errors were encountered: