Skip to content

Commit

Permalink
Add --warn:reflective-naming
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jackkoenig committed Jun 6, 2022
1 parent 370ca8a commit 97afd9b
Show file tree
Hide file tree
Showing 11 changed files with 264 additions and 22 deletions.
34 changes: 33 additions & 1 deletion core/src/main/scala/chisel3/RawModule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 20 additions & 3 deletions core/src/main/scala/chisel3/internal/Builder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/scala/chisel3/internal/Error.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
3 changes: 2 additions & 1 deletion src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down
16 changes: 16 additions & 0 deletions src/main/scala/chisel3/stage/ChiselAnnotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
1 change: 1 addition & 0 deletions src/main/scala/chisel3/stage/ChiselCli.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ trait ChiselCli { this: Shell =>
NoRunFirrtlCompilerAnnotation,
PrintFullStackTraceAnnotation,
ThrowOnFirstErrorAnnotation,
WarnReflectiveNamingAnnotation,
ChiselOutputFileAnnotation,
ChiselGeneratorAnnotation
)
Expand Down
23 changes: 13 additions & 10 deletions src/main/scala/chisel3/stage/ChiselOptions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down
11 changes: 6 additions & 5 deletions src/main/scala/chisel3/stage/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/main/scala/chisel3/stage/phases/Elaborate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
161 changes: 161 additions & 0 deletions src/test/scala/chiselTests/naming/ReflectiveNamingSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
// 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 an Instance" in {
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)
val fuzz = inst
}
val (log, chirrtl) = grabLog(emitChirrtl(new Example))
log should include("'inst' is renamed by reflection to 'fuzz'")
chirrtl should include("inst fuzz of AddOne")
}

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")
}
}

0 comments on commit 97afd9b

Please sign in to comment.