diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 29613d4b61c..ebe25d50ef3 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -974,16 +974,13 @@ abstract class Record extends Aggregate { // of hardware created when connecting to one of these elements private def setElementRefs(): Unit = { val opaqueType = this._isOpaqueType - // Since elements is a map, it is impossible for two elements to have the same - // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog - // which can cause collisions - val _namespace = Namespace.empty require( !opaqueType || (_elements.size == 1 && _elements.head._1 == ""), s"Opaque types must have exactly one element with an empty name, not ${_elements.size}: ${elements.keys.mkString(", ")}" ) + // Names of _elements have already been namespaced (and therefore sanitized) for ((name, elt) <- _elements) { - elt.setRef(this, _namespace.name(name, leadingDigitOk = true), opaque = opaqueType) + elt.setRef(this, name, opaque = opaqueType) } } @@ -1208,15 +1205,23 @@ abstract class Record extends Aggregate { // without having to recurse over all elements after the Record is // constructed. Laziness of _elements means that this check will // occur (only) at the first instance _elements is referenced. - private[chisel3] lazy val _elements: SeqMap[String, Data] = { - for ((name, field) <- elements) { - if (field.binding.isDefined) { - throw RebindingException( - s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware." - ) - } - } - elements + // Also used to sanitize names and convert to more optimized VectorMap datastructure + private[chisel3] lazy val _elements: VectorMap[String, Data] = { + // Since elements is a map, it is impossible for two elements to have the same + // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog + // which can cause collisions + val namespace = Namespace.empty + elements.view.map { + case (name, field) => + if (field.binding.isDefined) { + throw RebindingException( + s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware." + ) + } + // namespace.name also sanitizes for firrtl + val sanitizedName = namespace.name(name, leadingDigitOk = true) + sanitizedName -> field + }.to(VectorMap) // VectorMap has O(1) lookup whereas ListMap is O(n) } /** Name for Pretty Printing */ diff --git a/src/test/scala/chiselTests/PrintableSpec.scala b/src/test/scala/chiselTests/PrintableSpec.scala index 32d8be09b2a..505d4287489 100644 --- a/src/test/scala/chiselTests/PrintableSpec.scala +++ b/src/test/scala/chiselTests/PrintableSpec.scala @@ -177,6 +177,20 @@ class PrintableSpec extends AnyFlatSpec with Matchers with Utils { } } + it should "print use the sanitized names of Bundle elements" in { + class MyModule extends Module { + class UnsanitaryBundle extends Bundle { + val `a-x` = UInt(8.W) + } + val myBun = Wire(new UnsanitaryBundle) + myBun.`a-x` := 0.U + printf(p"$myBun") + } + generateAndCheck(new MyModule) { + case Seq(Printf("UnsanitaryBundle(aminusx -> %d)", Seq("myBun.aminusx"))) => + } + } + // Unit tests for cf it should "print regular scala variables with cf format specifier" in { diff --git a/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala b/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala index 27725c4979b..64cba72fbe1 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala @@ -294,6 +294,17 @@ object Examples { @public val nested = new NestedInstantiable(in = leafIn, out = leafOut) } + @instantiable + class HasUnsanitaryBundleField extends Module { + class Interface extends Bundle { + val `a-x` = UInt(8.W) + } + val realIn = IO(Input(new Interface)) + // It's important to have this redirection to trip an old bug + @public val in = realIn.`a-x` + @public val out = IO(Output(UInt(8.W))) + out := in + } class AddTwoNestedInstantiableData(width: Int) extends Module { val in = IO(Input(UInt(width.W))) diff --git a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala index 6957ce7248e..169f36fdc28 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala @@ -43,7 +43,7 @@ class InstanceSpec extends ChiselFunSpec with Utils { val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top) chirrtl should include("inst i0 of AddOne") } - it("0.3: BlackBoxes should be supported") { + it("(0.d): BlackBoxes should be supported") { class Top extends Module { val in = IO(Input(UInt(32.W))) val out = IO(Output(UInt(32.W))) @@ -66,6 +66,15 @@ class InstanceSpec extends ChiselFunSpec with Utils { chirrtl should include("connect i1.in, io.in") chirrtl should include("connect io.out, i1.out") } + it("(0.e): Instances with Bundles with unsanitary names should be supported") { + class Top extends Module { + val definition = Definition(new HasUnsanitaryBundleField) + val i0 = Instance(definition) + i0.in := 123.U + } + val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top) + chirrtl should include("connect i0.realIn.aminusx, UInt<7>(0h7b)") + } } describe("(1) Annotations on instances in same chisel compilation") { it("(1.a): should work on a single instance, annotating the instance") {