From d9de0574f56645944187ace0642a7bfaa3e47373 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Wed, 21 Jun 2023 17:58:49 -0400 Subject: [PATCH] [codegen] Emit literal identifiers for numeric ids Change the FIRRTL serializer to use literal identifiers if any identifiers begin with a leading digit. This is a FIRRTL 3.0.0 feature that simplifies parsing. This enables literal identifier emission anywhere. However, this code path is only reachable via MixedVec due to mangling of numeric names to add a leading underscore in all other circumstances. In the future, this commit will enable removing this restriction and switching to literal identifiers when this happens. No changes are made to annotation targets. E.g., the literal identifier, "wire `42`" in circuit "Foo" and module "Bar" is still referred to by a local target "~Foo|Bar>42". This is intentional (for now). Targets don't have parsing ambiguity like FIRRTL text and the name of this wire is still "42" not "`42`". It follows that the storage of this name in FIRRTL IR is not changed. This is the same way that this is handled in CIRCT. Signed-off-by: Schuyler Eldridge --- .../src/main/scala/firrtl/ir/Serializer.scala | 50 +++++++----- .../scala/firrtlTests/SerializerSpec.scala | 76 +++++++++++++++++++ 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/firrtl/src/main/scala/firrtl/ir/Serializer.scala b/firrtl/src/main/scala/firrtl/ir/Serializer.scala index 0d72805e597..c64ac554e56 100644 --- a/firrtl/src/main/scala/firrtl/ir/Serializer.scala +++ b/firrtl/src/main/scala/firrtl/ir/Serializer.scala @@ -82,15 +82,25 @@ object Serializer { case other => b ++= other.serialize // Handle user-defined nodes } + /** Hash map containing names that were changed due to legalization. */ + private val legalizedNames = scala.collection.mutable.HashMap.empty[String, String] + + /** Generate a legal FIRRTL name. */ + private def legalize(name: String): String = name match { + // If the name starts with a digit, then escape it with backticks. + case _ if name.head.isDigit => legalizedNames.getOrElseUpdate(name, s"`$name`") + case _ => name + } + private def s(str: StringLit)(implicit b: StringBuilder, indent: Int): Unit = b ++= str.serialize private def s(node: Expression)(implicit b: StringBuilder, indent: Int): Unit = node match { - case Reference(name, _) => b ++= name + case Reference(name, _) => b ++= legalize(name) case DoPrim(op, args, consts, _) => b ++= op.toString; b += '('; s(args, ", ", consts.isEmpty); s(consts, ", "); b += ')' case UIntLiteral(value, width) => b ++= "UInt"; s(width); b ++= "(0h"; b ++= value.toString(16); b ++= ")" - case SubField(expr, name, _) => s(expr); b += '.'; b ++= name + case SubField(expr, name, _) => s(expr); b += '.'; b ++= legalize(name) case SubIndex(expr, value, _) => s(expr); b += '['; b ++= value.toString; b += ']' case SubAccess(expr, index, _) => s(expr); b += '['; s(index); b += ']' case Mux(cond, tval, fval, _) => @@ -227,7 +237,7 @@ object Serializer { } private def s(node: Statement)(implicit b: StringBuilder, indent: Int): Unit = node match { - case DefNode(info, name, value) => b ++= "node "; b ++= name; b ++= " = "; s(value); s(info) + case DefNode(info, name, value) => b ++= "node "; b ++= legalize(name); b ++= " = "; s(value); s(info) case Connect(info, loc, expr) => b ++= "connect "; s(loc); b ++= ", "; s(expr); s(info) case c: Conditionally => b ++= sIt(c).mkString case EmptyStmt => b ++= "skip" @@ -240,13 +250,15 @@ object Serializer { if (args.nonEmpty) b ++= ", "; s(args, ", "); b += ')' sStmtName(print.name); s(info) case IsInvalid(info, expr) => b ++= "invalidate "; s(expr); s(info) - case DefWire(info, name, tpe) => b ++= "wire "; b ++= name; b ++= " : "; s(tpe); s(info) + case DefWire(info, name, tpe) => b ++= "wire "; b ++= legalize(name); b ++= " : "; s(tpe); s(info) case DefRegister(info, name, tpe, clock) => - b ++= "reg "; b ++= name; b ++= " : "; s(tpe); b ++= ", "; s(clock); s(info) + b ++= "reg "; b ++= legalize(name); b ++= " : "; s(tpe); b ++= ", "; s(clock); s(info) case DefRegisterWithReset(info, name, tpe, clock, reset, init) => - b ++= "regreset "; b ++= name; b ++= " : "; s(tpe); b ++= ", "; s(clock); b ++= ", "; s(reset); b ++= ", "; + b ++= "regreset "; b ++= legalize(name); b ++= " : "; s(tpe); b ++= ", "; s(clock); b ++= ", "; s(reset); + b ++= ", "; s(init); s(info) - case DefInstance(info, name, module, _) => b ++= "inst "; b ++= name; b ++= " of "; b ++= module; s(info) + case DefInstance(info, name, module, _) => + b ++= "inst "; b ++= legalize(name); b ++= " of "; b ++= legalize(module); s(info) case DefMemory( info, name, @@ -259,14 +271,14 @@ object Serializer { readwriters, readUnderWrite ) => - b ++= "mem "; b ++= name; b ++= " :"; s(info); newLineAndIndent(1) + b ++= "mem "; b ++= legalize(name); b ++= " :"; s(info); newLineAndIndent(1) b ++= "data-type => "; s(dataType); newLineAndIndent(1) b ++= "depth => "; b ++= depth.toString(); newLineAndIndent(1) b ++= "read-latency => "; b ++= readLatency.toString; newLineAndIndent(1) b ++= "write-latency => "; b ++= writeLatency.toString; newLineAndIndent(1) - readers.foreach { r => b ++= "reader => "; b ++= r; newLineAndIndent(1) } - writers.foreach { w => b ++= "writer => "; b ++= w; newLineAndIndent(1) } - readwriters.foreach { r => b ++= "readwriter => "; b ++= r; newLineAndIndent(1) } + readers.foreach { r => b ++= "reader => "; b ++= legalize(r); newLineAndIndent(1) } + writers.foreach { w => b ++= "writer => "; b ++= legalize(w); newLineAndIndent(1) } + readwriters.foreach { r => b ++= "readwriter => "; b ++= legalize(r); newLineAndIndent(1) } b ++= "read-under-write => "; b ++= readUnderWrite.toString case Attach(info, exprs) => // exprs should never be empty since the attach statement takes *at least* two signals according to the spec @@ -278,13 +290,13 @@ 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 += ']' + b ++= legalize(name); b ++= " : "; s(tpe); b ++= " ["; b ++= size.toString(); b += ']' if (readUnderWrite != ReadUnderWrite.Undefined) { // undefined is the default b += ' '; b ++= readUnderWrite.toString } s(info) case firrtl.CDefMPort(info, name, _, mem, exps, direction) => - b ++= direction.serialize; b ++= " mport "; b ++= name; b ++= " = "; b ++= mem + b ++= direction.serialize; b ++= " mport "; b ++= legalize(name); b ++= " = "; b ++= legalize(mem) b += '['; s(exps.head); b ++= "], "; s(exps(1)); s(info) case ProbeDefine(info, sink, probeExpr) => b ++= "define "; s(sink); b ++= " = "; s(probeExpr); s(info) @@ -322,7 +334,7 @@ object Serializer { } private def s(node: Field)(implicit b: StringBuilder, indent: Int): Unit = node match { - case Field(name, flip, tpe) => s(flip); b ++= name; b ++= " : "; s(tpe) + case Field(name, flip, tpe) => s(flip); b ++= legalize(name); b ++= " : "; s(tpe) } private def s(node: Type)(implicit b: StringBuilder, indent: Int): Unit = node match { @@ -350,7 +362,7 @@ object Serializer { private def s(node: Port)(implicit b: StringBuilder, indent: Int): Unit = node match { case Port(info, name, direction, tpe) => - s(direction); b += ' '; b ++= name; b ++= " : "; s(tpe); s(info) + s(direction); b += ' '; b ++= legalize(name); b ++= " : "; s(tpe); s(info) } private def s(node: Param)(implicit b: StringBuilder, indent: Int): Unit = node match { @@ -367,7 +379,7 @@ object Serializer { case Module(info, name, ports, body) => val start = { implicit val b = new StringBuilder - doIndent(0); b ++= "module "; b ++= name; b ++= " :"; s(info) + doIndent(0); b ++= "module "; b ++= legalize(name); b ++= " :"; s(info) ports.foreach { p => newLineAndIndent(1); s(p) } newLineNoIndent() // add a blank line between port declaration and body newLineNoIndent() // newline for body, sIt will indent @@ -376,14 +388,14 @@ object Serializer { Iterator(start) ++ sIt(body)(indent + 1) case ExtModule(info, name, ports, defname, params) => implicit val b = new StringBuilder - doIndent(0); b ++= "extmodule "; b ++= name; b ++= " :"; s(info) + doIndent(0); b ++= "extmodule "; b ++= legalize(name); b ++= " :"; s(info) ports.foreach { p => newLineAndIndent(1); s(p) } newLineAndIndent(1); b ++= "defname = "; b ++= defname params.foreach { p => newLineAndIndent(1); s(p) } Iterator(b.toString) case IntModule(info, name, ports, intrinsic, params) => implicit val b = new StringBuilder - doIndent(0); b ++= "intmodule "; b ++= name; b ++= " :"; s(info) + doIndent(0); b ++= "intmodule "; b ++= legalize(name); b ++= " :"; s(info) ports.foreach { p => newLineAndIndent(1); s(p) } newLineAndIndent(1); b ++= "intrinsic = "; b ++= intrinsic params.foreach { p => newLineAndIndent(1); s(p) } @@ -401,7 +413,7 @@ object Serializer { val prelude = { implicit val b = new StringBuilder b ++= s"FIRRTL version ${version.serialize}\n" - b ++= "circuit "; b ++= circuit.main; b ++= " :"; + b ++= "circuit "; b ++= legalize(circuit.main); b ++= " :"; if (annotations.nonEmpty) { b ++= "%["; b ++= JsonProtocol.serialize(annotations); b ++= "]"; } diff --git a/firrtl/src/test/scala/firrtlTests/SerializerSpec.scala b/firrtl/src/test/scala/firrtlTests/SerializerSpec.scala index 22aee23ec25..7e0df66cd26 100644 --- a/firrtl/src/test/scala/firrtlTests/SerializerSpec.scala +++ b/firrtl/src/test/scala/firrtlTests/SerializerSpec.scala @@ -282,4 +282,80 @@ class SerializerSpec extends AnyFlatSpec with Matchers { "Once we traverse the serializer, everything should execute" ) } + + it should "add backticks to names which begin with a numeric character" in { + info("circuit okay!") + Serializer.serialize(Circuit(NoInfo, Seq.empty[DefModule], "42_Circuit")) should include("circuit `42_Circuit`") + + info("modules okay!") + Serializer.serialize(Module(NoInfo, "42_module", Seq.empty, Block(Seq.empty))) should include("module `42_module`") + // TODO: an external module with a numeric defname should probably be rejected + Serializer.serialize(ExtModule(NoInfo, "42_extmodule", Seq.empty, "", Seq.empty)) should include( + "extmodule `42_extmodule`" + ) + Serializer.serialize(IntModule(NoInfo, "42_intmodule", Seq.empty, "foo", Seq.empty)) should include( + "intmodule `42_intmodule`" + ) + + info("ports okay!") + Serializer.serialize(Port(NoInfo, "42_port", Input, UIntType(IntWidth(1)))) should include("input `42_port`") + + info("types okay!") + Serializer.serialize(BundleType(Seq(Field("42_field", Default, UIntType(IntWidth(1)))))) should include( + "{ `42_field` : UInt<1>}" + ) + + info("declarations okay!") + Serializer.serialize(DefNode(NoInfo, "42_dest", Reference("42_src"))) should include("node `42_dest` = `42_src`") + Serializer.serialize(DefWire(NoInfo, "42_wire", UIntType(IntWidth(1)))) should include("wire `42_wire`") + Serializer.serialize(DefRegister(NoInfo, "42_reg", UIntType(IntWidth(1)), Reference("42_clock"))) should include( + "reg `42_reg` : UInt<1>, `42_clock`" + ) + Serializer.serialize( + DefRegisterWithReset( + NoInfo, + "42_regreset", + UIntType(IntWidth(1)), + Reference("42_clock"), + Reference("42_reset"), + Reference("42_init") + ) + ) should include("regreset `42_regreset` : UInt<1>, `42_clock`, `42_reset`, `42_init`") + Serializer.serialize(DefInstance(NoInfo, "42_inst", "42_module")) should include("inst `42_inst` of `42_module`") + (Serializer + .serialize( + DefMemory( + NoInfo, + "42_mem", + UIntType(IntWidth(1)), + 8, + 1, + 1, + Seq("42_r"), + Seq("42_w"), + Seq("42_rw"), + ReadUnderWrite.Undefined + ) + ) + .split('\n') + .map(_.strip) should contain).allOf( + "mem `42_mem` :", + "reader => `42_r`", + "writer => `42_w`", + "readwriter => `42_rw`" + ) + Serializer.serialize( + CDefMemory(NoInfo, "42_cmem", UIntType(IntWidth(1)), 8, true, ReadUnderWrite.Undefined) + ) should include("smem `42_cmem`") + Serializer.serialize( + firrtl.CDefMPort( + NoInfo, + "42_memport", + UIntType(IntWidth(1)), + "42_mem", + Seq(UIntLiteral(0, IntWidth(1)), Reference("42_clock")), + firrtl.MRead + ) + ) should include("mport `42_memport` = `42_mem`[UInt<1>(0h0)], `42_clock`") + } }