diff options
| author | mergify[bot] | 2022-06-16 23:15:42 +0000 |
|---|---|---|
| committer | GitHub | 2022-06-16 23:15:42 +0000 |
| commit | d001b34f816f1f65d0625aebf33e5cfc5ba93e49 (patch) | |
| tree | 7145978904fea41e4a61a14ff3b08d3b243cf951 | |
| parent | 13641d95951b189d7f5b0d4d99ace45f8b8f6282 (diff) | |
Define leading '_' as API for creating temporaries (backport #2580) (#2581)
* Define leading '_' as API for creating temporaries
Chisel and FIRRTL have long used signals with names beginning with an
underscore as an API to specify that the name does not really matter.
Tools like Verilator follow a similar convention and exclude signals
with underscore names from waveform dumps by default. With the
introduction of compiler-plugin prefixing in Chisel 3.4, the convention
remained but was hard for users to use unless the unnnamed signal
existed outside of any prefix domain. Notably, unnamed signals are most
useful when creating wires inside of utility methods which almost always
results in the signal ending up with a prefix.
With this commit, Chisel explicitly recognizes signals whos val names
start with an underscore and preserve that underscore regardless of any
prefixing. Chisel will also ignore such underscores when generating
prefixes based on the temporary signal, preventing accidental double
underscores in the names of signals that are prefixed by the temporary.
(cherry picked from commit bd94366290886f3489d58f88b9768c7c11fa2cb6)
* Remove unused defaultPrefix argument from _computeName
(cherry picked from commit ec178aa20a830df2c8c756b9e569709a59073554)
# Conflicts:
# core/src/main/scala/chisel3/Module.scala
# core/src/main/scala/chisel3/experimental/hierarchy/ModuleClone.scala
* Resolve backport conflicts
* Waive false positive binary compatibility errors
Co-authored-by: Jack Koenig <koenig@sifive.com>
| -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")) + } + } } |
