diff options
| -rw-r--r-- | build.sbt | 15 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/Module.scala | 4 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/RawModule.scala | 57 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/experimental/dataview/package.scala | 2 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala | 2 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/internal/Builder.scala | 30 | ||||
| -rw-r--r-- | core/src/main/scala/chisel3/internal/firrtl/IR.scala | 2 | ||||
| -rw-r--r-- | docs/src/explanations/naming.md | 121 | ||||
| -rw-r--r-- | plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala | 5 | ||||
| -rw-r--r-- | src/test/scala/chiselTests/naming/NamePluginSpec.scala | 19 | ||||
| -rw-r--r-- | src/test/scala/chiselTests/naming/PrefixSpec.scala | 93 |
11 files changed, 299 insertions, 51 deletions
@@ -170,7 +170,20 @@ lazy val core = (project in file("core")). buildInfoKeys := Seq[BuildInfoKey](buildInfoPackage, version, scalaVersion, sbtVersion) ). settings(publishSettings: _*). - settings(mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3-core" % "3.5.3")). + settings( + mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3-core" % "3.5.3"), + mimaBinaryIssueFilters ++= Seq( + // Modified package private methods (https://github.com/lightbend/mima/issues/53) + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.Data._computeName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.Data.forceName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.MemBase._computeName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.MemBase.forceName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.VerificationStatement._computeName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.VerificationStatement.forceName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.experimental.BaseModule._computeName"), + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.experimental.BaseModule.forceName"), + ) + ). settings( name := "chisel3-core", scalacOptions := scalacOptions.value ++ Seq( diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala index d03122f9..ba2d2e32 100644 --- a/core/src/main/scala/chisel3/Module.scala +++ b/core/src/main/scala/chisel3/Module.scala @@ -275,7 +275,7 @@ package internal { private[chisel3] def setRefAndPortsRef(namespace: Namespace): Unit = { val record = _portsRecord // Use .forceName to re-use default name resolving behavior - record.forceName(None, default = this.desiredName, namespace) + record.forceName(default = this.desiredName, namespace) // Now take the Ref that forceName set and convert it to the correct Arg val instName = record.getRef match { case Ref(name) => name @@ -497,7 +497,7 @@ package experimental { private[chisel3] def namePorts(names: HashMap[HasId, String]): Unit = { for (port <- getModulePorts) { - port._computeName(None, None).orElse(names.get(port)) match { + port._computeName(None).orElse(names.get(port)) match { case Some(name) => if (_namespace.contains(name)) { Builder.error( diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala index bd04fdc4..f2ce4c70 100644 --- a/core/src/main/scala/chisel3/RawModule.scala +++ b/core/src/main/scala/chisel3/RawModule.scala @@ -94,26 +94,26 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends id match { case id: ModuleClone[_] => id.setRefAndPortsRef(_namespace) // special handling case id: InstanceClone[_] => id.setAsInstanceRef() - case id: BaseModule => id.forceName(None, default = id.desiredName, _namespace) - case id: MemBase[_] => id.forceName(None, default = "MEM", _namespace) - case id: stop.Stop => id.forceName(None, default = "stop", _namespace) - case id: assert.Assert => id.forceName(None, default = "assert", _namespace) - case id: assume.Assume => id.forceName(None, default = "assume", _namespace) - case id: cover.Cover => id.forceName(None, default = "cover", _namespace) - case id: printf.Printf => id.forceName(None, default = "printf", _namespace) + case id: BaseModule => id.forceName(default = id.desiredName, _namespace) + case id: MemBase[_] => id.forceName(default = "MEM", _namespace) + case id: stop.Stop => id.forceName(default = "stop", _namespace) + case id: assert.Assert => id.forceName(default = "assert", _namespace) + case id: assume.Assume => id.forceName(default = "assume", _namespace) + case id: cover.Cover => id.forceName(default = "cover", _namespace) + case id: printf.Printf => id.forceName(default = "printf", _namespace) case id: Data => if (id.isSynthesizable) { id.topBinding match { case OpBinding(_, _) => - id.forceName(Some(""), default = "T", _namespace) + id.forceName(default = "_T", _namespace) case MemoryPortBinding(_, _) => - id.forceName(None, default = "MPORT", _namespace) + id.forceName(default = "MPORT", _namespace) case PortBinding(_) => - id.forceName(None, default = "PORT", _namespace) + id.forceName(default = "PORT", _namespace) case RegBinding(_, _) => - id.forceName(None, default = "REG", _namespace) + id.forceName(default = "REG", _namespace) case WireBinding(_, _) => - id.forceName(Some(""), default = "WIRE", _namespace) + id.forceName(default = "_WIRE", _namespace) case _ => // don't name literals } } // else, don't name unbound types @@ -189,18 +189,39 @@ package object internal { /** Marker trait for modules that are not true modules */ private[chisel3] trait PseudoModule extends BaseModule + /* Check if a String name is a temporary name */ + def isTemp(name: String): Boolean = name.nonEmpty && name.head == '_' + /** Creates a name String from a prefix and a seed * @param prefix The prefix associated with the seed (must be in correct order, *not* reversed) * @param seed The seed for computing the name (if available) */ def buildName(seed: String, prefix: Prefix): String = { - val builder = new StringBuilder() - prefix.foreach { p => - builder ++= p - builder += '_' + // Don't bother copying the String if there's no prefix + if (prefix.isEmpty) { + seed + } else { + // Using Java's String builder to micro-optimize appending a String excluding 1st character + // for temporaries + val builder = new java.lang.StringBuilder() + // Starting with _ is the indicator of a temporary + val temp = isTemp(seed) + // Make sure the final result is also a temporary if this is a temporary + if (temp) { + builder.append('_') + } + prefix.foreach { p => + builder.append(p) + builder.append('_') + } + if (temp) { + // We've moved the leading _ to the front, drop it here + builder.append(seed, 1, seed.length) + } else { + builder.append(seed) + } + builder.toString } - builder ++= seed - builder.toString } // Private reflective version of "val io" to maintain Chisel.Module semantics without having diff --git a/core/src/main/scala/chisel3/experimental/dataview/package.scala b/core/src/main/scala/chisel3/experimental/dataview/package.scala index c583c516..71ae2d8f 100644 --- a/core/src/main/scala/chisel3/experimental/dataview/package.scala +++ b/core/src/main/scala/chisel3/experimental/dataview/package.scala @@ -33,7 +33,7 @@ package object dataview { // The names of views do not matter except for when a view is annotated. For Views that correspond // To a single Data, we just forward the name of the Target. For Views that correspond to more // than one Data, we return this assigned name but rename it in the Convert stage - result.forceName(None, "view", Builder.viewNamespace) + result.forceName("view", Builder.viewNamespace) result } } diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala index 46a38e7c..c83479b0 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Lookupable.scala @@ -218,7 +218,7 @@ object Lookupable { result.bind(newBinding) result.setAllParents(Some(ViewParent)) - result.forceName(None, "view", Builder.viewNamespace) + result.forceName("view", Builder.viewNamespace) result } diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 6f02b57c..6fd9bdd5 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -177,21 +177,13 @@ private[chisel3] trait HasId extends InstanceId { } /** Computes the name of this HasId, if one exists - * @param defaultPrefix Optionally provide a default prefix for computing the name * @param defaultSeed Optionally provide default seed for computing the name * @return the name, if it can be computed */ - private[chisel3] def _computeName(defaultPrefix: Option[String], defaultSeed: Option[String]): Option[String] = { - if (hasSeed) { - Some(buildName(seedOpt.get, naming_prefix.reverse)) - } else { - defaultSeed.map { default => - defaultPrefix match { - case Some(p) => buildName(default, p :: naming_prefix.reverse) - case None => buildName(default, naming_prefix.reverse) - } - } - } + private[chisel3] def _computeName(defaultSeed: Option[String]): Option[String] = { + seedOpt + .orElse(defaultSeed) + .map(name => buildName(name, naming_prefix.reverse)) } /** This resolves the precedence of [[autoSeed]] and [[suggestName]] @@ -211,9 +203,9 @@ private[chisel3] trait HasId extends InstanceId { // Uses a namespace to convert suggestion into a true name // Will not do any naming if the reference already assigned. // (e.g. tried to suggest a name to part of a Record) - private[chisel3] def forceName(prefix: Option[String], default: => String, namespace: Namespace): Unit = + private[chisel3] def forceName(default: => String, namespace: Namespace): Unit = if (_ref.isEmpty) { - val candidate_name = _computeName(prefix, Some(default)).get + val candidate_name = _computeName(Some(default)).get val available_name = namespace.name(candidate_name) setRef(Ref(available_name)) // Clear naming prefix to free memory @@ -246,7 +238,7 @@ private[chisel3] trait HasId extends InstanceId { "This will become an error in Chisel 3.6." errors.deprecated(msg, None) errors.checkpoint(logger) - _computeName(None, None).get + _computeName(None).get } // Helper for reifying views if they map to a single Target @@ -506,7 +498,11 @@ private[chisel3] object Builder extends LazyLogging { } } buildAggName(d).map { name => - pushPrefix(name) + if (isTemp(name)) { + pushPrefix(name.tail) + } else { + pushPrefix(name) + } }.isDefined } @@ -749,7 +745,7 @@ private[chisel3] object Builder extends LazyLogging { logger.info("Elaborating design...") val mod = f if (forceModName) { // This avoids definition name index skipping with D/I - mod.forceName(None, mod.name, globalNamespace) + mod.forceName(mod.name, globalNamespace) } errors.checkpoint(logger) logger.info("Done elaborating.") diff --git a/core/src/main/scala/chisel3/internal/firrtl/IR.scala b/core/src/main/scala/chisel3/internal/firrtl/IR.scala index 1ee8842f..9327c29e 100644 --- a/core/src/main/scala/chisel3/internal/firrtl/IR.scala +++ b/core/src/main/scala/chisel3/internal/firrtl/IR.scala @@ -94,7 +94,7 @@ object Arg { case Some(arg) => arg.name case None => id match { - case data: Data => data._computeName(None, Some("?")).get + case data: Data => data._computeName(Some("?")).get case _ => "?" } } diff --git a/docs/src/explanations/naming.md b/docs/src/explanations/naming.md index a9f21936..99797cb2 100644 --- a/docs/src/explanations/naming.md +++ b/docs/src/explanations/naming.md @@ -24,11 +24,11 @@ import chisel3.experimental.{prefix, noPrefix} import chisel3.stage.ChiselStage ``` -With the release of Chisel 3.4, users should add the following line to their build.sbt settings to get the improved -naming: +With the release of Chisel 3.5, users are required to add the following line to +their build.sbt settings: ```scala -// chiselVersion is the String version (eg. "3.4.0") +// chiselVersion is the String version (eg. "3.5.3") addCompilerPlugin("edu.berkeley.cs" % "chisel3-plugin" % chiselVersion cross CrossVersion.full) ``` @@ -78,6 +78,29 @@ class Example2 extends Module { ChiselStage.emitVerilog(new Example2) ``` +Prefixing can also be derived from the name of signals on the left-hand side of a connection. +While this is not implemented via the compiler plugin, the behavior should feel similar: + +```scala mdoc +class ConnectPrefixing extends Module { + val in = IO(Input(UInt(2.W))) + // val in = autoNameRecursively("in")(prefix("in")(IO(Input(UInt(2.W))))) + + val out = IO(Output(UInt(2.W))) + // val out = autoNameRecursively("out")(prefix("out")(IO(Output(UInt(2.W))))) + + out := { // technically this is not wrapped in autoNameRecursively nor prefix + // But the Chisel runtime will still use the name of `out` as a prefix + val double = in * in + // val double = autoNameRecursively("double")(prefix("double")(in * in)) + double + 1.U + } +} +``` +```scala mdoc:verilog +ChiselStage.emitVerilog(new ConnectPrefixing) +``` + Note that the naming also works if the hardware type is nested in an `Option` or a subtype of `Iterable`: ```scala mdoc @@ -121,7 +144,7 @@ Users who desire a prefix are encouraged to provide one as [described below](#pr ### Prefixing As shown above, the compiler plugin automatically attempts to prefix some of your signals for you. However, you as a -user can also add your own prefixes. This is especially for ECO-type fixes where you need to add some logic to a module +user can also add your own prefixes. This is especially useful for ECO-type fixes where you need to add some logic to a module but don't want to influence other names in the module. In the following example, we prefix additional logic with "ECO", where `Example4` is pre-ECO and `Example5` is post-ECO: @@ -202,6 +225,96 @@ class Example8 extends Module { ChiselStage.emitVerilog(new Example8) ``` +Note that using `.suggestName` does **not** affect prefixes derived from val names; +however, it _can_ affect prefixes derived from connections (eg. `:=`): + +```scala mdoc +class ConnectionPrefixExample extends Module { + val in0 = IO(Input(UInt(2.W))) + val in1 = IO(Input(UInt(2.W))) + + val out0 = { + val port = IO(Output(UInt())) + // Even though this suggestName is before mul, the prefix used in this scope + // is derived from `val out0`, so this does not affect the name of mul + port.suggestName("foo") + // out0_mul + val mul = in0 * in1 + port := mul + 1.U + port + } + + val out1 = IO(Output(UInt())) + val out2 = IO(Output(UInt())) + + out1 := { + // out1_sum + val sum = in0 + in1 + sum + 1.U + } + // Comes after so does *not* affect prefix above + out1.suggestName("bar") + + // Comes before so *does* affect prefix below + out2.suggestName("fizz") + out2 := { + // fizz_diff + val diff = in0 - in1 + diff + 1.U + } +} +``` +```scala mdoc:verilog +ChiselStage.emitVerilog(new ConnectionPrefixExample) +``` + +As this example illustrates, this behavior is slightly inconsistent so is subject to change in a future version of Chisel. + + +### Behavior for "Unnamed signals" (aka "Temporaries") + +If you want to signify that the name of a signal does not matter, you can prefix the name of your val with `_`. +Chisel will preserve the convention of leading `_` signifying an unnamed signal across prefixes. +For example: + +```scala mdoc +class TemporaryExample extends Module { + val in0 = IO(Input(UInt(2.W))) + val in1 = IO(Input(UInt(2.W))) + + val out = { + val port = IO(Output(UInt())) + val _sum = in0 + in1 + port := _sum + 1.U + port + } +} +``` +```scala mdoc:verilog +ChiselStage.emitVerilog(new TemporaryExample) +``` + +If an unnamed signal is itself used to generate a prefix, the leading `_` will be ignored to avoid double `__` in the names of further nested signals. + + +```scala mdoc +class TemporaryPrefixExample extends Module { + val in0 = IO(Input(UInt(2.W))) + val in1 = IO(Input(UInt(2.W))) + val out = IO(Output(UInt())) + + val _sum = { + val x = in0 + in1 + x + 1.U + } + out := _sum & 0x2.U +} +``` +```scala mdoc:verilog +ChiselStage.emitVerilog(new TemporaryPrefixExample) +``` + + ### Set a Module Name If you want to specify the module's name (not the instance name of a module), you can always override the `desiredName` diff --git a/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala index f98049e2..b3bbdbe4 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala @@ -181,8 +181,11 @@ class ChiselComponent(val global: Global, arguments: ChiselPluginArguments) // If a Data or a Memory, get the name and a prefix else if (shouldMatchNamedComp(tpe)) { val str = stringFromTermName(name) + // Starting with '_' signifies a temporary, we ignore it for prefixing because we don't + // want double "__" in names when the user is just specifying a temporary + val prefix = if (str.head == '_') str.tail else str val newRHS = transform(rhs) - val prefixed = q"chisel3.experimental.prefix.apply[$tpt](name=$str)(f=$newRHS)" + val prefixed = q"chisel3.experimental.prefix.apply[$tpt](name=$prefix)(f=$newRHS)" val named = q"chisel3.internal.plugin.autoNameRecursively($str)($prefixed)" treeCopy.ValDef(dd, mods, name, tpt, localTyper.typed(named)) // If an instance, just get a name but no prefix diff --git a/src/test/scala/chiselTests/naming/NamePluginSpec.scala b/src/test/scala/chiselTests/naming/NamePluginSpec.scala index 482ef62b..a787bb80 100644 --- a/src/test/scala/chiselTests/naming/NamePluginSpec.scala +++ b/src/test/scala/chiselTests/naming/NamePluginSpec.scala @@ -340,4 +340,23 @@ class NamePluginSpec extends ChiselFlatSpec with Utils { Select.wires(top).map(_.instanceName) should be(List("a_b_c", "a_b", "a")) } } + + behavior.of("Unnamed values (aka \"Temporaries\")") + + they should "be declared by starting the name with '_'" in { + class Test extends Module { + { + val a = { + val b = { + val _c = Wire(UInt(3.W)) + 4.U // literal so there is no name + } + b + } + } + } + aspectTest(() => new Test) { top: Test => + Select.wires(top).map(_.instanceName) should be(List("_a_b_c")) + } + } } diff --git a/src/test/scala/chiselTests/naming/PrefixSpec.scala b/src/test/scala/chiselTests/naming/PrefixSpec.scala index 1e628391..6d52407e 100644 --- a/src/test/scala/chiselTests/naming/PrefixSpec.scala +++ b/src/test/scala/chiselTests/naming/PrefixSpec.scala @@ -233,18 +233,46 @@ class PrefixSpec extends ChiselPropSpec with Utils { } } - property("Prefixing should be the prefix during the last call to autoName/suggestName") { + property("Prefixing should NOT be influenced by suggestName") { class Test extends Module { { val wire = { - val x = Wire(UInt(3.W)).suggestName("mywire") - x + val x = Wire(UInt(3.W)) // wire_x + Wire(UInt(3.W)).suggestName("foo") + } + } + } + aspectTest(() => new Test) { top: Test => + Select.wires(top).map(_.instanceName) should be(List("wire_x", "foo")) + } + } + + property("Prefixing should be influenced by the \"current name\" of the signal") { + class Test extends Module { + { + val wire = { + val y = Wire(UInt(3.W)).suggestName("foo") + val x = Wire(UInt(3.W)) // wire_x + y + } + + val wire2 = Wire(UInt(3.W)) + wire2 := { + val x = Wire(UInt(3.W)) // wire2_x + x + 1.U + } + wire2.suggestName("bar") + + val wire3 = Wire(UInt(3.W)) + wire3.suggestName("fizz") + wire3 := { + val x = Wire(UInt(3.W)) // fizz_x + x + 1.U } } } aspectTest(() => new Test) { top: Test => - Select.wires(top).map(_.instanceName) should be(List("mywire")) - Select.wires(top).map(_.instanceName) shouldNot be(List("wire_mywire")) + Select.wires(top).map(_.instanceName) should be(List("foo", "wire_x", "bar", "wire2_x", "fizz", "fizz_x")) } } @@ -414,4 +442,59 @@ class PrefixSpec extends ChiselPropSpec with Utils { (chirrtl should include).regex("assume.*: x5_x3") (chirrtl should include).regex("printf.*: x5_x4") } + + property("Leading '_' in val names should be ignored in prefixes") { + class Test extends Module { + { + val a = { + val _b = { + val c = Wire(UInt(3.W)) + 4.U // literal because there is no name + } + _b + } + } + } + aspectTest(() => new Test) { top: Test => + Select.wires(top).map(_.instanceName) should be(List("a_b_c")) + } + } + + // This checks that we don't just blanket ignore leading _ in prefixes + property("User-specified prefixes with '_' should be respected") { + class Test extends Module { + { + val a = { + val _b = prefix("_b") { + val c = Wire(UInt(3.W)) + } + 4.U + } + } + } + aspectTest(() => new Test) { top: Test => + Select.wires(top).map(_.instanceName) should be(List("a__b_c")) + } + } + + property("Leading '_' in signal names should be ignored in prefixes from connections") { + class Test extends Module { + { + val a = { + val b = { + val _c = IO(Output(UInt(3.W))) // port so not selected as wire + _c := { + val d = Wire(UInt(3.W)) + d + } + 4.U // literal so there is no name + } + b + } + } + } + aspectTest(() => new Test) { top: Test => + Select.wires(top).map(_.instanceName) should be(List("a_b_c_d")) + } + } } |