Make SRAMs directly emit FIRRTL memories #3955

debs-sifive · 2024-03-28T21:08:48Z

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Feature (or new API)
Backend code generation
Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

SRAMs updated to directly emit FIRRTL memories (as opposed to creating SyncReadMems.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

debs-sifive · 2024-03-28T21:10:01Z

src/main/scala/chisel3/util/SRAM.scala

+  *
+  * @param readPort used to parameterize this class
+  */
+private[chisel3] final class FirrtlMemoryReader[T <: Data](readPort: MemoryReadPort[T]) extends Bundle {


@jackkoenig should these bundles live here discreetly? The file is getting kind of long, so I was thinking about separating these out into a different file.

I think they're fine here, but leave a comment about why they're private and mention that the write and readwrite bundles are missing mask (and explain why).

debs-sifive · 2024-03-28T21:12:18Z

core/src/main/scala/chisel3/internal/firrtl/IR.scala

@@ -288,6 +288,16 @@ private[chisel3] object ir {
    readUnderWrite: fir.ReadUnderWrite.Value)
      extends Definition

+  case class FirrtlMemory(


Thoughts on naming this DefMem and renaming DefMem to CDefMem for name alignment? Or not that important?

I don't think it's important, fine as is IMO

seldridge · 2024-03-28T21:17:13Z

src/test/scala/chiselTests/util/SRAMSpec.scala

+    chirrtl should include("mem _sram_MEM")
+    chirrtl should include("data-type => UInt<8>")
+    chirrtl should include("depth => 32")
+    chirrtl should include("read-latency => 1")
+    chirrtl should include("write-latency => 1")
+    chirrtl should include("readwriter => RW0")
+    chirrtl should include("read-under-write => undefined")
+    chirrtl should include("connect _sram_MEM.RW0.addr, sram.readwritePorts[0].address")
+    chirrtl should include("connect _sram_MEM.RW0.clk, clock")
+    chirrtl should include("connect _sram_MEM.RW0.en, sram.readwritePorts[0].enable")
+    chirrtl should include("connect sram.readwritePorts[0].readData, _sram_MEM.RW0.rdata")
+    chirrtl should include("connect _sram_MEM.RW0.wdata, sram.readwritePorts[0].writeData")
+    chirrtl should include("connect _sram_MEM.RW0.wmode, _sram_MEM.RW0.wmode")


This makes me so happy...

jackkoenig

LGTM, mostly style nits, please fix the names of the memories before merging.

src/test/scala/chiselTests/util/SRAMSpec.scala

jackkoenig · 2024-03-28T21:24:15Z

src/main/scala/chisel3/util/SRAM.scala

+  *
+  * @param readPort used to parameterize this class
+  */
+private[chisel3] final class FirrtlMemoryReader[T <: Data](readPort: MemoryReadPort[T]) extends Bundle {


I think they're fine here, but leave a comment about why they're private and mention that the write and readwrite bundles are missing mask (and explain why).

src/main/scala/chisel3/util/SRAM.scala

jackkoenig · 2024-03-28T21:30:57Z

src/test/scala/chiselTests/util/SRAMSpec.scala

+      val maskedVecRecordMem = SRAM.masked(
+        size = 64,
+        tpe = Vec(
+          3,
+          new Bundle {
+            val x = UInt(3.W)
+            val y = Vec(4, Bool())
+          }
+        ),
+        numReadPorts = 1,
+        numWritePorts = 1,
+        numReadwritePorts = 0
+      )
+      val maskedVecRecordMemIo = IO(maskedVecRecordMem.cloneType)
+      maskedVecRecordMemIo <> maskedVecRecordMem


Good job writing good tests 🙂

jackkoenig · 2024-03-28T21:31:52Z

core/src/main/scala/chisel3/internal/firrtl/IR.scala

@@ -288,6 +288,16 @@ private[chisel3] object ir {
    readUnderWrite: fir.ReadUnderWrite.Value)
      extends Definition

+  case class FirrtlMemory(


I don't think it's important, fine as is IMO

core/src/main/scala/chisel3/RawModule.scala

src/test/scala/chiselTests/util/SRAMSpec.scala

core/src/main/scala/chisel3/Data.scala

seldridge · 2024-03-29T17:57:21Z

core/src/main/scala/chisel3/experimental/Analog.scala

+      case a: MemTypeBinding[_]    => a
+      case a: FirrtlMemTypeBinding => a


Nit: combining matches may be cleaner.

seldridge · 2024-03-29T18:44:16Z

core/src/main/scala/chisel3/internal/firrtl/IR.scala

-  case class Slot(imm: Node, name: String) extends Arg {
+  case class Slot(imm: Arg, name: String) extends Arg {


What's going on with this change?

Good eye, so recall (or learn very quickly) that Arg is the Chisel IR Expression. Node is the Arg that wraps a HasId. In SRAM, we manually build some nested Args to emit the connections to the mask fields of the ports because representing those as actual Chisel types is really hard. We needed to build some Slots that didn't have real Chisel Data backing them up (thus no HasIds). We figured it was pretty weird that Slot required a HasId (whereas other Args like Index don't), so this change makes more sense anyway and let us do what we needed to do to emit the connections for the mask.

Follow up question: https://github.com/chipsalliance/chisel/pull/3955/files#diff-f01d2f59c3998b1bb4f17da0c509aa21205a6682b1cd42e274dca0fa92ebf33dR477 is preventing this alternative id-less Slot from showing up when doing a select query. This seems to make sense with your explanation.

Yeah that is the only place where this change had any effect. @debs-sifive maybe we should write a test on what happens if you do a Select query for connections on a Module with an SRAM. These connections to the real ports of the SRAM are iffy and that Select may let users get at them...

I played around with this a bit, and we can indeed see some of the memory ports, but it's all output stuff:

List(PredicatedConnect(List(),MemWrapper.foo.readPorts[0].data: Wire[UInt<8>],MemWrapper.foo_sram.R0.data: [UInt<8>],false), PredicatedConnect(List(),MemWrapper.foo.readwritePorts[0].readData: Wire[UInt<8>],MemWrapper.foo_sram.RW0.rdata: [UInt<8>],false))

So it looks like we can't drive anything on the actual memory with Select. If we want Select functionality with SRAM, I say we do it in a separate PR :)

src/test/scala/chiselTests/util/SRAMSpec.scala

sequencer · 2024-03-30T02:50:05Z

Really appreciate. And we finally have this feature! I wanna kill all Mem usage for a long time. After this feature leading to chisel, maybe we can finally deprecate Mem API.
Some questions but I don’t think it will block this PR:

does it support suggestName?(some time sram isn’t sram, but RF). Originally it doesn’t because it warp cmem.
maybe in the future. It can be directly lowered to SRAM Intrinsic? Then we can simply add more interface to sram. For example extend MBIST interface.
is possible to emit sram with Instance Select API for avoiding different lower pass for memory behavior models?

debs-sifive · 2024-04-01T16:21:39Z

@sequencer I haven't really worked with the Instance Select API, so not sure about that Q.

does it support suggestName?(some time sram isn’t sram, but RF). Originally it doesn’t because it warp cmem.

It currently supports suggestName of the SRAMInterface. I believe we can support suggestName of the memory itself, since the memory target is a HasID. But it's not implemented in this iteration.

* Specify right convention for modules * Specify `desiredName` as `defname` for `extmodule` Removed FixedPoint in muxes-and-input-selection.md (#3956) Make SRAMs directly emit FIRRTL memories (#3955) Add a new BoringUtils.drive API for boring to drive a sink. (#3960) This API allows users to bore to a sink they plan to drive, which complements the existing API to bore from a source to read. Support literals in DataView (#3964) Add requireIsAnnotatable for better errors when annotating literals (#3968)

have SRAMs emit FIRRTL mems

5ca283b

debs-sifive added the Backend Code Generation Affects backend code generation, will be included in release notes label Mar 28, 2024

debs-sifive requested a review from jackkoenig March 28, 2024 21:08

debs-sifive commented Mar 28, 2024

View reviewed changes

seldridge reviewed Mar 28, 2024

View reviewed changes

update Panama

d6400b6

jackkoenig approved these changes Mar 28, 2024

View reviewed changes

mwachs5 reviewed Mar 28, 2024

View reviewed changes

src/test/scala/chiselTests/util/SRAMSpec.scala Outdated Show resolved Hide resolved

address reviews

8404852

jackkoenig added this to the 7.0 milestone Mar 28, 2024

fix up tests

470a225

jackkoenig approved these changes Mar 29, 2024

View reviewed changes

seldridge approved these changes Mar 29, 2024

View reviewed changes

style fixes

bd0c07b

debs-sifive merged commit feab7a7 into main Apr 2, 2024
14 checks passed

debs-sifive deleted the sram-to-firrtlmem branch April 2, 2024 02:55

SpriteOvO mentioned this pull request Apr 22, 2024

Hard to obtain needed context to build IR for referencing results of FirrtlMemory #4022

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SRAMs directly emit FIRRTL memories #3955

Make SRAMs directly emit FIRRTL memories #3955

debs-sifive commented Mar 28, 2024

debs-sifive Mar 28, 2024

jackkoenig Mar 28, 2024

debs-sifive Mar 28, 2024

jackkoenig Mar 28, 2024

seldridge Mar 28, 2024 •

edited

Loading

jackkoenig left a comment

jackkoenig Mar 28, 2024

jackkoenig Mar 28, 2024

jackkoenig Mar 28, 2024

seldridge Mar 29, 2024

seldridge Mar 29, 2024

jackkoenig Mar 29, 2024

seldridge Mar 29, 2024

jackkoenig Mar 29, 2024

debs-sifive Mar 29, 2024

sequencer commented Mar 30, 2024

debs-sifive commented Apr 1, 2024

		case a: MemTypeBinding[_] => a
		case a: FirrtlMemTypeBinding => a

		case class Slot(imm: Node, name: String) extends Arg {
		case class Slot(imm: Arg, name: String) extends Arg {

Make SRAMs directly emit FIRRTL memories #3955

Make SRAMs directly emit FIRRTL memories #3955

Conversation

debs-sifive commented Mar 28, 2024

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sequencer commented Mar 30, 2024

debs-sifive commented Apr 1, 2024

seldridge Mar 28, 2024 •

edited

Loading