-
Notifications
You must be signed in to change notification settings - Fork 592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement read-write memory accessors for SyncReadMem #3190
Changes from 13 commits
e21ca01
0946329
77de908
768d784
43ab2f0
4a2bbdc
91ab433
2b8fc7b
675f171
624320d
5f114d3
f4c56af
6cc142a
94c0d3e
4e85fa6
c8a4c48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -392,4 +392,272 @@ sealed class SyncReadMem[T <: Data] private[chisel3] ( | |||||||||
} | ||||||||||
// note: we implement do_read(addr) for SyncReadMem in terms of do_read(addr, en) in order to ensure that | ||||||||||
// `mem.read(addr)` will always behave the same as `mem.read(addr, true.B)` | ||||||||||
|
||||||||||
/** Generates an explicit read-write port for this SyncReadMem. Note that this does not infer | ||||||||||
* port directionality based on connection semantics and the `when` context unlike SyncReadMem.apply(), | ||||||||||
* so the behavior of the port must be controlled by changing the values of the input parameters. | ||||||||||
* | ||||||||||
* @param idx memory element index to write into | ||||||||||
* @param writeData new data to write | ||||||||||
* @param enable enables access to the memory | ||||||||||
* @param isWrite performs a write instead of a read when enable is true; the return | ||||||||||
* value becomes undefined when this parameter is true | ||||||||||
* | ||||||||||
* @return The value of the memory at idx when enable is true and isWrite is false, | ||||||||||
* or an undefined value otherwise. | ||||||||||
* | ||||||||||
* @example Controlling a read/write port with IO signals | ||||||||||
* {{{ | ||||||||||
* class MyMemWrapper extends Module { | ||||||||||
* val width = 2 | ||||||||||
* | ||||||||||
* val io = IO(new Bundle { | ||||||||||
* val address = Input(UInt()) | ||||||||||
* val wdata = Input(UInt(width.W)) | ||||||||||
* val enable = Input(Bool()) | ||||||||||
* val isWrite = Input(Bool()) | ||||||||||
* val rdata = Output(UInt(width.W)) | ||||||||||
* }) | ||||||||||
* | ||||||||||
* val mem = SyncReadMem(2, UInt(width.W)) | ||||||||||
* val rwPort = mem.readWrite(io.address, io.wdata, io.enable, io.isWrite) | ||||||||||
* io.rdata := rwPort | ||||||||||
* } | ||||||||||
* | ||||||||||
* class Top extends Module { | ||||||||||
* val mem = Module(new MyMemWrapper) | ||||||||||
* mem.io := DontCare | ||||||||||
* | ||||||||||
* val rdata = Wire(UInt(2.W)) | ||||||||||
* readData := DontCare | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
* | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should default mem.io.enable to false.B |
||||||||||
* // Read from the memory port | ||||||||||
* when(doRead) { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is |
||||||||||
* mem.io.enable := true.B | ||||||||||
* mem.io.isWrite := false.B | ||||||||||
* mem.io.address := someAddress | ||||||||||
* } | ||||||||||
* // Read data will show up on the next cycle | ||||||||||
* when(timeToRead) { | ||||||||||
* readData := mem.io.rdata | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||
* } | ||||||||||
* | ||||||||||
* // Write to the memory | ||||||||||
* when(doWrite) { | ||||||||||
* mem.io.enable := true.B | ||||||||||
* mem.io.isWrite := true.B | ||||||||||
* mem.io.address := anotherAddress | ||||||||||
* mem.io.wdata := 3.U | ||||||||||
* } | ||||||||||
* } | ||||||||||
* }}} | ||||||||||
*/ | ||||||||||
def readWrite(idx: UInt, writeData: T, en: Bool, isWrite: Bool): T = macro SourceInfoTransform.idxDataEnIswArg | ||||||||||
|
||||||||||
/** @group SourceInfoTransformMacro */ | ||||||||||
def do_readWrite(idx: UInt, writeData: T, en: Bool, isWrite: Bool)(implicit sourceInfo: SourceInfo): T = | ||||||||||
_readWrite_impl(idx, writeData, en, isWrite, Builder.forcedClock, true) | ||||||||||
|
||||||||||
/** Generates an explicit read-write port for this SyncReadMem, using a clock that may be | ||||||||||
* different from the implicit clock. | ||||||||||
* | ||||||||||
* @param idx memory element index to write into | ||||||||||
* @param writeData new data to write | ||||||||||
* @param enable enables access to the memory | ||||||||||
* @param isWrite performs a write instead of a read when enable is true; the return | ||||||||||
* value becomes undefined when this parameter is true | ||||||||||
* @param clock clock to bind to this read-write port | ||||||||||
* | ||||||||||
* @return The value of the memory at idx when enable is true and isWrite is false, | ||||||||||
* or an undefined value otherwise. | ||||||||||
*/ | ||||||||||
jared-barocsi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
def readWrite(idx: UInt, writeData: T, en: Bool, isWrite: Bool, clock: Clock): T = | ||||||||||
macro SourceInfoTransform.idxDataEnIswClockArg | ||||||||||
|
||||||||||
/** @group SourceInfoTransformMacro */ | ||||||||||
def do_readWrite( | ||||||||||
idx: UInt, | ||||||||||
data: T, | ||||||||||
en: Bool, | ||||||||||
isWrite: Bool, | ||||||||||
clock: Clock | ||||||||||
)( | ||||||||||
implicit sourceInfo: SourceInfo | ||||||||||
): T = | ||||||||||
_readWrite_impl(idx, data, en, isWrite, clock, true) | ||||||||||
|
||||||||||
/** @group SourceInfoTransformMacro */ | ||||||||||
private def _readWrite_impl( | ||||||||||
addr: UInt, | ||||||||||
data: T, | ||||||||||
enable: Bool, | ||||||||||
isWrite: Bool, | ||||||||||
clock: Clock, | ||||||||||
warn: Boolean | ||||||||||
)( | ||||||||||
implicit sourceInfo: SourceInfo | ||||||||||
): T = { | ||||||||||
val a = Wire(UInt()) | ||||||||||
a := DontCare | ||||||||||
Comment on lines
+473
to
+474
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nits/golf:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments for the masked version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the declaration of 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. |
||||||||||
|
||||||||||
var port: Option[T] = None | ||||||||||
when(enable) { | ||||||||||
a := addr | ||||||||||
port = Some(super.do_apply_impl(a, clock, MemPortDirection.RDWR, warn)) | ||||||||||
|
||||||||||
when(isWrite) { | ||||||||||
port.get := data | ||||||||||
} | ||||||||||
} | ||||||||||
port.get | ||||||||||
} | ||||||||||
|
||||||||||
/** Generates an explicit read-write port for this SyncReadMem, with a bytemask for | ||||||||||
* performing partial writes to a Vec element. | ||||||||||
* | ||||||||||
* @param idx memory element index to write into | ||||||||||
* @param writeData new data to write | ||||||||||
* @param mask the write mask as a Seq of Bool: a write to the Vec element in | ||||||||||
* memory is only performed if the corresponding mask index is true. | ||||||||||
* @param enable enables access to the memory | ||||||||||
* @param isWrite performs a write instead of a read when enable is true; the return | ||||||||||
* value becomes undefined when this parameter is true | ||||||||||
* | ||||||||||
* @return The read Vec value of the memory at idx when enable is true and isWrite is false, | ||||||||||
* or an undefined value otherwise. | ||||||||||
* | ||||||||||
* @example Controlling a read/masked write port with IO signals | ||||||||||
* {{{ | ||||||||||
* class MyMaskedMemWrapper extends Module { | ||||||||||
* val width = 2 | ||||||||||
* | ||||||||||
* val io = IO(new Bundle { | ||||||||||
* val address = Input(UInt()) | ||||||||||
* val wdata = Input(UInt(width.W)) | ||||||||||
* val mask = Input(Vec(2, Bool())) | ||||||||||
* val enable = Input(Bool()) | ||||||||||
* val isWrite = Input(Bool()) | ||||||||||
* val rdata = Output(UInt(width.W)) | ||||||||||
* }) | ||||||||||
* | ||||||||||
* val mem = SyncReadMem(2, Vec(2, UInt(width.W))) | ||||||||||
* val rwPort = mem.readWrite(io.address, io.wdata, io.mask, io.enable, io.isWrite) | ||||||||||
* io.rdata := rwPort | ||||||||||
* } | ||||||||||
* | ||||||||||
* class Top extends Module { | ||||||||||
* val mem = Module(new MyMemWrapper) | ||||||||||
* mem.io := DontCare | ||||||||||
* | ||||||||||
* val rdata = Wire(UInt(2.W)) | ||||||||||
* readData := DontCare | ||||||||||
* | ||||||||||
* // Read from the memory port | ||||||||||
* when(doRead) { | ||||||||||
* mem.io.enable := true.B | ||||||||||
* mem.io.isWrite := false.B | ||||||||||
* mem.io.address := someAddress | ||||||||||
* } | ||||||||||
* // Read data will show up on the next cycle | ||||||||||
* when(timeToRead) { | ||||||||||
jared-barocsi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
* readData := mem.io.rdata | ||||||||||
* } | ||||||||||
* | ||||||||||
* // Write to all Vec entries at a specific address in the memory | ||||||||||
* when(doNormalWrite) { | ||||||||||
* mem.io.enable := true.B | ||||||||||
* mem.io.isWrite := true.B | ||||||||||
* mem.io.mask := VecInit(true.B, true.B) | ||||||||||
* mem.io.address := anotherAddress | ||||||||||
* mem.io.wdata := VecInit(2.U, 3.U) | ||||||||||
* } | ||||||||||
* | ||||||||||
* // Write to the memory with a partial bytemask | ||||||||||
* when(doMaskedWrite) { | ||||||||||
* mem.io.enable := true.B | ||||||||||
* mem.io.isWrite := true.B | ||||||||||
* mem.io.mask := VecInit(true.B, false.B) // Write only the first element of the vec at `anotherAddress` | ||||||||||
* mem.io.address := anotherAddress | ||||||||||
* mem.io.wdata := VecInit(0.U, 0.U) | ||||||||||
* } | ||||||||||
* } | ||||||||||
* }}} | ||||||||||
* | ||||||||||
* @note this is only allowed if the memory's element data type is a Vec | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||
*/ | ||||||||||
def readWrite( | ||||||||||
idx: UInt, | ||||||||||
writeData: T, | ||||||||||
mask: Seq[Bool], | ||||||||||
en: Bool, | ||||||||||
isWrite: Bool | ||||||||||
)( | ||||||||||
implicit evidence: T <:< Vec[_] | ||||||||||
): T = masked_readWrite_impl(idx, writeData, mask, en, isWrite, Builder.forcedClock, true) | ||||||||||
|
||||||||||
/** Generates an explicit read-write port for this SyncReadMem, with a bytemask for | ||||||||||
* performing partial writes to a Vec element and a clock that may be different from | ||||||||||
* the implicit clock. | ||||||||||
* | ||||||||||
* @param idx memory element index to write into | ||||||||||
* @param writeData new data to write | ||||||||||
* @param mask the write mask as a Seq of Bool: a write to the Vec element in | ||||||||||
* memory is only performed if the corresponding mask index is true. | ||||||||||
* @param enable enables access to the memory | ||||||||||
* @param isWrite performs a write instead of a read when enable is true; the return | ||||||||||
* value becomes undefined when this parameter is true | ||||||||||
* @param clock clock to bind to this read-write port | ||||||||||
* | ||||||||||
* @return The read Vec value of the memory at idx when enable is true and isWrite is false, | ||||||||||
* or an undefined value otherwise. | ||||||||||
* | ||||||||||
* @note this is only allowed if the memory's element data type is a Vec | ||||||||||
jared-barocsi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
*/ | ||||||||||
def readWrite( | ||||||||||
idx: UInt, | ||||||||||
writeData: T, | ||||||||||
mask: Seq[Bool], | ||||||||||
en: Bool, | ||||||||||
isWrite: Bool, | ||||||||||
clock: Clock | ||||||||||
)( | ||||||||||
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()) | ||||||||||
Comment on lines
+558
to
+574
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||
a := DontCare | ||||||||||
|
||||||||||
var port: Option[T] = None | ||||||||||
when(enable) { | ||||||||||
a := addr | ||||||||||
port = Some(super.do_apply_impl(a, clock, MemPortDirection.RDWR, warn)) | ||||||||||
val accessor = port.get.asInstanceOf[Vec[Data]] | ||||||||||
|
||||||||||
when(isWrite) { | ||||||||||
val dataVec = data.asInstanceOf[Vec[Data]] | ||||||||||
if (accessor.length != dataVec.length) { | ||||||||||
Builder.error(s"Mem write data must contain ${accessor.length} elements (found ${dataVec.length})") | ||||||||||
} | ||||||||||
if (accessor.length != mask.length) { | ||||||||||
Builder.error(s"Mem write mask must contain ${accessor.length} elements (found ${mask.length})") | ||||||||||
} | ||||||||||
|
||||||||||
for (((cond, p), datum) <- mask.zip(accessor).zip(dataVec)) | ||||||||||
when(cond) { p := datum } | ||||||||||
} | ||||||||||
} | ||||||||||
port.get | ||||||||||
} | ||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code would not quite compile