From 1260f7c89f1b95bdb00e56e49edb73dc2eac3a0e Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Tue, 10 Nov 2020 18:40:51 -0800 Subject: Refine autonaming to have more intuitive behavior (#1660) * Refine autonaming to have more intuitive behavior Last name in an Expression wins, while the first Statement to name wins. This is done via checking the _id of HasIds during autonaming and only applying a name if the HasId was created in the scope of autonaming. There is no change to .autoSeed or .suggestName behavior. Behavior of chisel3-plugins from before this change is maintained. * Update docs with naming plugin changes--- core/src/main/scala/chisel3/internal/Builder.scala | 5 +- .../scala/chisel3/internal/plugin/package.scala | 51 +++++++++ docs/src/cookbooks/naming.md | 2 +- docs/src/explanations/naming.md | 33 ++++-- .../chisel3/internal/plugin/ChiselPlugin.scala | 85 ++++++++++++--- .../scala/chiselTests/naming/NamePluginSpec.scala | 117 ++++++++++++++++++++- 6 files changed, 269 insertions(+), 24 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 56a85fb6..d665b7bc 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -59,6 +59,7 @@ private[chisel3] class IdGen { counter += 1 counter } + def value: Long = counter } /** Public API to access Node/Signal names. @@ -119,7 +120,9 @@ private[chisel3] trait HasId extends InstanceId { * @param seed Seed for the name of this component * @return this object */ - private [chisel3] def autoSeed(seed: String): this.type = { + private[chisel3] def autoSeed(seed: String): this.type = forceAutoSeed(seed) + // Bypass the overridden behavior of autoSeed in [[Data]], apply autoSeed even to ports + private[chisel3] def forceAutoSeed(seed: String): this.type = { auto_seed = Some(seed) for(hook <- auto_postseed_hooks) { hook(seed) } prefix_seed = Builder.getPrefix() diff --git a/core/src/main/scala/chisel3/internal/plugin/package.scala b/core/src/main/scala/chisel3/internal/plugin/package.scala index dd091ccc..c17baf22 100644 --- a/core/src/main/scala/chisel3/internal/plugin/package.scala +++ b/core/src/main/scala/chisel3/internal/plugin/package.scala @@ -10,6 +10,7 @@ package object plugin { * @param nameMe The thing to be named * @tparam T The type of the thing to be named * @return The thing, but now named + * @note This is the version called by chisel3-plugin prior to v3.4.1 */ def autoNameRecursively[T <: Any](name: String, nameMe: T): T = { chisel3.internal.Builder.nameRecursively( @@ -19,4 +20,54 @@ package object plugin { ) nameMe } + + // The actual implementation + // Cannot be unified with (String, T) => T (v3.4.0 version) because of special behavior of ports + // in .autoSeed + private def _autoNameRecursively[T <: Any](prevId: Long, name: String, nameMe: T): T = { + chisel3.internal.Builder.nameRecursively( + name, + nameMe, + (id: chisel3.internal.HasId, n: String) => { + // Name override only if result was created in this scope + if (id._id > prevId) { + id.forceAutoSeed(n) + } + } + ) + nameMe + } + + /** Used by Chisel's compiler plugin to automatically name signals + * DO NOT USE in your normal Chisel code!!! + * + * @param name The name to use + * @param nameMe The thing to be named + * @tparam T The type of the thing to be named + * @return The thing, but now named + */ + def autoNameRecursively[T <: Any](name: String)(nameMe: => T): T = { + // The _id of the most recently constructed HasId + val prevId = Builder.idGen.value + val result = nameMe + _autoNameRecursively(prevId, name, result) + } + + /** Used by Chisel's compiler plugin to automatically name signals + * DO NOT USE in your normal Chisel code!!! + * + * @param names The names to use corresponding to interesting fields of the Product + * @param nameMe The [[Product]] to be named + * @tparam T The type of the thing to be named + * @return The thing, but with each member named + */ + def autoNameRecursivelyProduct[T <: Product](names: List[Option[String]])(nameMe: => T): T = { + // The _id of the most recently constructed HasId + val prevId = Builder.idGen.value + val result = nameMe + for ((name, t) <- names.iterator.zip(result.productIterator) if name.nonEmpty) { + _autoNameRecursively(prevId, name.get, t) + } + result + } } diff --git a/docs/src/cookbooks/naming.md b/docs/src/cookbooks/naming.md index 150f0639..4caabaff 100644 --- a/docs/src/cookbooks/naming.md +++ b/docs/src/cookbooks/naming.md @@ -42,7 +42,7 @@ name collisions, which are what triggers all those annoying signal name bumps! ### I want to force a signal (or instance) name to something, how do I do that? -Use the `.suggestName` method, which is on all classes which subtype 'Data'. +Use the `.suggestName` method, which is on all classes which subtype `Data`. ### All this prefixing is annoying, how do I fix it? diff --git a/docs/src/explanations/naming.md b/docs/src/explanations/naming.md index 86db515c..a626b878 100644 --- a/docs/src/explanations/naming.md +++ b/docs/src/explanations/naming.md @@ -42,11 +42,11 @@ side with a call to `autoNameRecursively`, which names the signal/module. ```scala mdoc class MyBundle extends Bundle { val foo = Input(UInt(3.W)) - // val foo = autoNameRecursively("foo", Input(UInt(3.W))) + // val foo = autoNameRecursively("foo")(Input(UInt(3.W))) } class Example1 extends MultiIOModule { val io = IO(new MyBundle()) - // val io = autoNameRecursively("io", IO(new MyBundle())) + // val io = autoNameRecursively("io")(IO(new MyBundle())) } println(ChiselStage.emitVerilog(new Example1)) ``` @@ -57,15 +57,15 @@ side of the val declaration: ```scala mdoc class Example2 extends MultiIOModule { val in = IO(Input(UInt(2.W))) - // val in = autoNameRecursively("in", prefix("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))))) + // val out = autoNameRecursively("out")(prefix("out")(IO(Output(UInt(2.W))))) def inXin() = in * in val add = 3.U + inXin() - // val add = autoNameRecursively("add", prefix("add")(3.U + inXin())) + // val add = autoNameRecursively("add")(prefix("add")(3.U + inXin())) // Note that the intermediate result of the multiplication is prefixed with `add` out := add + 1.U @@ -79,16 +79,16 @@ Note that the naming also works if the hardware type is nested in an `Option` or ```scala mdoc class Example3 extends MultiIOModule { val in = IO(Input(UInt(2.W))) - // val in = autoNameRecursively("in", prefix("in")(IO(Input(UInt(2.W))))) + // val in = autoNameRecursively("in")(prefix("in")(IO(Input(UInt(2.W))))) val out = IO(Output(UInt())) - // val out = autoNameRecursively("out", prefix("out")(IO(Output(UInt(2.W))))) + // val out = autoNameRecursively("out")(prefix("out")(IO(Output(UInt(2.W))))) def inXin() = in * in val opt = Some(3.U + inXin()) // Note that the intermediate result of the inXin() is prefixed with `opt`: - // val opt = autoNameRecursively("opt", prefix("opt")(Some(3.U + inXin()))) + // val opt = autoNameRecursively("opt")(prefix("opt")(Some(3.U + inXin()))) out := opt.get + 1.U } @@ -96,6 +96,21 @@ class Example3 extends MultiIOModule { println(ChiselStage.emitVerilog(new Example3)) ``` +There is also a slight variant (`autoNameRecursivelyProduct`) for naming hardware with names provided by an unapply: +```scala mdoc +class UnapplyExample extends MultiIOModule { + def mkIO() = (IO(Input(UInt(2.W))), IO(Output(UInt()))) + val (in, out) = mkIO() + // val (in, out) = autoNameRecursivelyProduct(List(Some("in"), Some("out")))(mkIO()) + + out := in +} + +println(ChiselStage.emitVerilog(new UnapplyExample)) +``` +Note that the compiler plugin will not insert a prefix in these cases because it is ambiguous what the prefix should be. +Users who desire a prefix are encouraged to provide one as [described below](#prefixing). + ### Prefixing As shown above, the compiler plugin automatically attempts to prefix some of your signals for you. However, you as a @@ -226,4 +241,4 @@ class Example10 extends MultiIOModule { ### @chiselName This macro is no longer recommended as its functionality is entirely replaced by the compiler plugin. Feel free to -delete from your Chisel designs! \ No newline at end of file +delete from your Chisel designs! diff --git a/plugin/src/main/scala-2.12/chisel3/internal/plugin/ChiselPlugin.scala b/plugin/src/main/scala-2.12/chisel3/internal/plugin/ChiselPlugin.scala index 9f90ba16..59be7588 100644 --- a/plugin/src/main/scala-2.12/chisel3/internal/plugin/ChiselPlugin.scala +++ b/plugin/src/main/scala-2.12/chisel3/internal/plugin/ChiselPlugin.scala @@ -39,7 +39,7 @@ class ChiselComponent(val global: Global) extends PluginComponent with TypingTra class MyTypingTransformer(unit: CompilationUnit) extends TypingTransformer(unit) { - private def shouldMatchGen(bases: Tree*): ValDef => Boolean = { + private def shouldMatchGen(bases: Tree*): Type => Boolean = { val cache = mutable.HashMap.empty[Type, Boolean] val baseTypes = bases.map(inferType) @@ -72,8 +72,7 @@ class ChiselComponent(val global: Global) extends PluginComponent with TypingTra } // Return function so that it captures the cache - { p: ValDef => - val q = inferType(p.tpt) + { q: Type => cache.getOrElseUpdate(q, { // First check if a match, then check early exit, then recurse if(terminate(q)){ @@ -87,9 +86,10 @@ class ChiselComponent(val global: Global) extends PluginComponent with TypingTra } } - private val shouldMatchData : ValDef => Boolean = shouldMatchGen(tq"chisel3.Data") - private val shouldMatchDataOrMem : ValDef => Boolean = shouldMatchGen(tq"chisel3.Data", tq"chisel3.MemBase[_]") - private val shouldMatchModule : ValDef => Boolean = shouldMatchGen(tq"chisel3.experimental.BaseModule") + + private val shouldMatchData : Type => Boolean = shouldMatchGen(tq"chisel3.Data") + private val shouldMatchDataOrMem : Type => Boolean = shouldMatchGen(tq"chisel3.Data", tq"chisel3.MemBase[_]") + private val shouldMatchModule : Type => Boolean = shouldMatchGen(tq"chisel3.experimental.BaseModule") // Given a type tree, infer the type and return it private def inferType(t: Tree): Type = localTyper.typed(t, nsc.Mode.TYPEmode).tpe @@ -118,6 +118,50 @@ class ChiselComponent(val global: Global) extends PluginComponent with TypingTra okFlags(dd.mods) && !isNull && dd.rhs != EmptyTree } + // TODO Unify with okVal + private def okUnapply(dd: ValDef): Boolean = { + + // These were found through trial and error + def okFlags(mods: Modifiers): Boolean = { + val badFlags = Set( + Flag.PARAM, + Flag.DEFERRED, + Flags.TRIEDCOOKING, + Flags.CASEACCESSOR, + Flags.PARAMACCESSOR + ) + val goodFlags = Set( + Flag.SYNTHETIC, + Flag.ARTIFACT + ) + goodFlags.forall(f => mods.hasFlag(f)) && badFlags.forall(f => !mods.hasFlag(f)) + } + + // Ensure expression isn't null, as you can't call `null.autoName("myname")` + val isNull = dd.rhs match { + case Literal(Constant(null)) => true + case _ => false + } + val tpe = inferType(dd.tpt) + definitions.isTupleType(tpe) && okFlags(dd.mods) && !isNull && dd.rhs != EmptyTree + } + + private def findUnapplyNames(tree: Tree): Option[List[String]] = { + val applyArgs: Option[List[Tree]] = tree match { + case Match(_, List(CaseDef(_, _, Apply(_, args)))) => Some(args) + case _ => None + } + applyArgs.flatMap { args => + var ok = true + val result = mutable.ListBuffer[String]() + args.foreach { + case Ident(TermName(name)) => result += name + // Anything unexpected and we abort + case _ => ok = false + } + if (ok) Some(result.toList) else None + } + } // Whether this val is directly enclosed by a Bundle type private def inBundle(dd: ValDef): Boolean = { @@ -131,30 +175,47 @@ class ChiselComponent(val global: Global) extends PluginComponent with TypingTra override def transform(tree: Tree): Tree = tree match { // Check if a subtree is a candidate case dd @ ValDef(mods, name, tpt, rhs) if okVal(dd) => + val tpe = inferType(tpt) // If a Data and in a Bundle, just get the name but not a prefix - if (shouldMatchData(dd) && inBundle(dd)) { + if (shouldMatchData(tpe) && inBundle(dd)) { val str = stringFromTermName(name) val newRHS = transform(rhs) // chisel3.internal.plugin.autoNameRecursively - val named = q"chisel3.internal.plugin.autoNameRecursively($str, $newRHS)" + val named = q"chisel3.internal.plugin.autoNameRecursively($str)($newRHS)" treeCopy.ValDef(dd, mods, name, tpt, localTyper typed named) } // If a Data or a Memory, get the name and a prefix - else if (shouldMatchDataOrMem(dd)) { + else if (shouldMatchDataOrMem(tpe)) { val str = stringFromTermName(name) val newRHS = transform(rhs) val prefixed = q"chisel3.experimental.prefix.apply[$tpt](name=$str)(f=$newRHS)" - val named = q"chisel3.internal.plugin.autoNameRecursively($str, $prefixed)" + 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 - } else if (shouldMatchModule(dd)) { + } else if (shouldMatchModule(tpe)) { val str = stringFromTermName(name) val newRHS = transform(rhs) - val named = q"chisel3.internal.plugin.autoNameRecursively($str, $newRHS)" + val named = q"chisel3.internal.plugin.autoNameRecursively($str)($newRHS)" treeCopy.ValDef(dd, mods, name, tpt, localTyper typed named) } else { // Otherwise, continue super.transform(tree) } + case dd @ ValDef(mods, name, tpt, rhs @ Match(_, _)) if okUnapply(dd) => + val tpe = inferType(tpt) + val fieldsOfInterest: List[Boolean] = tpe.typeArgs.map(shouldMatchData) + // Only transform if at least one field is of interest + if (fieldsOfInterest.reduce(_ || _)) { + findUnapplyNames(rhs) match { + case Some(names) => + val onames: List[Option[String]] = fieldsOfInterest.zip(names).map { case (ok, name) => if (ok) Some(name) else None } + val named = q"chisel3.internal.plugin.autoNameRecursivelyProduct($onames)($rhs)" + treeCopy.ValDef(dd, mods, name, tpt, localTyper typed named) + case None => // It's not clear how this could happen but we don't want to crash + super.transform(tree) + } + } else { + super.transform(tree) + } // Otherwise, continue case _ => super.transform(tree) } diff --git a/src/test/scala/chiselTests/naming/NamePluginSpec.scala b/src/test/scala/chiselTests/naming/NamePluginSpec.scala index 0f9533e4..5e7133d1 100644 --- a/src/test/scala/chiselTests/naming/NamePluginSpec.scala +++ b/src/test/scala/chiselTests/naming/NamePluginSpec.scala @@ -166,7 +166,7 @@ class NamePluginSpec extends ChiselFlatSpec with Utils { } } - "Multiple names on a non-IO" should "get the last name" in { + "Multiple names on a non-IO" should "get the first name" in { class Test extends MultiIOModule { { val a = Wire(UInt(3.W)) @@ -174,6 +174,73 @@ class NamePluginSpec extends ChiselFlatSpec with Utils { } } + aspectTest(() => new Test) { + top: Test => + Select.wires(top).map(_.instanceName) should be (List("a")) + } + } + + "Outer Expression, First Statement naming" should "apply to IO" in { + class Test extends RawModule { + { + val widthOpt: Option[Int] = Some(4) + val out = widthOpt.map { w => + val port = IO(Output(UInt(w.W))) + port + } + val foo = out + val bar = out.get + } + } + + aspectTest(() => new Test) { + top: Test => + Select.ios(top).map(_.instanceName) should be (List("out")) + } + } + + "Outer Expression, First Statement naming" should "apply to non-IO" in { + class Test extends RawModule { + { + val widthOpt: Option[Int] = Some(4) + val fizz = widthOpt.map { w => + val wire = Wire(UInt(w.W)) + wire + } + val foo = fizz + val bar = fizz.get + } + } + + aspectTest(() => new Test) { + top: Test => + Select.wires(top).map(_.instanceName) should be (List("fizz")) + } + } + + "autoSeed" should "NOT override automatic naming for IO" in { + class Test extends RawModule { + { + val a = IO(Output(UInt(3.W))) + a.autoSeed("b") + } + } + + aspectTest(() => new Test) { + top: Test => + Select.ios(top).map(_.instanceName) should be (List("a")) + } + } + + + "autoSeed" should "override automatic naming for non-IO" in { + class Test extends MultiIOModule { + { + val a = Wire(UInt(3.W)) + a.autoSeed("b") + } + } + aspectTest(() => new Test) { top: Test => Select.wires(top).map(_.instanceName) should be (List("b")) @@ -193,6 +260,54 @@ class NamePluginSpec extends ChiselFlatSpec with Utils { } } + "Unapply assignments" should "not override already named things" in { + class Test extends MultiIOModule { + { + val x = Wire(UInt(3.W)) + val (a, b) = (x, Wire(UInt(3.W))) + } + } + + aspectTest(() => new Test) { + top: Test => + Select.wires(top).map(_.instanceName) should be (List("x", "b")) + } + } + + "Case class unapply assignments" should "be named" in { + case class Foo(x: UInt, y: UInt) + class Test extends MultiIOModule { + { + def func() = Foo(Wire(UInt(3.W)), Wire(UInt(3.W))) + val Foo(a, b) = func() + } + } + + aspectTest(() => new Test) { + top: Test => + Select.wires(top).map(_.instanceName) should be (List("a", "b")) + } + } + + "Complex unapply assignments" should "be named" in { + case class Foo(x: UInt, y: UInt) + class Test extends MultiIOModule { + { + val w = Wire(UInt(3.W)) + def func() = { + val x = Foo(Wire(UInt(3.W)), Wire(UInt(3.W))) + (x, w) :: Nil + } + val ((Foo(a, _), c) :: Nil) = func() + } + } + + aspectTest(() => new Test) { + top: Test => + Select.wires(top).map(_.instanceName) should be (List("w", "a", "_WIRE")) + } + } + "Recursive types" should "not infinitely loop" in { // When this fails, it causes a StackOverflow when compiling the tests // Unfortunately, this doesn't seem to work with assertCompiles(...), it probably ignores the -- cgit v1.2.3