Implement read-write memory accessors for SyncReadMem #3190

jared-barocsi · 2023-04-17T23:52:23Z

Implements SyncReadMem.readWrite(address, writeData, enabled, isWrite) as a way to explicitly instantiate a readwrite port for a memory. This addresses the problem of unintentionally creating multiple read or write ports through multiple calls of SyncReadMem.read() and/or SyncReadMem.write(). Additionally, implements clocked and masked versions of readWrite for parity with the existing read and write functions.

Compare:

val myMem = SyncReadMem(100, UInt(32.W))
val readValue = myMem.read(someAddress)
when (timeToWrite)
  myMem.write(someAddress, valueToWrite)
when (timeToWrite) 
  myMem.write(someAddress, valueToWrite)

which instantiates a 1R2W memory, as opposed to

val myMem = SyncReadMem(100, UInt(32.W))
val rwPort = myMem.readWrite(someAddress, writeData, portEnabled, timeToWrite)

val readValue = rwPort
when (timeToWrite)
  writeData = valueToWrite
when (timeToWrite)
  writeData = differentValueToWrite

which instantiates a 1RW memory.

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)

Desired Merge Strategy

squash and merge

Release Notes

SyncReadMem.readWrite(address, writeData, enabled, isWrite) explicitly generates a read-write port that supports both read and write access to the memory.

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.5.x or 3.6.x depending on impact, API modification or big change: 5.0.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.

mwachs5 · 2023-04-18T12:57:30Z

Can we add this to the Memories explanations doc

mwachs5 · 2023-04-18T12:58:05Z

Or add a new cookbook "how to I make a 1readwrite port memory, how do I make a 1r1w port memory"?

core/src/main/scala/chisel3/Mem.scala

mwachs5 · 2023-04-19T16:47:27Z

core/src/main/scala/chisel3/Mem.scala

+    *   val mem = Module(new MyMemWrapper)
+    *   mem.io := DontCare
+    *
+    *   val rdata = Wire(UInt(2.W))


this code would not quite compile

mwachs5 · 2023-04-19T16:47:39Z

core/src/main/scala/chisel3/Mem.scala

+    *   val rdata = Wire(UInt(2.W))
+    *   readData := DontCare


Suggested change

* val rdata = Wire(UInt(2.W))

* readData := DontCare

* val rdata = Wire(UInt(2.W))

* rdata := DontCare

mwachs5 · 2023-04-19T16:48:07Z

core/src/main/scala/chisel3/Mem.scala

+    *   readData := DontCare
+    *
+    *   // Read from the memory port
+    *   when(doRead) {


what is doRead? an Input?

mwachs5 · 2023-04-19T16:48:38Z

core/src/main/scala/chisel3/Mem.scala

+    *   }
+    *   // Read data will show up on the next cycle
+    *   when(timeToRead) {
+    *     readData := mem.io.rdata


this is a little confusing, the user is going to have to understand that the rddata is available the following cycle. Let's just omit this part

mwachs5 · 2023-04-19T16:48:59Z

core/src/main/scala/chisel3/Mem.scala

+    *
+    *   val rdata = Wire(UInt(2.W))
+    *   readData := DontCare
+    *


we should default mem.io.enable to false.B

core/src/main/scala/chisel3/Mem.scala

docs/src/explanations/memories.md

scaladoc nits

mwachs5 · 2023-04-21T16:49:29Z

This LGTM. I am not sure if there are any issues on the CIRCT side in interpreting this. Can @jackkoenig @seldridge give a review?

seldridge

A long missing API. Great work!

Minor nits throughout. Good to land whenever you're ready.

seldridge · 2023-04-25T03:29:32Z

core/src/main/scala/chisel3/Mem.scala

+    val a = Wire(UInt())
+    a := DontCare


Using val _a indicates better that this is a temporary. Same for port. Basically, I would use _ for any hardware component created in a library/utility.

Nits/golf:

Suggested change

val a = Wire(UInt())

a := DontCare

val _a = WireDefault(chiselTypeOf(addr), DontCare)

Same comments for the masked version.

Is the declaration of a separate from the connection of addr to it required for the enable to infer correctly @jared-barocsi? Or why else is is split?

Also are we sure returning the port works? Such value capturing is often problematic for lexical scoping, what happens if someone uses the return value of this function? Perhaps we should be assigning the read data to a wire? The integration test is passing so that suggests the lexical scoping is okay or at least supported by firtool but I'd be curious to see what the FIRRTL actually looks like.

seldridge · 2023-04-25T03:37:09Z

core/src/main/scala/chisel3/Mem.scala

+    * }
+    * }}}
+    *
+    * @note this is only allowed if the memory's element data type is a Vec


