`makeTopEntity` doesn't handle nested product types correctly

[martijnbastiaan]

Given:

{-# OPTIONS_GHC -ddump-splices #-}

module Issue where

import Clash.Prelude
import Clash.Annotations.TH

topEntity :: ("a" ::: Bool,  ("b" ::: Bool, "c" ::: Bool)) -> "result" ::: Bool
topEntity = undefined
makeTopEntity 'topEntity

makeTopEntity produces:

    {-# ANN topEntity Synthesize
                        {t_name = "topEntity",
                         t_inputs = [(PortProduct [])
                                       [PortName "a", PortName "b", PortName "c"]],
                         t_output = PortName "result"} #-}

Where I would have expected it to generate:

    {-# ANN topEntity Synthesize
                        {t_name = "topEntity",
                         t_inputs = [(PortProduct [])
                                       [PortName "a",
                                        (PortProduct []) [PortName "b", PortName "c"]]],
                         t_output = PortName "result"} #-}

This can be worked around by supplying an empty name as such:

topEntity :: ("a" ::: Bool, "" ::: ("b" ::: Bool, "c" ::: Bool)) -> "result" ::: Bool

I'm marking this as a bug however, as I'm fairly certain that this wasn't the intent of the original author seeing that a non-nested tuple is handled correctly. For example:

topEntity :: ("a" ::: Bool, "b" ::: Bool) -> "result" ::: Bool

yields:

    {-# ANN topEntity Synthesize
                        {t_name = "topEntity",
                         t_inputs = [(PortProduct []) [PortName "a", PortName "b"]],
                         t_output = PortName "result"} #-}

@blaxill Could you comment on your intent?

[blaxill]

I haven't used Clash in a while, but the first criteria I'd check is whether Clash accepts/generates correct HDL with the flattened version. If the generated HDL is incorrect (names get mixed up) then its 100% a bug.

FWIW, I think this might occur with all nested unannotated types, as the PortProduct looks like it only gets inserted when an annotation is encountered: https://github.com/clash-lang/clash-compiler/blob/f2d3a23a90421e77a31ba07950171db6434d9214/clash-prelude/src/Clash/Annotations/TH.hs#L261-L270

It might be preferable to insert a PortProduct at every type constructor, but if my memory is correct, this is not easy with template Haskell as you only have the syntax tree:

• The tree has arbitrary association so its not as simple as finding the left most item in a series of AppT
• The tree can contain the usual arbitrary combination of type functions/applications etc

It might be productive to rewrite this as a compiler plugin if that gives access to the evaluated types (as long as you can still access the annotations), or some other approach, but I'm afraid I don't have time to take on development of that.

[martijnbastiaan]

Thanks for your comments @blaxill!

I haven't used Clash in a while, but the first criteria I'd check is whether Clash accepts/generates correct HDL with the flattened version.

Yeah it collapses product types / drops port names this way.

It might be productive to rewrite this as a compiler plugin if that gives access to the evaluated types

Rewriting it as part of the compiler is definitely doable! I'll use the workaround for now, but I'm putting it on the stack of things to do :-).

[christiaanb]

Another "work-around" would be to use records, where it feels more natural to name every field. One can use barbies, see also https://groups.google.com/g/clash-language/c/S_jWFAS19nA/m/_E5bAgoFBAAJ, to make it easier to switch between a record of signals, and a signal of a record, while using the same record data type.

[paddytheplaster]

Hi again,

The following may be related (I pulled Clash earlier this morning).

I just rewrote my sources because a fix for issue 1524 is still pending. I then synthesized my top entity, which went fine, and compiled the resulting VHDL. However, something must have gone wrong because compiling theVHDL went too fast: a few seconds, whereas normally this takes minutes.

When I looked at the warnings I noticed the following:

No output dependent on input pin "RX".

The following is my top entity.

{-# ANN topEntity
    (Synthesize
    { t_name    = "paddy_circuit"
    , t_inputs  = [ PortName "CLOCK_XX"
                  , PortName "KEY0"
                  , PortName "RX"
                  ]
    , t_output  = PortProduct "" [ PortName "TX"
                                 , PortName "LED"
                                 ]
    }
  ) #-}
topEntity
  :: Clock  ClockDomain                    -- clock
  -> Signal ClockDomain Bool           -- reset
  -> Signal ClockDomain Bit              -- raw RX input
  -> Signal ClockDomain (Bit,BitVector 8)
topEntity clock_xx reset_button rx_raw
  = exposeClockResetEnable (top rx_raw) clock_xx rst en
  where en = enableGen
        rst = unsafeFromLowPolarity reset_button

The (quartus) .qsf file properly configures the input and output ports. When I simulate my circuit, I initialise it using the RX signal and it appears to run properly.

Could this be related? If not I'll see if I can find out more about the cause. Otherwise, I'll do some more waiting until this problem (issue 1535) is fixed.

Regards,

Paddy

[blaxill]

@paddytheplaster your issue is unrelated, this issue in this thread is specific to makeTopEntity which you do not use. For clarity it would be best to start a new issue with your problem and I suggest attaching the generated HDL too.

[paddytheplaster]

Thanks @blaxill.

Regards,

Paddy.