Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

[smem] fix read-under-write serialization #2595

Merged
merged 3 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[smem] fix read-under-write serialization
Also adds some tests for the parser and
the serializer.
  • Loading branch information
ekiwi committed Jan 25, 2023
commit 79f7119a3d5fed2afe228ef908ae4d3ede14fe6d
6 changes: 5 additions & 1 deletion src/main/scala/firrtl/ir/Serializer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,11 @@ object Serializer {
// WIR
case firrtl.CDefMemory(info, name, tpe, size, seq, readUnderWrite) =>
if (seq) b ++= "smem " else b ++= "cmem "
b ++= name; b ++= " : "; s(tpe); b ++= " ["; b ++= size.toString(); b += ']'; s(info)
b ++= name; b ++= " : "; s(tpe); b ++= " ["; b ++= size.toString(); b += ']'
if (readUnderWrite != ReadUnderWrite.Undefined) {
b += ' '; b ++= readUnderWrite.toString
}
ekiwi marked this conversation as resolved.
Show resolved Hide resolved
s(info)
case firrtl.CDefMPort(info, name, _, mem, exps, direction) =>
b ++= direction.serialize; b ++= " mport "; b ++= name; b ++= " = "; b ++= mem
b += '['; s(exps.head); b ++= "], "; s(exps(1)); s(info)
Expand Down
42 changes: 42 additions & 0 deletions src/test/scala/firrtlTests/ParserSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,48 @@ class ParserSpec extends FirrtlFlatSpec {
firrtl.Parser.parse(input)
}
}

it should "parse smem read-under-write behavior" in {
val undefined = firrtl.Parser.parse(SMemTestCircuit.src(""))
assert(SMemTestCircuit.findRuw(undefined) == ReadUnderWrite.Undefined)

val undefined2 = firrtl.Parser.parse(SMemTestCircuit.src(" undefined"))
assert(SMemTestCircuit.findRuw(undefined2) == ReadUnderWrite.Undefined)

val old = firrtl.Parser.parse(SMemTestCircuit.src(" old"))
assert(SMemTestCircuit.findRuw(old) == ReadUnderWrite.Old)

val readNew = firrtl.Parser.parse(SMemTestCircuit.src(" new"))
assert(SMemTestCircuit.findRuw(readNew) == ReadUnderWrite.New)
}
}

/** used to test parsing and serialization of smems */
object SMemTestCircuit {
def src(ruw: String): String =
s"""circuit Example :
| module Example :
| input clock : Clock
| input reset : UInt<1>
| input addr : UInt<3>
| output data : UInt<8>
|
| smem mem : UInt<8> [8] $ruw@[main.scala 10:25]
| wire _data_WIRE : UInt @[main.scala 11:20]
| _data_WIRE is invalid @[main.scala 11:20]
| when UInt<1>("h1") : @[main.scala 11:20]
| _data_WIRE <= addr @[main.scala 11:20]
| node _data_T = or(_data_WIRE, UInt<3>("h0")) @[main.scala 11:20]
| node _data_T_1 = bits(_data_T, 2, 0) @[main.scala 11:20]
| read mport data_MPORT = mem[_data_T_1], clock @[main.scala 11:20]
| data <= data_MPORT @[main.scala 11:8]
|""".stripMargin
ekiwi marked this conversation as resolved.
Show resolved Hide resolved

def findRuw(c: Circuit): ReadUnderWrite.Value = {
val main = c.modules.head.asInstanceOf[ir.Module]
val mem = main.body.asInstanceOf[ir.Block].stmts.collectFirst { case m: CDefMemory => m }.get
mem.readUnderWrite
}
}

class ParserPropSpec extends FirrtlPropSpec {
Expand Down
15 changes: 15 additions & 0 deletions src/test/scala/firrtlTests/SerializerSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,19 @@ class SerializerSpec extends AnyFlatSpec with Matchers {
val serialized = Serializer.serialize(when, 1)
serialized should be(" when cond :\n skip\n")
}

it should "serialize read-under-write behavior for smems correctly" in {
def parseSerializeParse(src: String): Circuit = Parser.parse(Parser.parse(src).serialize)
val undefined = parseSerializeParse(SMemTestCircuit.src(""))
assert(SMemTestCircuit.findRuw(undefined) == ReadUnderWrite.Undefined)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: using non-assert matchers may be more readable:

SMemTestCircuit.findRuw(undefined) should be (ReadUnderWrite.Undefined)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. I like to keep things simple and scalatest will still highlight the failure correctly when using plain asserts. The should or must matchers have some weird corner cases (I forgot what they were, but there was some trouble in the past that would have been avoided by sticking to the basics).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you insist on using the should and I will change it in order to get this change in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no problem with using them here and continuing to use them aligns with all the other tests in this file.

For corner cases, you may be thinking of string matching on include vs contains? The former looks for a substring match and the latter looks for an exact match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't care. It's good to land as-is. 👍


val undefined2 = parseSerializeParse(SMemTestCircuit.src(" undefined"))
assert(SMemTestCircuit.findRuw(undefined2) == ReadUnderWrite.Undefined)

val old = parseSerializeParse(SMemTestCircuit.src(" old"))
assert(SMemTestCircuit.findRuw(old) == ReadUnderWrite.Old)

val readNew = parseSerializeParse(SMemTestCircuit.src(" new"))
assert(SMemTestCircuit.findRuw(readNew) == ReadUnderWrite.New)
}
}