At least in FIRRTL this is allowed so long as the type of the mask is the same type as the underlying data type with the leaf types as all 1-bit UInts. I.e., this is supported on any aggregate including bundles.

This is a relatively niche feature and pretty rare. I am fine to start this as only working for Vec (which makes sense).

seldridge · 2023-04-25T03:45:13Z

src/test/scala/chiselTests/Mem.scala

@@ -188,6 +188,156 @@ private class TrueDualPortMemory(addrW: Int, dataW: Int) extends RawModule {
  }
 }

+class MemReadWriteTester extends BasicTester {


I'm not a huge fan of big integration tests like this. However, this is entirely consistent with the rest of this existing file.

Can you open an issue to move all refactor this to move the integration tests into the integration test area and slim these down to just checking the CHIRRTL?

seldridge · 2023-04-25T03:46:26Z

I didn't notice you had auto-merge on. 😂 Any cleanup/feedback can happen post-merge. 😉

jackkoenig · 2023-04-25T22:19:37Z

src/test/scala/chiselTests/Mem.scala

+      enable := true.B;
+      isWrite := false.B;


Can you please remove all of the semicolons in this file?

Adds new APIs to generate explicit read-write (rdwr) ports for a SyncReadMem, and all masked or clocked variants. --------- Co-authored-by: Megan Wachs <megan@sifive.com> (cherry picked from commit 1e6f2b4)

Adds new APIs to generate explicit read-write (rdwr) ports for a SyncReadMem, and all masked or clocked variants. --------- Co-authored-by: Megan Wachs <megan@sifive.com> (cherry picked from commit 1e6f2b4) Co-authored-by: Jared Barocsi <82000041+jared-barocsi@users.noreply.github.com>

jackkoenig · 2023-04-26T19:16:24Z

core/src/main/scala/chisel3/Mem.scala

+  )(
+    implicit evidence: T <:< Vec[_]
+  ): T = masked_readWrite_impl(idx, writeData, mask, en, isWrite, clock, true)
+
+  private def masked_readWrite_impl(
+    addr:    UInt,
+    data:    T,
+    mask:    Seq[Bool],
+    enable:  Bool,
+    isWrite: Bool,
+    clock:   Clock,
+    warn:    Boolean
+  )(
+    implicit evidence: T <:< Vec[_]
+  ): T = {
+    implicit val sourceInfo = UnlocatableSourceInfo
+    val a = Wire(UInt())


It would also be good to have a source locator here, since we're already taking an implicit argument we can just take the sourceInfo argument. Users won't be able to immediately call .apply on the return value, but due to the existing implicit argument, they already can't.

#3213) * Implement read-write memory accessors for SyncReadMem (#3190) Adds new APIs to generate explicit read-write (rdwr) ports for a SyncReadMem, and all masked or clocked variants. --------- Co-authored-by: Megan Wachs <megan@sifive.com> (cherry picked from commit 1e6f2b4) * Add CompileOptions to SyncReadMem.readWrite methods --------- Co-authored-by: Jared Barocsi <82000041+jared-barocsi@users.noreply.github.com> Co-authored-by: Jack Koenig <koenig@sifive.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

jardhu added 9 commits April 14, 2023 14:04

Fixup 1: Add Mem.readWrite to declare RDWR ports

e21ca01

Merge branch 'main' into mem-explicit-port-apis

0946329

Implement readWrite enable-sensitive functions in SyncReadMem

77de908

Refactor all readWrite functionality from MemBase to SyncReadMem

768d784

Tests for readWrite

43ab2f0

Implement masked readWrites

4a2bbdc

Masked readwrite tests

91ab433

Scalafmt

2b8fc7b

Mention readWrite function in memory docs

675f171

jackkoenig added the Feature New feature, will be included in release notes label Apr 18, 2023

Fix macro issues

624320d

mwachs5 reviewed Apr 18, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 18, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 18, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 18, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 18, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Show resolved Hide resolved

jardhu added 3 commits April 18, 2023 13:30

Documentation examples and rewording/fixups

5f114d3

Apply fixups to ALL the new readWrite functions

f4c56af

Fix error in masked write example

6cc142a

mwachs5 reviewed Apr 19, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 19, 2023

View reviewed changes

core/src/main/scala/chisel3/Mem.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 19, 2023

View reviewed changes

docs/src/explanations/memories.md Outdated Show resolved Hide resolved

mwachs5 reviewed Apr 19, 2023

View reviewed changes

docs/src/explanations/memories.md Outdated Show resolved Hide resolved

mwachs5 and others added 2 commits April 19, 2023 10:07

Update Mem.scala

94c0d3e

scaladoc nits

Scaladoc fixes

4e85fa6

jared-barocsi enabled auto-merge (squash) April 21, 2023 17:22

Mdoc fixes

c8a4c48

seldridge approved these changes Apr 25, 2023

View reviewed changes

jared-barocsi merged commit 1e6f2b4 into main Apr 25, 2023

jared-barocsi deleted the mem-readwrite-port-apis branch April 25, 2023 03:45

jackkoenig reviewed Apr 25, 2023

View reviewed changes

jackkoenig added this to the 3.6.x milestone Apr 25, 2023

mergify bot added the Backported This PR has been backported label Apr 25, 2023

This was referenced Apr 25, 2023

Implement read-write memory accessors for SyncReadMem (backport #3190) #3213

Merged

Implement read-write memory accessors for SyncReadMem (backport #3190) #3214

Merged

jackkoenig reviewed Apr 26, 2023

View reviewed changes

jackkoenig mentioned this pull request May 9, 2023

Explicit readwrite ports for memories #238

Closed

sequencer mentioned this pull request Jun 20, 2023

Fix BuildInfo in 3.6.x #3371

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement read-write memory accessors for SyncReadMem #3190

Implement read-write memory accessors for SyncReadMem #3190

jared-barocsi commented Apr 17, 2023 •

edited

Loading

mwachs5 commented Apr 18, 2023

mwachs5 commented Apr 18, 2023

mwachs5 Apr 19, 2023

mwachs5 Apr 19, 2023

mwachs5 Apr 19, 2023

mwachs5 Apr 19, 2023 •

edited

Loading

mwachs5 Apr 19, 2023

mwachs5 commented Apr 21, 2023

seldridge left a comment

seldridge Apr 25, 2023

seldridge Apr 25, 2023

seldridge Apr 25, 2023

jackkoenig Apr 25, 2023

seldridge Apr 25, 2023

seldridge Apr 25, 2023

seldridge commented Apr 25, 2023

jackkoenig Apr 25, 2023

jackkoenig Apr 26, 2023

	val a = Wire(UInt())
	a := DontCare
	val _a = WireDefault(chiselTypeOf(addr), DontCare)

Implement read-write memory accessors for SyncReadMem #3190

Implement read-write memory accessors for SyncReadMem #3190

Conversation

jared-barocsi commented Apr 17, 2023 • edited Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

mwachs5 commented Apr 18, 2023

mwachs5 commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwachs5 Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwachs5 commented Apr 21, 2023

seldridge 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

seldridge commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jared-barocsi commented Apr 17, 2023 •

edited

Loading

mwachs5 Apr 19, 2023 •

edited

Loading