From 9a7945fd86fcad02da0556d8f4a30daa4b005f9d Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Tue, 10 Jan 2023 07:19:45 +0000 Subject: Check for Vec subaccess in NamedComponent and throw a nicer error. (backport #2907) (#2928) * Check for Vec subaccess in NamedComponent and throw a nicer error. (#2907) This would previously end up throwing an exception later, when trying to create a component name and realizing that it was invalid. Instead, this detects Vec subaccesses early, and gives a more precise error and suggestion. (cherry picked from commit d8c30961c7b293ee19024a487698630367ee71c6) # Conflicts: # core/src/main/scala/chisel3/internal/Builder.scala * Resolve backport conflicts Co-authored-by: Mike Urbach --- core/src/main/scala/chisel3/internal/Builder.scala | 46 +++++++++--- src/test/scala/chiselTests/VecToTargetSpec.scala | 86 ++++++++++++++++++++++ 2 files changed, 121 insertions(+), 11 deletions(-) create mode 100644 src/test/scala/chiselTests/VecToTargetSpec.scala diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index fe7d7bea..be2c4cfb 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -299,25 +299,31 @@ private[chisel3] trait HasId extends InstanceId { // Builder.deprecated mechanism, we have to create our own one off ErrorLog and print the // warning right away. // It's especially bad because --warnings-as-errors does not work with these warnings - val nameGuess = _computeName(None) match { - case Some(name) => s": '$name'" - case None => "" - } - val parentGuess = _parent match { - case Some(ViewParent) => s", in module '${reifyParent.pathName}'" - case Some(p) => s", in module '${p.pathName}'" - case None => "" - } val errors = new ErrorLog(false) val logger = new _root_.logger.Logger(this.getClass.getName) val msg = - "Accessing the .instanceName or .toTarget of non-hardware Data is deprecated" + nameGuess + parentGuess + ". " + + "Accessing the .instanceName or .toTarget of non-hardware Data is deprecated" + _errorContext + ". " + "This will become an error in Chisel 3.6." errors.deprecated(msg, None) errors.checkpoint(logger) _computeName(None).get } + private[chisel3] def _errorContext: String = { + val nameGuess: String = _computeName(None) match { + case Some(name) => s": '$name'" + case None => "" + } + + val parentGuess: String = _parent match { + case Some(ViewParent) => s", in module '${reifyParent.pathName}'" + case Some(p) => s", in module '${p.pathName}'" + case None => "" + } + + nameGuess + parentGuess + } + // Helper for reifying views if they map to a single Target private[chisel3] def reifyTarget: Option[Data] = this match { case d: Data => reifySingleData(d) // Only Data can be views @@ -393,13 +399,16 @@ private[chisel3] trait NamedComponent extends HasId { /** Returns a FIRRTL ComponentName that references this object * @note Should not be called until circuit elaboration is complete */ - final def toNamed: ComponentName = + final def toNamed: ComponentName = { + assertValidTarget() ComponentName(this.instanceName, ModuleName(this.parentModName, CircuitName(this.circuitName))) + } /** Returns a FIRRTL ReferenceTarget that references this object * @note Should not be called until circuit elaboration is complete */ final def toTarget: ReferenceTarget = { + assertValidTarget() val name = this.instanceName if (!validComponentName(name)) throwException(s"Illegal component name: $name (note: literals are illegal)") import _root_.firrtl.annotations.{Target, TargetToken} @@ -423,6 +432,21 @@ private[chisel3] trait NamedComponent extends HasId { case None => localTarget } } + + private def assertValidTarget(): Unit = { + val isVecSubaccess = getOptionRef.map { + case Index(_, _: ULit) => true // Vec literal indexing + case Index(_, _: Node) => true // Vec dynamic indexing + case _ => false + }.getOrElse(false) + + if (isVecSubaccess) { + throwException( + s"You cannot target Vec subaccess" + _errorContext + + ". Instead, assign it to a temporary (for example, with WireInit) and target the temporary." + ) + } + } } // Mutable global state for chisel that can appear outside a Builder context diff --git a/src/test/scala/chiselTests/VecToTargetSpec.scala b/src/test/scala/chiselTests/VecToTargetSpec.scala new file mode 100644 index 00000000..20c6f306 --- /dev/null +++ b/src/test/scala/chiselTests/VecToTargetSpec.scala @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chiselTests + +import chisel3._ +import chisel3.stage.ChiselStage + +trait VecToTargetSpecUtils extends Utils { + this: ChiselFunSpec => + + class Foo extends RawModule { + val vec = IO(Input(Vec(4, Bool()))) + + // Index a Vec with a Scala literal. + val scalaLit = 0 + val vecSubaccessScalaLit = vec(scalaLit) + + // Index a Vec with a Chisel literal. + val chiselLit = 0.U + val vecSubaccessChiselLit = vec(chiselLit) + + // Index a Vec with a node. + val node = IO(Input(UInt(2.W))) + val vecSubaccessNode = vec(node) + + // Put an otherwise un-targetable Vec subaccess into a temp. + val vecSubaccessTmp = WireInit(vecSubaccessNode) + } + + val expectedError = "You cannot target Vec subaccess:" + + def conversionSucceeds(data: InstanceId) = { + describe(".toTarget") { + it("should convert successfully") { + data.toTarget + } + } + + describe(".toNamed") { + it("should convert successfully") { + data.toNamed + } + } + } + + def conversionFails(data: InstanceId) = { + describe(".toTarget") { + it("should fail to convert with a useful error message") { + (the[ChiselException] thrownBy extractCause[ChiselException] { + data.toTarget + }).getMessage should include(expectedError) + } + } + + describe(".toNamed") { + it("should fail to convert with a useful error message") { + (the[ChiselException] thrownBy extractCause[ChiselException] { + data.toNamed + }).getMessage should include(expectedError) + } + } + } +} + +class VecToTargetSpec extends ChiselFunSpec with VecToTargetSpecUtils { + describe("Vec subaccess") { + var foo: Foo = null + ChiselStage.elaborate { foo = new Foo; foo } + + describe("with a Scala literal") { + (it should behave).like(conversionSucceeds(foo.vecSubaccessScalaLit)) + } + + describe("with a Chisel literal") { + (it should behave).like(conversionFails(foo.vecSubaccessChiselLit)) + } + + describe("with a Node") { + (it should behave).like(conversionFails(foo.vecSubaccessNode)) + } + + describe("with an un-targetable construct that is assigned to a temporary") { + (it should behave).like(conversionSucceeds(foo.vecSubaccessTmp)) + } + } +} -- cgit v1.2.3