diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala index 926e33d5ab3..b6f66d8cd91 100644 --- a/core/src/main/scala/chisel3/Module.scala +++ b/core/src/main/scala/chisel3/Module.scala @@ -117,8 +117,8 @@ object Module extends SourceInfoDoc { */ abstract class Module(implicit moduleCompileOptions: CompileOptions) extends RawModule { // Implicit clock and reset pins - final val clock: Clock = IO(Input(Clock())).suggestName("clock") - final val reset: Reset = IO(Input(mkReset)).suggestName("reset") + final val clock: Clock = IO(Input(Clock()))._suggestNameInternal("clock") + final val reset: Reset = IO(Input(mkReset))._suggestNameInternal("reset") // TODO It's hard to remove these deprecated override methods because they're used by // Chisel.QueueCompatibility which extends chisel3.Queue which extends chisel3.Module diff --git a/core/src/main/scala/chisel3/experimental/package.scala b/core/src/main/scala/chisel3/experimental/package.scala index 730aae8be51..290a15877a4 100644 --- a/core/src/main/scala/chisel3/experimental/package.scala +++ b/core/src/main/scala/chisel3/experimental/package.scala @@ -73,7 +73,7 @@ package object experimental { gen.elements.toSeq.reverse.map { case (name, data) => val p = IO(coerceDirection(chiselTypeClone(data).asInstanceOf[Data])) - p.suggestName(name) + p._suggestNameInternal(name) p } diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index cefc3a798c9..bd437ceab2b 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -129,31 +129,53 @@ private[chisel3] trait HasId extends InstanceId { this } - // Private internal version of suggestName that tells you if the name changed - // Returns Some(old name, old prefix) if name changed, None otherwise - private[chisel3] def _suggestNameCheck(seed: => String): Option[(String, Prefix)] = { - val oldSeed = this.seedOpt - val oldPrefix = this.naming_prefix - suggestName(seed) - if (oldSeed.nonEmpty && (oldSeed != this.seedOpt || oldPrefix != this.naming_prefix)) { - Some(oldSeed.get -> oldPrefix) - } else None - } - - /** Takes the first seed suggested. Multiple calls to this function will be ignored. + def _suggestNameInternal(seed: => String): this.type = { + if (suggested_seed.isEmpty) suggested_seed = Some(seed) + naming_prefix = Builder.getPrefix + this + } + + /** Takes the first seed suggested. Multiple calls to this function will be ignored (and will become an error in future). * If the final computed name conflicts with another name, it may get uniquified by appending * a digit at the end. * * Is a higher priority than [[autoSeed]], in that regardless of whether [[autoSeed]] * was called, [[suggestName]] will always take precedence. * + * @note calling this after the name has already been computed will become an error in the future. + * * @param seed The seed for the name of this component * @return this object */ def suggestName(seed: => String): this.type = { - if (suggested_seed.isEmpty) suggested_seed = Some(seed) - naming_prefix = Builder.getPrefix - this + if (!Builder.hasDynamicContext) { + // This is super hacky but this is just for a short term deprecation. + // This access is detected outside of Chisel elaboration so we cannot use the normal + // Builder.deprecated mechanism, we have to create our own one off ErrorLog and print the + // warning right away. + val errors = new ErrorLog + val logger = new _root_.logger.Logger(this.getClass.getName) + val msg = "suggestName(\"" + seed + "\") should only be called from a Builder context." + + "This will become an error in Chisel 3.6." + errors.deprecated(msg, None) + errors.checkpoint(logger) + } + if (suggested_seed.isDefined) { + Builder.deprecated( + "Calling suggestName(\"" + seed + "\"), when already called with \"" + suggested_seed.get + "\", will become an error in Chisel 3.6" + ) + } + if (!HasId.canBeNamed(this)) { + Builder.deprecated( + "Calling suggestName(\"" + seed + "\") on \"" + this + "\" (which cannot actually be named) will become an error in Chisel 3.6" + ) + } + if (_parent.map(_.isClosed).getOrElse(false)) { // not sure what it means to have no parent + Builder.deprecated( + "Calling suggestName(\"" + seed + "\") on \"" + this + "\" when the containing module \"" + _parent.get.name + "\" has already completed elaboration will become an error in Chisel 3.6" + ) + } + _suggestNameInternal(seed) } // Internal version of .suggestName that can override a user-suggested name @@ -163,11 +185,12 @@ private[chisel3] trait HasId extends InstanceId { // This could be called with user prefixes, ignore them noPrefix { suggested_seed = Some(seed) - this.suggestName(seed) + this._suggestNameInternal(seed) } } /** Computes the name of this HasId, if one exists + * * @param defaultSeed Optionally provide default seed for computing the name * @return the name, if it can be computed */ @@ -178,10 +201,22 @@ private[chisel3] trait HasId extends InstanceId { } /** This resolves the precedence of [[autoSeed]] and [[suggestName]] + * + * @note It will become an error in the future to suggestName the same thing that autoSeed would have assigned * * @return the current calculation of a name, if it exists */ - private[chisel3] def seedOpt: Option[String] = suggested_seed.orElse(auto_seed) + private[chisel3] def seedOpt: Option[String] = { + if (suggested_seed.isDefined && auto_seed.isDefined) { + if (suggested_seed.get == auto_seed.get) { + Builder.deprecated( + "calling suggestName(\"" + suggested_seed.get + "\") on \"" + this._parent.get.name + '.' + suggested_seed.get + "\" had no effect as it is the same as the automatically given name, this will become an error in 3.6", + Some("(unknown)") + ) + } + } + suggested_seed.orElse(auto_seed) + } /** @return Whether either autoName or suggestName has been called */ def hasSeed: Boolean = seedOpt.isDefined @@ -268,6 +303,25 @@ private[chisel3] trait HasId extends InstanceId { } } +private[chisel3] object HasId { + + /** Utility for things that (currently) appear to be nameable but actually cannot be */ + private def canBeNamed(id: HasId): Boolean = id match { + case d: Data => + d.binding match { + case Some(_: ConstrainedBinding) => true + case _ => false + } + case b: BaseModule => true + case m: MemBase[_] => true + // These names don't affect hardware + case _: VerificationStatement => false + // While the above should be comprehensive, since this is used in warning we want to be careful + // to never accidentally have a match error + case _ => false + } +} + /** Holds the implementation of toNamed for Data and MemBase */ private[chisel3] trait NamedComponent extends HasId { diff --git a/core/src/main/scala/chisel3/internal/Namer.scala b/core/src/main/scala/chisel3/internal/Namer.scala index c36aafeff9c..4f62480f6ee 100644 --- a/core/src/main/scala/chisel3/internal/Namer.scala +++ b/core/src/main/scala/chisel3/internal/Namer.scala @@ -108,7 +108,7 @@ class NamingContext extends NamingContextInterface { closed = true for ((ref, suffix) <- items) { // First name the top-level object - chisel3.internal.Builder.nameRecursively(prefix + suffix, ref, (id, name) => id.suggestName(name)) + chisel3.internal.Builder.nameRecursively(prefix + suffix, ref, (id, name) => id._suggestNameInternal(name)) // Then recurse into descendant contexts if (descendants.containsKey(ref)) { diff --git a/src/test/scala/chiselTests/naming/SuggestNameSpec.scala b/src/test/scala/chiselTests/naming/SuggestNameSpec.scala new file mode 100644 index 00000000000..f04ae6a8486 --- /dev/null +++ b/src/test/scala/chiselTests/naming/SuggestNameSpec.scala @@ -0,0 +1,240 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chiselTests.naming + +import chisel3._ +import chisel3.aop.Select +import chisel3.stage.{ChiselGeneratorAnnotation, ChiselStage} +import chisel3.experimental.{dump, noPrefix, prefix, treedump} +import chiselTests.{ChiselPropSpec, Utils} +import chisel3.stage.{DesignAnnotation, NoRunFirrtlCompilerAnnotation} +import firrtl.options.{Dependency, Phase, PhaseManager} +import firrtl.options.phases.DeletedWrapper + +class SuggestNameSpec extends ChiselPropSpec with Utils { + implicit val minimumMajorVersion: Int = 12 + + property("0. Calling suggestName 2x should be a runtime deprecation") { + class Test extends Module { + { + val wire = { + val x = WireInit(0.U(3.W)).suggestName("mywire") + dontTouch(x) + } + wire.suggestName("somethingElse") + } + } + val (log, _) = grabLog(ChiselStage.emitVerilog(new Test())) + log should include( + "Calling suggestName(\"somethingElse\"), when already called with \"mywire\", will become an error in Chisel 3.6" + ) + } + + property("1. Calling suggestName outside of a Builder context should be a runtime deprecation") { + class Test extends Module { + val wire = { + val x = WireInit(0.U(3.W)) + dontTouch(x) + } + } + + // Nasty use of var, only for this test purpose. Don't do stuff like this! + var test: Test = null + ChiselStage.elaborate { + test = new Test + test + } + val (log, _) = grabLog { + test.wire.suggestName("somethingElse") + } + log should include("suggestName(\"somethingElse\") should only be called from a Builder context") + } + + property("2. Calling suggestName after module close should be a runtime deprecation") { + class Child extends Module { + val wire = { + val x = WireInit(0.U(3.W)) + dontTouch(x) + } + } + class Test extends Module { + val child = Module(new Child()) + child.wire.suggestName("somethingElse") + } + val (log, _) = grabLog(ChiselStage.emitVerilog(new Test())) + log should include( + "Calling suggestName(\"somethingElse\") on \"Child.wire: Wire[UInt<3>]\" when the containing module \"Child\" has already completed elaboration" + ) + } + + property("3. Calling suggestName with the same thing prefix would have given should be a runtime deprecation") { + class Test extends Module { + val wire = { + val x = WireInit(0.U(3.W)).suggestName("wire") + dontTouch(x) + } + } + val (log, _) = grabLog(ChiselStage.emitVerilog(new Test())) + log should include( + "calling suggestName(\"wire\") on \"Test.wire\" had no effect as it is the same as the automatically given name" + ) + } + + property("4a. Calling suggestName on a node should be allowed") { + class Example extends Module { + val foo, bar = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + + val sum = foo +& bar + sum.suggestName("fuzz") + out := sum + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should equal("") + chirrtl should include("node fuzz = add(foo, bar)") + } + + property("4b. Calling suggestName on an IO should be allowed") { + class Example extends Module { + val foo, bar = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + + val sum = foo +& bar + foo.suggestName("FOO") + bar.suggestName("BAR") + out := sum + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should equal("") + chirrtl should include("node sum = add(FOO, BAR)") + } + + property("4c. Calling suggestName on a prefixed node should be allowed") { + class Example extends Module { + val foo, bar = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + + out := { + val sum = { + val node = foo +& bar + node.suggestName("fuzz") + node +% 0.U + } + sum + } + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should equal("") + chirrtl should include("node out_sum_fuzz = add(foo, bar)") + } + + property("4d. Calling suggestName on a Module instance should be allowed") { + import chisel3.util._ + + class PassThrough extends Module { + val enq = IO(Flipped(Decoupled(UInt(8.W)))) + val deq = IO(Decoupled(UInt(8.W))) + deq <> enq + } + + class Example extends Module { + val enq = IO(Flipped(Decoupled(UInt(8.W)))) + val deq = IO(Decoupled(UInt(8.W))) + + val q = Module(new PassThrough()) + q.enq <> enq + deq <> q.deq + q.suggestName("fuzz") + } + + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should equal("") + chirrtl should include("inst fuzz of PassThrough") + } + + property("4e. Calling suggestName on a Mem should be allowed") { + class Example extends Module { + val mem = SyncReadMem(8, UInt(8.W)) + + mem.suggestName("fuzz") + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should equal("") + chirrtl should include("smem fuzz") + } + + property("4f. Calling suggestName on a verif statement should be a runtime deprecation") { + class Example extends Module { + val in = IO(Input(UInt(8.W))) + val z = chisel3.assert(in =/= 123.U) + z.suggestName("fuzz") + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should include("Calling suggestName(\"fuzz\") on \"chisel3.assert$Assert") + (chirrtl should include).regex("assert.*: z") + } + + property("4g. Calling suggestName on a literal should be a runtime deprecation") { + class Example extends Module { + val out = IO(Output(UInt(8.W))) + + val sum = 0.U + sum.suggestName("fuzz") + out := sum + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should include("Calling suggestName(\"fuzz\") on \"UInt<1>(0)\" (which cannot actually be named)") + chirrtl should include("out <= UInt") + } + + property("4h. Calling suggestName on a field of an Aggregate should be a runtime deprecation") { + class Example extends Module { + val io = IO(new Bundle { + val in = Input(UInt(8.W)) + val out = Output(UInt(8.W)) + }) + io.in.suggestName("fuzz") + io.out.suggestName("bar") + io.out := io.in + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should include( + "Calling suggestName(\"fuzz\") on \"Example.io.in: IO[UInt<8>]\" (which cannot actually be named)" + ) + log should include( + "Calling suggestName(\"bar\") on \"Example.io.out: IO[UInt<8>]\" (which cannot actually be named)" + ) + + chirrtl should include("io.out <= io.in") + } + + property("4i. Calling suggestName on unbound Data should be a runtime deprecation") { + class Example extends Module { + val in = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + val z = UInt(8.W) + z.suggestName("fuzz") + out := in + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should include("Calling suggestName(\"fuzz\") on \"UInt<8>\" (which cannot actually be named)") + chirrtl should include("out <= in") + } + + /* + // This test is commented out until https://github.com/chipsalliance/chisel3/issues/2366 is resolved + property("4j. Calling suggestName on an Instance instance should be allowed") { + import chisel3.experimental.hierarchy.{Definition, Instance} + import chiselTests.experimental.hierarchy.Examples.AddOne + class Example extends Module { + val defn = Definition(new AddOne) + val inst = Instance(defn) + inst.suggestName("fuzz") + val fuzz = inst + } + val (log, chirrtl) = grabLog(ChiselStage.emitChirrtl(new Example)) + log should equal("") + chirrtl should include("inst fuzz of AddOne") + } + */ +}