circt
circt copied to clipboard
llvm
[HWMemSimImpl] Add support for two flavors of read latencies
The current version of HWMemSimImpl implements read latency by inserting pipeline registers into the address and enable lines, which delays the time until the underlying data array is probed for a value. I think we have inherited this behaviour from SFC.
In real-world SRAMs there is a second component to the read latency: the time it takes for the value sampled in the data array to propagate to the output ports. The choices for this pre- and post-array latency has subtle implications on the read-under-write behaviour of the memory, mainly concerning the order of writes that a given read sees. From experience I would argue that most SRAMs have a post-array latency, and very little pre-array latency.
This commit splits the readLatency into two components: a pre- and post-latency. By default, the pass continues to treat the read latency annotated on FIR memories as a pure pre-array latency, and keeps the post-array latency at zero. However, it also introduces the read-latency-is-propagation-delay option which switches this behaviour into modeling that read latency entirely with a post-array delay.
The intent here is to have a non-functional change, but with an opt-in switch such that we can start testing the alternate (and in my opinion correct behaviour) on real-world designs. I suspect that none of SiFive's designs relies on this behaviour to ensure portability between different technology nodes, and we can just switch to this new behaviour for completeness' sake.
Also adds a off-by-default mem-read-latency-is-propagation-delay option to firtool.
Todo
- [x] Land #3537
Would it make sense to just define two latencies as opposed to having a bool choose between two interpretations of the same parameter? It's more verbose, but less confusing possibly.
Would it make sense to just define two latencies as opposed to having a bool choose between two interpretations of the same parameter?
Yeah that would definitely be nice. But that might require propagating the change all the way up to FIRRTL and Chisel, wouldn't it?
Eventually, yes. However, it doesn't have to be captured in Chisel or FIRRTL, just defining (or changing) how the existing Chisel and FIRRTL simplistic read/write latencies are lowered to the new representation.
I agree with @seldridge that it makes sense to just define two latencies. Presumably the write latency is then just the delay before the ready path first can sample it?
I'm going to close this PR but keep the branch around. With the seq.firmem op we'll get a better opportunity to implement this.