chipsalliance / chipsalliance/chisel

Support for two-cycle read latency on SRAMs

Open
#2,316 5 comments 2 reactions 1 assignee Assigned to @jackkoenig View on GitHub
feature request
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.