chipsalliance / chipsalliance/chisel
Support for two-cycle read latency on SRAMs
- Dominant language
- Scala
- Stars
- 4.8k
- Forks
- 658
- Avg merge
- 18h 59m
- Merged PRs (30d)
- 14
Description
**Type of issue**: feature request
**Impact**: API addition (no impact on existing code) | API modification
**Development Phase**: request
**Other information**
**If the current behavior is a bug, please provide the steps to reproduce the problem:**
**What is the current behavior?**
**What is the expected behavior?**
**Please tell us about your environment:**
**What is the use case for changing the behavior?**
The `SyncReadMem` construct lets users specify an SRAM with one-cycle write and read latencies. However, some memory compilers let you build a register bank into the read path of an SRAM (which is usually a very tight timing path), resulting in an SRAM with a two-cycle read latency. In Chisel, you cannot specify an SRAM which matches that behavior.
FIRRTL supports SRAMs with arbitrary read/write latencies. I believe this is the current code path for going from `SyncReadMem` instances to FIRRTL memory objects:
- When users create a `SyncReadMem` instance, a `DefSeqMemory` is created. This gets converted to a FIRRTL `CDefMemory` object here: https://github.com/chipsalliance/chisel3/blob/df1b4aaf06cbca60bb48c3697d478dcdba48af36/core/src/main/scala/chisel3/internal/firrtl/Converter.scala#L130-L131
- Those `CDefMemory`s are converted to `DefMemory`s in the `RemoveCHIRRTL` pass, where the `readLatency` and `writeLatency` parameters are set to 1: https://github.com/chipsalliance/firrtl/blob/c00a4ebb0608f9ef98729e9b610a2678be2bc4fd/src/main/scala/firrtl/passes/RemoveCHIRRTL.scala#L133-L144
So, this would seem to be a feature of FIRRTL that Chisel doesn't currently expose. I don't know what the right API would be for this, or what range of read (and maybe write) latencies Chisel should expose.
Since FIRRTL associates the read/write latencies with the `DefMemory` object, I don't think we can have different ports with different latencies, so it wouldn't make sense to specify latency in the `read()` or `write()` methods of `SyncReadMem`. So, we could add `readLatency`/`writeLatency` constructor parameters to `SyncReadMem` which default to 1 and are `require`d to be 1 or greater.
Contributor guide
Assessment
This issue has not been assessed yet.