Clash should store clock periods in `fs` instead of `ps`

[martijnbastiaan]

We currently store the clock period in picoseconds. The limitations of ps hasn't been a problem so far, but with the introduction of dynamic clocks users might wish to represent tiny frequency differences which are currently unrepresentable:

clashi> hzToPeriod 200.001e6
4999
clashi> hzToPeriod 200.002e6
4999

To fix this, we should should store clock periods in femtoseconds (the minimal time unit in Verilog/VHDL) instead. To prevent silent breakage, I propose the following newtype:

newtype Femtosecond = Femtosecond Natural

Users of hzToPeriod shouldn't notice any difference, as long as we change the type signature accordingly from:

https://github.com/clash-lang/clash-compiler/blob/804067a6e52263329f286d742293723010b9cc9f/clash-prelude/src/Clash/Signal/Internal.hs#L1542-L1542

to

hzToPeriod :: HasCallStack => Ratio Natural -> Femtosecond

[martijnbastiaan]

Vivado:

Xilinx recommends that you run simulations using a resolution of 1 ps. Some Xilinx primitive components, such as MMCM, require a 1 ps resolution to work properly in either functional or timing simulation. There is no need to use a finer resolution, such as femtoseconds (fs) as some simulators will round the numbers while others will truncate the numbers.

Verilator:

TBCLOCK compares the current time to when the next edge will take place, and returns that amount of time in picoseconds. (Why picoseconds? It was an arbitrary decision based upon the reality that nanoseconds wasn’t enough for the application(s) shown above, and femptoseconds were overkill.)

Just switching to ps is therefor not enough. We'd need to make it configurable, adding a bit of complexity.

[DigitalBrains1]

Perhaps we could alter our clockGen so it accumulates fractional picoseconds and whenever that overflows it ticks one picosecond early or late to compensate? It's just an idea, I haven't really thought it through. It would not drift, but it would exhibit some jitter.

[martijnbastiaan]

That seems fairly difficult/surprising to have to explain. My proposal would be (although I haven't thought it through that much):

• Store it in fs on the Clash side of things
• Add a flag -fclash-hdl-timeunit={fs,ps}. I'm not sure what should be the default. I doubt many users need fs, so perhaps ps is better at least until we've got a better understanding of what happens to simulation speed (etc.) when set to fs.
• Make the Vivado workaround flag imply ps.
• Always use ps in the Verilator snippets.

[DigitalBrains1]

Right, I did mean only with dynamic clocks, not with static ones. So only the complex cases would lead to complex arrangements. (And obviously everything parsing clocks on the Haskell side needs to time it exactly the same, but that is a simple matter of defining a function for incrementing times rather than the current (+ period) all the prims do).

[edit] Seems like ModelSim can do femtosecond and Vivado can do ModelSim. But that may still run afoul of Xilinx's recommendation, and simulation of the MMCM primitive and who knows what other primitives. [/edit]

[DigitalBrains1]

In your proposal, I think you also need a flag to specify what resolution Haskell simulation uses, as we guarantee cycle exact simulation which is rather difficult when one HDL tool uses femtosecond resolution and another uses picosecond resolution.

[DigitalBrains1]

Users of hzToPeriod shouldn't notice any difference, as long as we change the type signature accordingly from:

https://github.com/clash-lang/clash-compiler/blob/804067a6e52263329f286d742293723010b9cc9f/clash-prelude/src/Clash/Signal/Internal.hs#L1542-L1542

to

hzToPeriod :: HasCallStack => Ratio Natural -> Femtosecond

How much are you willing to bet that there are users who use hzToPeriod for some other calculation (e.g., number of clock ticks to stall so a certain time has passed) that will then cause a type error? :-)

[rowanG077]

What about DomainPeriod? Will it return 'Femtosecond? This would cause a lot of type errors for me. See here for example: https://github.com/rowanG077/clash-netlist-gen-bug/blob/main/src/Example/SDRAM/Types.hs#L24

Not a big problem imo but it will definitely be a breaking API change.

[martijnbastiaan]

I guess it should @rowanG077. If we don't we'd risk silent breakage (right?).

Btw, there isn't a consensus on whether to move to fs at all yet.

[rowanG077]

@martijnbastiaan

Yes the move to a different type is definitely required to avoid silent breakage. Type errors are not such a big problem for me personally. But I thought we'd have to do the whole song and dance with deprecation and removal over multiple versions.

[martijnbastiaan]

But I thought we'd have to do the whole song and dance with deprecation and removal over multiple versions.

If there's a way, definitely. Sometimes things come up though where we don't really have a choice, and I think this is one of them.

[DigitalBrains1]

Why couldn't we leave static clocks as picoseconds and have dynamic clocks return a signal of femtoseconds? I don't see the need for femtosecond resolution for static clocks (I'm going to regret saying that in a few years, aren't I).

[martijnbastiaan]

I think we can. Dynamic clocks are a very experimental feature anyway, so we can abuse it in any way possible.

[martijnbastiaan]

Discussion on Slack: