From 82e5ebbbb1a9b685cd6b7db7d3820c72d9034944 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Fri, 3 Jun 2022 15:44:41 -0700 Subject: [PATCH] Add --warn:reflective-naming This new argument (and associated annotation) will turn on a warning whenever reflective naming changes the name of a signal. This is provided to help migrate from Chisel 3.5 to 3.6 since reflective naming is removed in Chisel 3.6. --- core/src/main/scala/chisel3/RawModule.scala | 34 +++- .../hierarchy/core/Definition.scala | 5 +- .../main/scala/chisel3/internal/Builder.scala | 23 ++- .../main/scala/chisel3/internal/Error.scala | 5 + .../aop/injecting/InjectingAspect.scala | 3 +- .../chisel3/stage/ChiselAnnotations.scala | 16 ++ src/main/scala/chisel3/stage/ChiselCli.scala | 1 + .../scala/chisel3/stage/ChiselOptions.scala | 23 +-- src/main/scala/chisel3/stage/package.scala | 11 +- .../chisel3/stage/phases/Elaborate.scala | 4 +- .../naming/ReflectiveNamingSpec.scala | 148 ++++++++++++++++++ 11 files changed, 251 insertions(+), 22 deletions(-) create mode 100644 src/test/scala/chiselTests/naming/ReflectiveNamingSpec.scala diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala index 37eb44afcc9..acac1444de5 100644 --- a/core/src/main/scala/chisel3/RawModule.scala +++ b/core/src/main/scala/chisel3/RawModule.scala @@ -43,6 +43,22 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends val compileOptions = moduleCompileOptions + // This could be factored into a common utility + 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 + } + private[chisel3] override def generateComponent(): Option[Component] = { require(!_closed, "Can't generate module more than once") _closed = true @@ -53,8 +69,24 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends namePorts(names) // Then everything else gets named + val warnReflectiveNaming = Builder.warnReflectiveNaming for ((node, name) <- names) { - node.suggestName(name) + node match { + case d: HasId if warnReflectiveNaming && canBeNamed(d) => + val result = d._suggestNameCheck(name) + result match { + case None => // All good, no warning + case Some((oldName, oldPrefix)) => + val prevName = buildName(oldName, oldPrefix.reverse) + val newName = buildName(name, Nil) + val msg = s"[module ${this.name}] '$prevName' is renamed by reflection to '$newName'. " + + s"Chisel 3.6 removes reflective naming so the name will remain '$prevName'." + Builder.warningNoLoc(msg) + } + // Note that unnamable things end up here (eg. literals), this is supporting backwards + // compatibility + case _ => node.suggestName(name) + } } // All suggestions are in, force names to every node. diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index e846374511f..0ecefd9ac8d 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala @@ -100,7 +100,10 @@ object Definition extends SourceInfoDoc { implicit sourceInfo: SourceInfo, compileOptions: CompileOptions ): Definition[T] = { - val dynamicContext = new DynamicContext(Nil, Builder.captureContext().throwOnFirstError) + val dynamicContext = { + val context = Builder.captureContext() + new DynamicContext(Nil, context.throwOnFirstError, context.warnReflectiveNaming) + } Builder.globalNamespace.copyTo(dynamicContext.globalNamespace) dynamicContext.inDefinition = true val (ir, module) = Builder.build(Module(proto), dynamicContext, false) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index 280e2622ff8..9c800f4adc6 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -137,6 +137,17 @@ 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. * If the final computed name conflicts with another name, it may get uniquified by appending * a digit at the end. @@ -358,7 +369,10 @@ private[chisel3] class ChiselContext() { val viewNamespace = Namespace.empty } -private[chisel3] class DynamicContext(val annotationSeq: AnnotationSeq, val throwOnFirstError: Boolean) { +private[chisel3] class DynamicContext( + val annotationSeq: AnnotationSeq, + val throwOnFirstError: Boolean, + val warnReflectiveNaming: Boolean) { val importDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a } // Ensure there are no repeated names for imported Definitions @@ -633,6 +647,8 @@ private[chisel3] object Builder extends LazyLogging { throwException("Error: No implicit reset.") ) + def warnReflectiveNaming: Boolean = dynamicContext.warnReflectiveNaming + // TODO(twigg): Ideally, binding checks and new bindings would all occur here // However, rest of frontend can't support this yet. def pushCommand[T <: Command](c: T): T = { @@ -674,8 +690,9 @@ private[chisel3] object Builder extends LazyLogging { throwException(m) } } - def warning(m: => String): Unit = if (dynamicContextVar.value.isDefined) errors.warning(m) - def deprecated(m: => String, location: Option[String] = None): Unit = + def warning(m: => String): Unit = if (dynamicContextVar.value.isDefined) errors.warning(m) + def warningNoLoc(m: => String): Unit = if (dynamicContextVar.value.isDefined) errors.warningNoLoc(m) + def deprecated(m: => String, location: Option[String] = None): Unit = if (dynamicContextVar.value.isDefined) errors.deprecated(m, location) /** Record an exception as an error, and throw it. diff --git a/core/src/main/scala/chisel3/internal/Error.scala b/core/src/main/scala/chisel3/internal/Error.scala index 62086870b82..3b0846eb658 100644 --- a/core/src/main/scala/chisel3/internal/Error.scala +++ b/core/src/main/scala/chisel3/internal/Error.scala @@ -186,6 +186,11 @@ private[chisel3] class ErrorLog { def warning(m: => String): Unit = errors += new Warning(m, getUserLineNumber) + /** Log a warning message without a source locator */ + def warningNoLoc(m: => String): Unit = { + errors += new Warning(m, None) + } + /** Emit an informational message */ @deprecated("This method will be removed in 3.5", "3.4") def info(m: String): Unit = diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala index ba873c234f4..087bdae2c3a 100644 --- a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala +++ b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala @@ -58,7 +58,8 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule]( final def toAnnotation(modules: Iterable[M], circuit: String, moduleNames: Seq[String]): AnnotationSeq = { RunFirrtlTransformAnnotation(new InjectingTransform) +: modules.map { module => val chiselOptions = view[ChiselOptions](annotationsInAspect) - val dynamicContext = new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError) + val dynamicContext = + new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError, chiselOptions.warnReflectiveNaming) // Add existing module names into the namespace. If injection logic instantiates new modules // which would share the same name, they will get uniquified accordingly moduleNames.foreach { n => diff --git a/src/main/scala/chisel3/stage/ChiselAnnotations.scala b/src/main/scala/chisel3/stage/ChiselAnnotations.scala index e046319d900..eda05a7d831 100644 --- a/src/main/scala/chisel3/stage/ChiselAnnotations.scala +++ b/src/main/scala/chisel3/stage/ChiselAnnotations.scala @@ -79,6 +79,22 @@ case object ThrowOnFirstErrorAnnotation } +/** Warn when reflective naming changes names of signals */ +case object WarnReflectiveNamingAnnotation + extends NoTargetAnnotation + with ChiselOption + with HasShellOptions + with Unserializable { + + val options = Seq( + new ShellOption[Unit]( + longOption = "warn:reflective-naming", + toAnnotationSeq = _ => Seq(this), + helpText = "Warn when reflective naming changes the name of signals (3.6 migration)" + ) + ) +} + /** An [[firrtl.annotations.Annotation]] storing a function that returns a Chisel module * @param gen a generator function */ diff --git a/src/main/scala/chisel3/stage/ChiselCli.scala b/src/main/scala/chisel3/stage/ChiselCli.scala index f38bf50c349..8c5eb79ada7 100644 --- a/src/main/scala/chisel3/stage/ChiselCli.scala +++ b/src/main/scala/chisel3/stage/ChiselCli.scala @@ -10,6 +10,7 @@ trait ChiselCli { this: Shell => NoRunFirrtlCompilerAnnotation, PrintFullStackTraceAnnotation, ThrowOnFirstErrorAnnotation, + WarnReflectiveNamingAnnotation, ChiselOutputFileAnnotation, ChiselGeneratorAnnotation ) diff --git a/src/main/scala/chisel3/stage/ChiselOptions.scala b/src/main/scala/chisel3/stage/ChiselOptions.scala index 7e4305faf9a..a03f3d7b20e 100644 --- a/src/main/scala/chisel3/stage/ChiselOptions.scala +++ b/src/main/scala/chisel3/stage/ChiselOptions.scala @@ -5,24 +5,27 @@ package chisel3.stage import chisel3.internal.firrtl.Circuit class ChiselOptions private[stage] ( - val runFirrtlCompiler: Boolean = true, - val printFullStackTrace: Boolean = false, - val throwOnFirstError: Boolean = false, - val outputFile: Option[String] = None, - val chiselCircuit: Option[Circuit] = None) { + val runFirrtlCompiler: Boolean = true, + val printFullStackTrace: Boolean = false, + val throwOnFirstError: Boolean = false, + val warnReflectiveNaming: Boolean = false, + val outputFile: Option[String] = None, + val chiselCircuit: Option[Circuit] = None) { private[stage] def copy( - runFirrtlCompiler: Boolean = runFirrtlCompiler, - printFullStackTrace: Boolean = printFullStackTrace, - throwOnFirstError: Boolean = throwOnFirstError, - outputFile: Option[String] = outputFile, - chiselCircuit: Option[Circuit] = chiselCircuit + runFirrtlCompiler: Boolean = runFirrtlCompiler, + printFullStackTrace: Boolean = printFullStackTrace, + throwOnFirstError: Boolean = throwOnFirstError, + warnReflectiveNaming: Boolean = warnReflectiveNaming, + outputFile: Option[String] = outputFile, + chiselCircuit: Option[Circuit] = chiselCircuit ): ChiselOptions = { new ChiselOptions( runFirrtlCompiler = runFirrtlCompiler, printFullStackTrace = printFullStackTrace, throwOnFirstError = throwOnFirstError, + warnReflectiveNaming = warnReflectiveNaming, outputFile = outputFile, chiselCircuit = chiselCircuit ) diff --git a/src/main/scala/chisel3/stage/package.scala b/src/main/scala/chisel3/stage/package.scala index b1064c050b9..10c8c524fad 100644 --- a/src/main/scala/chisel3/stage/package.scala +++ b/src/main/scala/chisel3/stage/package.scala @@ -15,11 +15,12 @@ package object stage { def view(options: AnnotationSeq): ChiselOptions = options.collect { case a: ChiselOption => a } .foldLeft(new ChiselOptions()) { (c, x) => x match { - case NoRunFirrtlCompilerAnnotation => c.copy(runFirrtlCompiler = false) - case PrintFullStackTraceAnnotation => c.copy(printFullStackTrace = true) - case ThrowOnFirstErrorAnnotation => c.copy(throwOnFirstError = true) - case ChiselOutputFileAnnotation(f) => c.copy(outputFile = Some(f)) - case ChiselCircuitAnnotation(a) => c.copy(chiselCircuit = Some(a)) + case NoRunFirrtlCompilerAnnotation => c.copy(runFirrtlCompiler = false) + case PrintFullStackTraceAnnotation => c.copy(printFullStackTrace = true) + case ThrowOnFirstErrorAnnotation => c.copy(throwOnFirstError = true) + case WarnReflectiveNamingAnnotation => c.copy(warnReflectiveNaming = true) + case ChiselOutputFileAnnotation(f) => c.copy(outputFile = Some(f)) + case ChiselCircuitAnnotation(a) => c.copy(chiselCircuit = Some(a)) } } diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/src/main/scala/chisel3/stage/phases/Elaborate.scala index 55331cb4d7d..ba29e5f241f 100644 --- a/src/main/scala/chisel3/stage/phases/Elaborate.scala +++ b/src/main/scala/chisel3/stage/phases/Elaborate.scala @@ -29,8 +29,10 @@ class Elaborate extends Phase { case ChiselGeneratorAnnotation(gen) => val chiselOptions = view[ChiselOptions](annotations) try { + val context = + new DynamicContext(annotations, chiselOptions.throwOnFirstError, chiselOptions.warnReflectiveNaming) val (circuit, dut) = - Builder.build(Module(gen()), new DynamicContext(annotations, chiselOptions.throwOnFirstError)) + Builder.build(Module(gen()), context) Seq(ChiselCircuitAnnotation(circuit), DesignAnnotation(dut)) } catch { /* if any throwable comes back and we're in "stack trace trimming" mode, then print an error and trim the stack trace diff --git a/src/test/scala/chiselTests/naming/ReflectiveNamingSpec.scala b/src/test/scala/chiselTests/naming/ReflectiveNamingSpec.scala new file mode 100644 index 00000000000..437f3d6f34f --- /dev/null +++ b/src/test/scala/chiselTests/naming/ReflectiveNamingSpec.scala @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chiselTests.naming + +import chisel3._ +import chiselTests.{ChiselFlatSpec, Utils} + +class ReflectiveNamingSpec extends ChiselFlatSpec with Utils { + + behavior.of("Reflective naming") + + private def emitChirrtl(gen: => RawModule): String = { + // Annoyingly need to emit files to use CLI + val targetDir = createTestDirectory(this.getClass.getSimpleName).toString + val args = Array("--warn:reflective-naming", "-td", targetDir) + (new chisel3.stage.ChiselStage).emitChirrtl(gen, args) + } + + it should "NOT warn when no names are changed" in { + class Example extends Module { + val foo, bar = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + + val sum = foo +& bar + out := sum + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should equal("") + chirrtl should include("node sum = add(foo, bar)") + } + + it should "warn when changing the name of a node" in { + class Example extends Module { + val foo, bar = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + + val sum = foo +& bar + val fuzz = sum + out := sum + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should include("'sum' is renamed by reflection to 'fuzz'") + chirrtl should include("node fuzz = add(foo, bar)") + } + + // This also checks correct prefix reversing + it should "warn when changing the name of a node with a prefix in the name" in { + class Example extends Module { + val foo, bar = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + + // This is sketch, don't do this + var fuzz = 0.U + out := { + val sum = { + val node = foo +& bar + fuzz = node + node +% 0.U + } + sum + } + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should include("'out_sum_node' is renamed by reflection to 'fuzz'") + chirrtl should include("node fuzz = add(foo, bar)") + } + + it should "warn when changing the name of a Module instance" in { + import chisel3.util._ + class Example extends Module { + val enq = IO(Flipped(Decoupled(UInt(8.W)))) + val deq = IO(Decoupled(UInt(8.W))) + + val q = Module(new Queue(UInt(8.W), 4)) + q.io.enq <> enq + deq <> q.io.deq + + val fuzz = q + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should include("'q' is renamed by reflection to 'fuzz'") + chirrtl should include("inst fuzz of Queue") + } + + it should "warn when changing the name of a Mem" in { + class Example extends Module { + val mem = SyncReadMem(8, UInt(8.W)) + + val fuzz = mem + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should include("'mem' is renamed by reflection to 'fuzz'") + chirrtl should include("smem fuzz") + } + + it should "NOT warn when changing the name of a verification statement" in { + class Example extends Module { + val in = IO(Input(UInt(8.W))) + val z = chisel3.assert(in =/= 123.U) + val fuzz = z + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should equal("") + // But the name is actually changed + chirrtl should include regex("assert.*: fuzz") + } + + it should "NOT warn when \"naming\" a literal" in { + class Example extends Module { + val out = IO(Output(UInt(8.W))) + + val sum = 0.U + val fuzz = sum + out := sum + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should equal("") + chirrtl should include("out <= UInt") + } + + it should "NOT warn when \"naming\" a field of an Aggregate" in { + class Example extends Module { + val io = IO(new Bundle { + val in = Input(UInt(8.W)) + val out = Output(UInt(8.W)) + }) + val in = io.in + val out = io.out + out := in + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should equal("") + chirrtl should include("io.out <= io.in") + } + + it should "NOT warn when \"naming\" unbound Data" in { + class Example extends Module { + val in = IO(Input(UInt(8.W))) + val out = IO(Output(UInt(8.W))) + val z = UInt(8.W) + val a = z + out := in + } + val (log, chirrtl) = grabLog(emitChirrtl(new Example)) + log should equal("") + chirrtl should include("out <= in") + } +}