From 3ef3ea2ba7d7a65bc59b43f823b1e1487d2fe4ce Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 6 Apr 2023 21:14:21 +0000 Subject: [PATCH 1/3] Fix naming for RHS of named unapply expressions (backport #3163) (#3165) * Fix naming for RHS of named unapply expressions (#3163) Previously, the naming plugin would not recurse for the RHS of an unapply, meaning that vals declared in blocks that ultimately return some result that is unapplied into some named Data would not be named. Also, audit and improve the naming mdocs. (cherry picked from commit 262caa7312a31b073965df1d44cfdbb5f515a482) # Conflicts: # docs/src/cookbooks/naming.md * Resolve backport conflicts --------- Co-authored-by: Jack Koenig --- docs/src/cookbooks/naming.md | 36 ++++++++----- docs/src/explanations/naming.md | 50 +++++++++++-------- .../internal/plugin/ChiselComponent.scala | 3 +- .../chiselTests/naming/NamePluginSpec.scala | 16 ++++++ 4 files changed, 71 insertions(+), 34 deletions(-) diff --git a/docs/src/cookbooks/naming.md b/docs/src/cookbooks/naming.md index 4e6d26e3f56..37e97c30b39 100644 --- a/docs/src/cookbooks/naming.md +++ b/docs/src/cookbooks/naming.md @@ -7,6 +7,10 @@ section: "chisel3" ```scala mdoc:invisible import chisel3._ import circt.stage.ChiselStage +def emitSystemVerilog(gen: => RawModule): String = { + val prettyArgs = Array("--disable-all-randomization", "--strip-debug-info") + ChiselStage.emitSystemVerilog(gen, firtoolOpts = prettyArgs) +} ``` # Naming Cookbook @@ -28,8 +32,8 @@ We recommend you manually insert calls to `prefix` to clarify these cases: import chisel3.experimental.prefix class ExamplePrefix extends Module { - Seq.tabulate{2} {i => - Seq.tabulate{2}{ j => + Seq.tabulate(2) { i => + Seq.tabulate(2) { j => prefix(s"loop_${i}_${j}"){ val x = WireInit((i*0x10+j).U(8.W)) dontTouch(x) @@ -39,7 +43,7 @@ class ExamplePrefix extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new ExamplePrefix) +emitSystemVerilog(new ExamplePrefix) ``` ### How can I get better names for code generated by `when` clauses? @@ -53,18 +57,20 @@ class ExampleWhenPrefix extends Module { out := DontCare - Seq.tabulate{2}{ i => + Seq.tabulate(2) { i => val j = i + 1 - when (in === j.U) { prefix(s"clause_${j}"){ - val foo = Wire(UInt(4.W)) - foo := in +& j.U(4.W) - out := foo - }} + prefix(s"clause_${j}") { + when (in === j.U) { + val foo = Reg(UInt(4.W)) + foo := in + j.U(4.W) + out := foo + } + } } } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new ExampleWhenPrefix) +emitSystemVerilog(new ExampleWhenPrefix) ``` ### I still see _GEN signals, can this be fixed? @@ -100,13 +106,17 @@ class ExampleNoPrefix extends Module { val in = IO(Input(UInt(2.W))) val out = IO(Output(UInt())) - val add = noPrefix { in + in + in } + val add = noPrefix { + // foo will not get a prefix + val foo = RegNext(in + 1.U) + foo + in + } out := add } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new ExampleNoPrefix) +emitSystemVerilog(new ExampleNoPrefix) ``` ### I am still not getting the name I want. For example, inlining an instance changes my name! @@ -138,7 +148,7 @@ class MyLeaf extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new WrapperExample) +emitSystemVerilog(new WrapperExample) ``` This can be used to rename instances and non-aggregate typed signals. diff --git a/docs/src/explanations/naming.md b/docs/src/explanations/naming.md index 0045c8ae99b..bc58570c10d 100644 --- a/docs/src/explanations/naming.md +++ b/docs/src/explanations/naming.md @@ -21,7 +21,14 @@ systemic name-stability issues, please refer to the naming [cookbook](../cookboo // Imports used by the following examples import chisel3._ import chisel3.experimental.{prefix, noPrefix} +``` + +```scala mdoc:invisible import circt.stage.ChiselStage +def emitSystemVerilog(gen: => RawModule): String = { + val prettyArgs = Array("--disable-all-randomization", "--strip-debug-info") + ChiselStage.emitSystemVerilog(gen, firtoolOpts = prettyArgs) +} ``` With the release of Chisel 3.5, users are required to add the following line to @@ -51,7 +58,7 @@ class Example1 extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example1) +emitSystemVerilog(new Example1) ``` Otherwise, it is rewritten to also include the name as a prefix to any signals generated while executing the right-hand- @@ -75,7 +82,7 @@ class Example2 extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example2) +emitSystemVerilog(new Example2) ``` Prefixing can also be derived from the name of signals on the left-hand side of a connection. @@ -98,7 +105,7 @@ class ConnectPrefixing extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new ConnectPrefixing) +emitSystemVerilog(new ConnectPrefixing) ``` Note that the naming also works if the hardware type is nested in an `Option` or a subtype of `Iterable`: @@ -109,19 +116,22 @@ class Example3 extends Module { // 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())))) - def inXin() = in * in + def func() = { + val delay = RegNext(in) + delay + 1.U + } - 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 = Some(func()) + // Note that the register in func() is prefixed with `opt`: + // val opt = autoNameRecursively("opt")(prefix("opt")(Some(func())) out := opt.get + 1.U } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example3) +emitSystemVerilog(new Example3) ``` There is also a slight variant (`autoNameRecursivelyProduct`) for naming hardware with names provided by an unapply: @@ -135,7 +145,7 @@ class UnapplyExample extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new UnapplyExample) +emitSystemVerilog(new UnapplyExample) ``` Note that the compiler plugin will not insert a prefix in these cases because it is ambiguous what the prefix should be. @@ -169,8 +179,8 @@ class Example5 extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example4) -ChiselStage.emitSystemVerilog(new Example5) +emitSystemVerilog(new Example4) +emitSystemVerilog(new Example5) ``` Also note that the prefixes append to each other (including the prefix generated by the compiler plugin): @@ -186,7 +196,7 @@ class Example6 extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example6) +emitSystemVerilog(new Example6) ``` Sometimes you may want to disable the prefixing. This might occur if you are writing a library function and @@ -203,7 +213,7 @@ class Example7 extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example7) +emitSystemVerilog(new Example7) ``` ### Suggest a Signal's Name (or the instance name of a Module) @@ -222,7 +232,7 @@ class Example8 extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example8) +emitSystemVerilog(new Example8) ``` Note that using `.suggestName` does **not** affect prefixes derived from val names; @@ -265,7 +275,7 @@ class ConnectionPrefixExample extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new ConnectionPrefixExample) +emitSystemVerilog(new ConnectionPrefixExample) ``` As this example illustrates, this behavior is slightly inconsistent so is subject to change in a future version of Chisel. @@ -291,7 +301,7 @@ class TemporaryExample extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new TemporaryExample) +emitSystemVerilog(new TemporaryExample) ``` If an unnamed signal is itself used to generate a prefix, the leading `_` will be ignored to avoid double `__` in the names of further nested signals. @@ -311,7 +321,7 @@ class TemporaryPrefixExample extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new TemporaryPrefixExample) +emitSystemVerilog(new TemporaryPrefixExample) ``` @@ -333,8 +343,8 @@ class Example9(width: Int) extends Module { } ``` ```scala mdoc:verilog -ChiselStage.emitSystemVerilog(new Example9(8)) -ChiselStage.emitSystemVerilog(new Example9(1)) +emitSystemVerilog(new Example9(8)) +emitSystemVerilog(new Example9(1)) ``` ### Reflection Naming diff --git a/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala index dd9f24fb6a2..1daabcbde1e 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala @@ -233,7 +233,8 @@ class ChiselComponent(val global: Global, arguments: ChiselPluginArguments) 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)" + val newRHS = transform(rhs) + val named = q"chisel3.internal.plugin.autoNameRecursivelyProduct($onames)($newRHS)" 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) diff --git a/src/test/scala/chiselTests/naming/NamePluginSpec.scala b/src/test/scala/chiselTests/naming/NamePluginSpec.scala index c0b7c74bdca..5d878d85653 100644 --- a/src/test/scala/chiselTests/naming/NamePluginSpec.scala +++ b/src/test/scala/chiselTests/naming/NamePluginSpec.scala @@ -267,6 +267,22 @@ class NamePluginSpec extends ChiselFlatSpec with Utils { } } + "Unapply assignments" should "name (but not prefix) local vals on the RHS" in { + class Test extends Module { + { + val (a, b) = { + val x, y = Wire(UInt(3.W)) + val sum = WireInit(x + y) + (x, y) + } + } + } + + aspectTest(() => new Test) { top: Test => + Select.wires(top).map(_.instanceName) should be(List("a", "b", "sum")) + } + } + "Unapply assignments" should "not override already named things" in { class Test extends Module { { From f446fff13dc89447d8c79fcd0a0d14d248e6db17 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 12 Apr 2023 22:07:42 +0000 Subject: [PATCH 2/3] Report firtool version when firtool invocation errors (backport #3174) (#3175) * Report firtool version when firtool invocation errors (#3174) * the firtool version is now included in the BuildInfo * firtool now must be on the path when publishing, but is not required for building or testing * FirtoolNonZeroExitCode now no longer includes a stack trace (cherry picked from commit 06b569daa5e5c2ac1fd00dbac60b0d5d852623d5) # Conflicts: # build.sbt * Resolve backport conflicts * [CI] Install firtool in the publish step (#3176) --------- Co-authored-by: Jack Koenig --- .github/workflows/test.yml | 2 ++ build.sbt | 24 ++++++++++++++++++- src/main/scala/circt/stage/phases/CIRCT.scala | 9 ++++++- .../circtTests/stage/ChiselStageSpec.scala | 7 ++++++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index fe3d5f03620..ccb542dfe0e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -195,6 +195,8 @@ jobs: steps: - name: Checkout uses: actions/checkout@v3 + - name: Install CIRCT + uses: ./.github/workflows/install-circt - name: Setup Scala uses: actions/setup-java@v3 with: diff --git a/build.sbt b/build.sbt index ab0ee3f596b..b2b1b99c253 100644 --- a/build.sbt +++ b/build.sbt @@ -5,6 +5,18 @@ enablePlugins(SiteScaladocPlugin) addCommandAlias("fmt", "; scalafmtAll ; scalafmtSbt") addCommandAlias("fmtCheck", "; scalafmtCheckAll ; scalafmtSbtCheck") +lazy val firtoolVersion = settingKey[Option[String]]("Determine the version of firtool on the PATH") +ThisBuild / firtoolVersion := { + import scala.sys.process._ + val Version = """^CIRCT firtool-(\S+)$""".r + try { + val lines = Process(Seq("firtool", "--version")).lineStream + lines.collectFirst { case Version(v) => v } + } catch { + case e: java.io.IOException => None + } +} + val defaultVersions = Map( "firrtl" -> "edu.berkeley.cs" %% "firrtl" % "1.6-SNAPSHOT" // chiseltest intentionally excluded so that release automation does not try to set its version @@ -69,6 +81,16 @@ lazy val publishSettings = Seq( versionScheme := Some("pvp"), publishMavenStyle := true, Test / publishArtifact := false, + // We are just using 'publish / skip' as a hook to run checks required for publishing, + // but that are not necessarily required for local development or running testing in CI + publish / skip := { + // Check that firtool exists on the PATH so Chisel can use the version it was tested against + // in error messages + if (firtoolVersion.value.isEmpty) { + sys.error(s"Failed to determine firtool version. Make sure firtool is found on the PATH.") + } + (publish / skip).value + }, pomIncludeRepository := { x => false }, pomExtra := http://chisel.eecs.berkeley.edu/ @@ -197,7 +219,7 @@ lazy val core = (project in file("core")) .settings( buildInfoPackage := "chisel3", buildInfoUsePackageAsPath := true, - buildInfoKeys := Seq[BuildInfoKey](buildInfoPackage, version, scalaVersion, sbtVersion) + buildInfoKeys := Seq[BuildInfoKey](buildInfoPackage, version, scalaVersion, sbtVersion, firtoolVersion) ) .settings(publishSettings: _*) .settings(mimaPreviousArtifacts := Set()) diff --git a/src/main/scala/circt/stage/phases/CIRCT.scala b/src/main/scala/circt/stage/phases/CIRCT.scala index 4c92cc294f6..f02a3224c62 100644 --- a/src/main/scala/circt/stage/phases/CIRCT.scala +++ b/src/main/scala/circt/stage/phases/CIRCT.scala @@ -4,6 +4,7 @@ package circt.stage.phases import chisel3.experimental.hierarchy.core.ImportDefinitionAnnotation import chisel3.stage.{ChiselCircuitAnnotation, DesignAnnotation, SourceRootAnnotation} +import chisel3.BuildInfo.{version => chiselVersion, firtoolVersion} import circt.Implicits.BooleanImplicits import circt.stage.{CIRCTOptions, CIRCTTarget, EmittedMLIR, PreserveAggregate} @@ -26,6 +27,7 @@ import firrtl.stage.{FirrtlOptions, RunFirrtlTransformAnnotation} import _root_.logger.LogLevel import chisel3.InternalErrorException +import scala.util.control.NoStackTrace import scala.collection.mutable import java.io.File @@ -81,6 +83,10 @@ private[this] object Exceptions { |${"-" * 78}""".stripMargin } + def versionAdvice: String = + s"Note that this version of Chisel ($chiselVersion) was published against firtool version " + + firtoolVersion.getOrElse("") + "." + /** Indicates that the firtool binary failed with a non-zero exit code. This generally indicates a compiler error * either originating from a user error or from a crash. * @@ -92,10 +98,11 @@ private[this] object Exceptions { class FirtoolNonZeroExitCode(binary: String, exitCode: Int, stdout: String, stderr: String) extends RuntimeException( dramaticError( - header = s"${binary} returned a non-zero exit code", + header = s"${binary} returned a non-zero exit code. $versionAdvice", body = s"ExitCode:\n${exitCode}\nSTDOUT:\n${stdout}\nSTDERR:\n${stderr}" ) ) + with NoStackTrace /** Indicates that the firtool binary was not found. This likely indicates that the user didn't install * CIRCT/firtool. diff --git a/src/test/scala/circtTests/stage/ChiselStageSpec.scala b/src/test/scala/circtTests/stage/ChiselStageSpec.scala index 0bc1196a9ac..62c9bc1e52f 100644 --- a/src/test/scala/circtTests/stage/ChiselStageSpec.scala +++ b/src/test/scala/circtTests/stage/ChiselStageSpec.scala @@ -503,6 +503,13 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.Utils { lines(idx + 2) should equal(" ^") } + it("should report the firtool version against which Chisel was published in error messages") { + val e = intercept[java.lang.Exception] { + ChiselStage.emitSystemVerilog(new ChiselStageSpec.ErrorCaughtByFirtool) + } + val version = chisel3.BuildInfo.firtoolVersion.getOrElse("") + e.getMessage should include(s"firtool version $version") + } } describe("ChiselStage custom transform support") { From 9ebf28de8a72f5b5ceb8808b138ec7e73557882b Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 13 Apr 2023 00:30:25 +0000 Subject: [PATCH 3/3] Added .exclude to Connectable (#3172) (#3178) (cherry picked from commit 4b891ecfed0fc96f935ea042a0a02b0c17ac75d9) Co-authored-by: Adam Izraelevitz --- core/src/main/scala/chisel3/Data.scala | 87 +++++------ .../scala/chisel3/connectable/Alignment.scala | 5 +- .../chisel3/connectable/Connectable.scala | 53 +++++-- .../chisel3/connectable/Connection.scala | 82 +++++----- docs/src/explanations/connectable.md | 39 ++++- .../scala/chiselTests/ConnectableSpec.scala | 145 ++++++++++++++++++ 6 files changed, 316 insertions(+), 95 deletions(-) diff --git a/core/src/main/scala/chisel3/Data.scala b/core/src/main/scala/chisel3/Data.scala index 96cd5f9549e..47430b01eb7 100644 --- a/core/src/main/scala/chisel3/Data.scala +++ b/core/src/main/scala/chisel3/Data.scala @@ -830,51 +830,52 @@ object Data { * * Only zips immediate children (vs members, which are all children/grandchildren etc.) */ - implicit private[chisel3] val DataMatchingZipOfChildren = new DataMirror.HasMatchingZipOfChildren[Data] { + implicit val dataMatchingZipOfChildren: DataMirror.HasMatchingZipOfChildren[Data] = + new DataMirror.HasMatchingZipOfChildren[Data] { - implicit class VecOptOps(vOpt: Option[Vec[Data]]) { - // Like .get, but its already defined on Option - def grab(i: Int): Option[Data] = vOpt.flatMap { _.lift(i) } - def size = vOpt.map(_.size).getOrElse(0) - } - implicit class RecordOptGet(rOpt: Option[Record]) { - // Like .get, but its already defined on Option - def grab(k: String): Option[Data] = rOpt.flatMap { _._elements.get(k) } - def keys: Iterable[String] = rOpt.map { r => r._elements.map(_._1) }.getOrElse(Seq.empty[String]) - } - //TODO(azidar): Rewrite this to be more clear, probably not the cleanest way to express this - private def isDifferent(l: Option[Data], r: Option[Data]): Boolean = - l.nonEmpty && r.nonEmpty && !isRecord(l, r) && !isVec(l, r) && !isElement(l, r) - private def isRecord(l: Option[Data], r: Option[Data]): Boolean = - l.orElse(r).map { _.isInstanceOf[Record] }.getOrElse(false) - private def isVec(l: Option[Data], r: Option[Data]): Boolean = - l.orElse(r).map { _.isInstanceOf[Vec[_]] }.getOrElse(false) - private def isElement(l: Option[Data], r: Option[Data]): Boolean = - l.orElse(r).map { _.isInstanceOf[Element] }.getOrElse(false) - - /** Zips matching children of `left` and `right`; returns Nil if both are empty - * - * The canonical API to iterate through two Chisel types or components, where - * matching children are provided together, while non-matching members are provided - * separately - * - * Only zips immediate children (vs members, which are all children/grandchildren etc.) - * - * Returns Nil if both are different types - */ - def matchingZipOfChildren(left: Option[Data], right: Option[Data]): Seq[(Option[Data], Option[Data])] = - (left, right) match { - case (None, None) => Nil - case (lOpt, rOpt) if isDifferent(lOpt, rOpt) => Nil - case (lOpt: Option[Vec[Data] @unchecked], rOpt: Option[Vec[Data] @unchecked]) if isVec(lOpt, rOpt) => - (0 until (lOpt.size.max(rOpt.size))).map { i => (lOpt.grab(i), rOpt.grab(i)) } - case (lOpt: Option[Record @unchecked], rOpt: Option[Record @unchecked]) if isRecord(lOpt, rOpt) => - (lOpt.keys ++ rOpt.keys).toList.distinct.map { k => (lOpt.grab(k), rOpt.grab(k)) } - case (lOpt: Option[Element @unchecked], rOpt: Option[Element @unchecked]) if isElement(lOpt, rOpt) => Nil - case _ => - throw new InternalErrorException(s"Match Error: left=$left, right=$right") + implicit class VecOptOps(vOpt: Option[Vec[Data]]) { + // Like .get, but its already defined on Option + def grab(i: Int): Option[Data] = vOpt.flatMap { _.lift(i) } + def size = vOpt.map(_.size).getOrElse(0) } - } + implicit class RecordOptGet(rOpt: Option[Record]) { + // Like .get, but its already defined on Option + def grab(k: String): Option[Data] = rOpt.flatMap { _._elements.get(k) } + def keys: Iterable[String] = rOpt.map { r => r._elements.map(_._1) }.getOrElse(Seq.empty[String]) + } + //TODO(azidar): Rewrite this to be more clear, probably not the cleanest way to express this + private def isDifferent(l: Option[Data], r: Option[Data]): Boolean = + l.nonEmpty && r.nonEmpty && !isRecord(l, r) && !isVec(l, r) && !isElement(l, r) + private def isRecord(l: Option[Data], r: Option[Data]): Boolean = + l.orElse(r).map { _.isInstanceOf[Record] }.getOrElse(false) + private def isVec(l: Option[Data], r: Option[Data]): Boolean = + l.orElse(r).map { _.isInstanceOf[Vec[_]] }.getOrElse(false) + private def isElement(l: Option[Data], r: Option[Data]): Boolean = + l.orElse(r).map { _.isInstanceOf[Element] }.getOrElse(false) + + /** Zips matching children of `left` and `right`; returns Nil if both are empty + * + * The canonical API to iterate through two Chisel types or components, where + * matching children are provided together, while non-matching members are provided + * separately + * + * Only zips immediate children (vs members, which are all children/grandchildren etc.) + * + * Returns Nil if both are different types + */ + def matchingZipOfChildren(left: Option[Data], right: Option[Data]): Seq[(Option[Data], Option[Data])] = + (left, right) match { + case (None, None) => Nil + case (lOpt, rOpt) if isDifferent(lOpt, rOpt) => Nil + case (lOpt: Option[Vec[Data] @unchecked], rOpt: Option[Vec[Data] @unchecked]) if isVec(lOpt, rOpt) => + (0 until (lOpt.size.max(rOpt.size))).map { i => (lOpt.grab(i), rOpt.grab(i)) } + case (lOpt: Option[Record @unchecked], rOpt: Option[Record @unchecked]) if isRecord(lOpt, rOpt) => + (lOpt.keys ++ rOpt.keys).toList.distinct.map { k => (lOpt.grab(k), rOpt.grab(k)) } + case (lOpt: Option[Element @unchecked], rOpt: Option[Element @unchecked]) if isElement(lOpt, rOpt) => Nil + case _ => + throw new InternalErrorException(s"Match Error: left=$left, right=$right") + } + } /** * Provides generic, recursive equality for [[Bundle]] and [[Vec]] hardware. This avoids the diff --git a/core/src/main/scala/chisel3/connectable/Alignment.scala b/core/src/main/scala/chisel3/connectable/Alignment.scala index 39250bab3c3..30bc7ddc472 100644 --- a/core/src/main/scala/chisel3/connectable/Alignment.scala +++ b/core/src/main/scala/chisel3/connectable/Alignment.scala @@ -73,6 +73,8 @@ private[chisel3] sealed trait Alignment { final def isSqueezed: Boolean = base.squeezed.contains(member) + final def isExcluded: Boolean = base.excluded.contains(member) + // Whether the current member is an aggregate final def isAgg: Boolean = member.isInstanceOf[Aggregate] @@ -94,6 +96,7 @@ private[chisel3] case class EmptyAlignment(base: Connectable[Data], isConsumer: def member = DontCare def waived = Set.empty def squeezed = Set.empty + def excluded = Set.empty def invert = this def coerced = false def coerce = this @@ -167,7 +170,7 @@ object Alignment { left: Option[Alignment], right: Option[Alignment] ): Seq[(Option[Alignment], Option[Alignment])] = { - Data.DataMatchingZipOfChildren.matchingZipOfChildren(left.map(_.member), right.map(_.member)).map { + Data.dataMatchingZipOfChildren.matchingZipOfChildren(left.map(_.member), right.map(_.member)).map { case (l, r) => l.map(deriveChildAlignment(_, left.get)) -> r.map(deriveChildAlignment(_, right.get)) } } diff --git a/core/src/main/scala/chisel3/connectable/Connectable.scala b/core/src/main/scala/chisel3/connectable/Connectable.scala index cb30f7079d7..6cb4ba65fd9 100644 --- a/core/src/main/scala/chisel3/connectable/Connectable.scala +++ b/core/src/main/scala/chisel3/connectable/Connectable.scala @@ -17,14 +17,19 @@ import experimental.{prefix, requireIsHardware} final class Connectable[+T <: Data] private ( val base: T, private[chisel3] val waived: Set[Data], - private[chisel3] val squeezed: Set[Data]) { + private[chisel3] val squeezed: Set[Data], + private[chisel3] val excluded: Set[Data]) { requireIsHardware(base, s"Can only created Connectable of components, not unbound Chisel types") - /** True if no members are waived or squeezed */ - def notSpecial = waived.isEmpty && squeezed.isEmpty + /** True if no members are waived or squeezed or excluded */ + def notWaivedOrSqueezedOrExcluded = waived.isEmpty && squeezed.isEmpty && excluded.isEmpty - private[chisel3] def copy(waived: Set[Data] = this.waived, squeezed: Set[Data] = this.squeezed): Connectable[T] = - new Connectable(base, waived, squeezed) + private[chisel3] def copy( + waived: Set[Data] = this.waived, + squeezed: Set[Data] = this.squeezed, + excluded: Set[Data] = this.excluded + ): Connectable[T] = + new Connectable(base, waived, squeezed, excluded) /** Select members of base to waive * @@ -83,6 +88,31 @@ final class Connectable[+T <: Data] private ( this.copy(squeezed = squeezedMembers.toSet) // not appending squeezed because we are collecting all members } + /** Adds base to excludes */ + def exclude: Connectable[T] = this.copy(excluded = excluded ++ addOpaque(Seq(base))) + + /** Select members of base to exclude + * + * @param members functions given the base return a member to exclude + */ + def exclude(members: (T => Data)*): Connectable[T] = this.copy(excluded = excluded ++ members.map(f => f(base)).toSet) + + /** Select members of base to exclude and static cast to a new type + * + * @param members functions given the base return a member to exclude + */ + def excludeAs[S <: Data](members: (T => Data)*)(implicit ev: T <:< S): Connectable[S] = + this.copy(excluded = excluded ++ members.map(f => f(base)).toSet).asInstanceOf[Connectable[S]] + + /** Programmatically select members of base to exclude and static cast to a new type + * + * @param members partial function applied to all recursive members of base, if match, can return a member to exclude + */ + def excludeEach[S <: Data](pf: PartialFunction[Data, Seq[Data]])(implicit ev: T <:< S): Connectable[S] = { + val excludedMembers = DataMirror.collectMembers(base)(pf).flatten + this.copy(excluded = excluded ++ excludedMembers.toSet).asInstanceOf[Connectable[S]] + } + /** Add any elements of members that are OpaqueType */ private def addOpaque(members: Seq[Data]): Seq[Data] = { members.flatMap { @@ -98,13 +128,18 @@ object Connectable { def apply[T <: Data]( base: T, waiveSelection: Data => Boolean = { _ => false }, - squeezeSelection: Data => Boolean = { _ => false } + squeezeSelection: Data => Boolean = { _ => false }, + excludeSelection: Data => Boolean = { _ => false } ): Connectable[T] = { - val (waived, squeezed) = { + val (waived, squeezed, excluded) = { val members = DataMirror.collectMembers(base) { case x => x } - (members.filter(waiveSelection).toSet, members.filter(squeezeSelection).toSet) + ( + members.filter(waiveSelection).toSet, + members.filter(squeezeSelection).toSet, + members.filter(excludeSelection).toSet + ) } - new Connectable(base, waived, squeezed) + new Connectable(base, waived, squeezed, excluded) } /** The default connection operators for Chisel hardware components diff --git a/core/src/main/scala/chisel3/connectable/Connection.scala b/core/src/main/scala/chisel3/connectable/Connection.scala index 5b2360bf52d..7f2f7306152 100644 --- a/core/src/main/scala/chisel3/connectable/Connection.scala +++ b/core/src/main/scala/chisel3/connectable/Connection.scala @@ -70,7 +70,7 @@ private[chisel3] case object ColonLessGreaterEq extends Connection { // when calling DirectionConnection.connect. Hence, we can just default to false to take the non-optimized emission path case e: Throwable => false } - (typeEquivalent && consumer.notSpecial && producer.notSpecial) + (typeEquivalent && consumer.notWaivedOrSqueezedOrExcluded && producer.notWaivedOrSqueezedOrExcluded) } } @@ -173,76 +173,84 @@ private[chisel3] object Connection { import Alignment.deriveChildAlignment def doConnection( - consumerAlignment: Alignment, - producerAlignment: Alignment + conAlign: Alignment, + proAlign: Alignment )( implicit sourceInfo: SourceInfo ): Unit = { - (consumerAlignment, producerAlignment) match { + (conAlign, proAlign) match { // Base Case 0: should probably never happen case (_: EmptyAlignment, _: EmptyAlignment) => () - // Base Case 1: early exit if dangling/unconnected is wavied - case (consumerAlignment: NonEmptyAlignment, _: EmptyAlignment) if consumerAlignment.isWaived => () - case (_: EmptyAlignment, producerAlignment: NonEmptyAlignment) if producerAlignment.isWaived => () + // Base Case 1: early exit if dangling/unconnected is excluded + case (conAlign: Alignment, proAlign: Alignment) if conAlign.isExcluded && proAlign.isExcluded => () - // Base Case 2: early exit if operator requires matching orientations, but they don't align - case (consumerAlignment: NonEmptyAlignment, producerAlignment: NonEmptyAlignment) - if (!consumerAlignment.alignsWith(producerAlignment)) && (connectionOp.noWrongOrientations) => - errors = (s"inversely oriented fields ${consumerAlignment.member} and ${producerAlignment.member}") +: errors + // Base Case 2(A,B): early exit if dangling/unconnected is wavied or excluded + case (conAlign: NonEmptyAlignment, _: EmptyAlignment) if conAlign.isWaived || conAlign.isExcluded => () + case (_: EmptyAlignment, proAlign: NonEmptyAlignment) if proAlign.isWaived || proAlign.isExcluded => () - // Base Case 3: early exit if operator requires matching widths, but they aren't the same - case (consumerAlignment: NonEmptyAlignment, producerAlignment: NonEmptyAlignment) - if (consumerAlignment - .truncationRequired(producerAlignment, connectionOp) - .nonEmpty) && (connectionOp.noMismatchedWidths) => - val mustBeTruncated = consumerAlignment.truncationRequired(producerAlignment, connectionOp).get + // Base Case 3: early exit if dangling/unconnected is wavied + case (conAlign: NonEmptyAlignment, proAlign: NonEmptyAlignment) if conAlign.isExcluded || proAlign.isExcluded => + val (excluded, included) = + if (conAlign.isExcluded) (conAlign, proAlign) + else (proAlign, conAlign) + errors = (s"excluded field ${excluded.member} has matching non-excluded field ${included.member}") +: errors + + // Base Case 4: early exit if operator requires matching orientations, but they don't align + case (conAlign: NonEmptyAlignment, proAlign: NonEmptyAlignment) + if (!conAlign.alignsWith(proAlign)) && (connectionOp.noWrongOrientations) => + errors = (s"inversely oriented fields ${conAlign.member} and ${proAlign.member}") +: errors + + // Base Case 5: early exit if operator requires matching widths, but they aren't the same + case (conAlign: NonEmptyAlignment, proAlign: NonEmptyAlignment) + if (conAlign.truncationRequired(proAlign, connectionOp).nonEmpty) && (connectionOp.noMismatchedWidths) => + val mustBeTruncated = conAlign.truncationRequired(proAlign, connectionOp).get errors = - (s"mismatched widths of ${consumerAlignment.member} and ${producerAlignment.member} might require truncation of $mustBeTruncated") +: errors + (s"mismatched widths of ${conAlign.member} and ${proAlign.member} might require truncation of $mustBeTruncated") +: errors - // Base Case 3: operator error on dangling/unconnected fields + // Base Case 6: operator error on dangling/unconnected fields case (consumer: NonEmptyAlignment, _: EmptyAlignment) => - errors = (s"${consumer.errorWord(connectionOp)} consumer field ${consumerAlignment.member}") +: errors + errors = (s"${consumer.errorWord(connectionOp)} consumer field ${conAlign.member}") +: errors case (_: EmptyAlignment, producer: NonEmptyAlignment) => - errors = (s"${producer.errorWord(connectionOp)} producer field ${producerAlignment.member}") +: errors + errors = (s"${producer.errorWord(connectionOp)} producer field ${proAlign.member}") +: errors // Recursive Case 4: non-empty orientations - case (consumerAlignment: NonEmptyAlignment, producerAlignment: NonEmptyAlignment) => - (consumerAlignment.member, producerAlignment.member) match { + case (conAlign: NonEmptyAlignment, proAlign: NonEmptyAlignment) => + (conAlign.member, proAlign.member) match { case (consumer: Aggregate, producer: Aggregate) => - matchingZipOfChildren(Some(consumerAlignment), Some(producerAlignment)).foreach { + matchingZipOfChildren(Some(conAlign), Some(proAlign)).foreach { case (ceo, peo) => - doConnection(ceo.getOrElse(consumerAlignment.empty), peo.getOrElse(producerAlignment.empty)) + doConnection(ceo.getOrElse(conAlign.empty), peo.getOrElse(proAlign.empty)) } case (consumer: Aggregate, DontCare) => consumer.getElements.foreach { case f => doConnection( - deriveChildAlignment(f, consumerAlignment), - deriveChildAlignment(f, consumerAlignment).swap(DontCare) + deriveChildAlignment(f, conAlign), + deriveChildAlignment(f, conAlign).swap(DontCare) ) } case (DontCare, producer: Aggregate) => producer.getElements.foreach { case f => doConnection( - deriveChildAlignment(f, producerAlignment).swap(DontCare), - deriveChildAlignment(f, producerAlignment) + deriveChildAlignment(f, proAlign).swap(DontCare), + deriveChildAlignment(f, proAlign) ) } case (consumer, producer) => val alignment = ( - consumerAlignment.alignsWith(producerAlignment), - (!consumerAlignment.alignsWith( - producerAlignment + conAlign.alignsWith(proAlign), + (!conAlign.alignsWith( + proAlign ) && connectionOp.connectToConsumer && !connectionOp.connectToProducer), - (!consumerAlignment.alignsWith( - producerAlignment + (!conAlign.alignsWith( + proAlign ) && !connectionOp.connectToConsumer && connectionOp.connectToProducer) ) match { - case (true, _, _) => consumerAlignment - case (_, true, _) => consumerAlignment - case (_, _, true) => producerAlignment + case (true, _, _) => conAlign + case (_, true, _) => conAlign + case (_, _, true) => proAlign case other => throw new Exception(other.toString) } val lAndROpt = alignment.computeLandR(consumer, producer, connectionOp) diff --git a/docs/src/explanations/connectable.md b/docs/src/explanations/connectable.md index e322dc640c6..d6a4e4dec77 100644 --- a/docs/src/explanations/connectable.md +++ b/docs/src/explanations/connectable.md @@ -397,7 +397,7 @@ For example, a user may want to hook up anonymous `Record` components who may ha Alternatively, one may want to connect two types that have different widths. `Connectable` is the mechanism to specialize connection operator behavior in these scenarios. -For additional members which are not present in the other component being connected to or for mismatched widths, they can be explicitly waived from the operator to be ignored, rather than trigger an error. +For additional members which are not present in the other component being connected to, or for mismatched widths, or for always excluding a member from being connected too, they can be explicitly called out from the `Connectable` object, rather than trigger an error. In addition, there are other techniques that can be used to address similar use cases including `.viewAsSuperType`, a static cast to a supertype (e.g. `(x: T)`), or creating a custom dataview. For a discussion about when to use each technique, please continue [here](#techniques-for-connecting-structurally-inequivalent-chisel-types). @@ -406,7 +406,7 @@ This section demonstrates how `Connectable` specifically can be used in a multit ### Connecting Records -A not uncommon usecase is to try to connect two Records; for matching members, they should be connected, but for unmatched members, they should be ignored. +A not uncommon usecase is to try to connect two Records; for matching members, they should be connected, but for unmatched members, the errors caused due to them being unmatched should be ignored. To accomplish this, use the other operators to initialize all Record members, then use `:<>=` with `waiveAll` to connect only the matching members. > Note that none of `.viewAsSuperType`, static casts, nor a custom DataView helps this case because the Scala types are still `Record`. @@ -489,7 +489,7 @@ This generates the following Verilog, where `ready` and `valid` are connected, a getVerilogString(new Example6) ``` -### Always ignore extra members (partial connection operator) +### Always ignore errors caused by extra members (partial connection operator) The most unsafe connection is to connect only members that are present in both consumer and producer, and ignore all other members. This is unsafe because this connection will never error on any Chisel types. @@ -543,6 +543,35 @@ This generates the following Verilog, where `p` is implicitly truncated prior to getVerilogString(new Example14) ``` +### Excluding members from any operator on a Connectable + +If a user wants to always exclude a field from a connect, use the `exclude` mechanism which will never connect the field (as if it didn't exist to the connection). + +Note that if a field matches in both producer and consumer, but only one is excluded, the other non-excluded field will still trigger an error; to fix this, use either `waive` or `exclude`. + +```scala mdoc:silent +import scala.collection.immutable.SeqMap + +class BundleWithSpecialField extends Bundle { + val foo = UInt(3.W) + val special = Bool() +} +class Example15 extends RawModule { + val p = IO(Flipped(new BundleWithSpecialField())) + val c = IO(new BundleWithSpecialField()) + + c.special := true.B // must initialize it + + c.exclude(_.special) :<>= p.exclude(_.special) +} +``` + +This generates the following Verilog, where the `special` field is not connected: + +```scala mdoc:verilog +getVerilogString(new Example15) +``` + ## Techniques for connecting structurally inequivalent Chisel types `DataView` and `viewAsSupertype` create a view of the component that has a different Chisel type. @@ -566,7 +595,7 @@ Things to remember about `Connectable` vs `viewAsSupertype`/`DataView` vs static - `DataView` and `viewAsSupertype` will preemptively remove members that are not present in the new view which has a different Chisel type, thus `DataView` *does* affect what is connected - `Connectable` can be used to waive the error on members who end up being dangling or unconnected. -Importantly, `Connectable` *does not* affect what is connected +Importantly, `Connectable` waives *do not* affect what is connected - Static cast does not remove extra members, thus a static cast *does not* affect what is connected ### Connecting different sub-types of the same super-type, with colliding names @@ -617,7 +646,7 @@ class Example13 extends RawModule { } ``` -Note that the `bits` fields ARE connected, even though they are waived, as `waive` just changes whether an error should be thrown if they are missing, NOT to not connect them if they are structurally equivalent. +Note that the `bits` fields ARE connected, even though they are waived, as `waive` just changes whether an error should be thrown if they are missing, NOT to not connect them if they are structurally equivalent. To always omit the connection, use `exclude` on one side and either `exclude` or `waive` on the other side. ```scala mdoc:verilog getVerilogString(new Example13) diff --git a/src/test/scala/chiselTests/ConnectableSpec.scala b/src/test/scala/chiselTests/ConnectableSpec.scala index 57288121f81..e7d7ee17f59 100644 --- a/src/test/scala/chiselTests/ConnectableSpec.scala +++ b/src/test/scala/chiselTests/ConnectableSpec.scala @@ -1259,6 +1259,151 @@ class ConnectableSpec extends ChiselFunSpec with Utils { ) } } + describe("(E): Connectable excluding") { + import scala.collection.immutable.SeqMap + class Decoupled(val hasBigData: Boolean) extends Bundle { + val valid = Bool() + val ready = Flipped(Bool()) + val data = if (hasBigData) UInt(32.W) else UInt(8.W) + } + class BundleMap(fields: SeqMap[String, () => Data]) extends Record { + val elements = fields.map { case (name, gen) => name -> gen() } + } + object BundleMap { + def onlyIncludeUnion[T <: Data](x: T, y: T): (Connectable[T], Connectable[T]) = { + val xFields = collection.mutable.ArrayBuffer[Data]() + val yFields = collection.mutable.ArrayBuffer[Data]() + DataMirror.collectMembersOverAll(x.asInstanceOf[Data], y.asInstanceOf[Data]) { + case (Some(a), None) => xFields += a + case (None, Some(a)) => yFields += a + } + (Connectable(x, Set.empty, Set.empty, xFields.toSet), Connectable(y, Set.empty, Set.empty, yFields.toSet)) + } + } + class DecoupledGen[T <: Data](val gen: () => T) extends Bundle { + val valid = Bool() + val ready = Flipped(Bool()) + val data = gen() + } + it("(E.a) Using exclude works for nested field") { + class NestedDecoupled(val hasBigData: Boolean) extends Bundle { + val foo = new Decoupled(hasBigData) + } + class MyModule extends Module { + val in = IO(Flipped(new NestedDecoupled(true))) + val out = IO(new NestedDecoupled(false)) + out.excludeEach { case d: Decoupled => Seq(d.data) } :<>= in.excludeEach { case d: Decoupled => Seq(d.data) } + } + testCheck( + ChiselStage.emitCHIRRTL({ new MyModule() }, args = Array("--full-stacktrace", "--throw-on-first-error")), + Seq( + "out.foo.valid <= in.foo.valid", + "in.foo.ready <= out.foo.ready" + ), + Seq( + "out.foo.data <= in.foo.data" + ) + ) + } + it("(E.b) exclude works on UInt") { + class MyModule extends Module { + val in = IO(Flipped(UInt(3.W))) + val out = IO(UInt(1.W)) + out.exclude :<>= in.exclude + } + testCheck( + ChiselStage.emitCHIRRTL({ new MyModule() }), + Nil, + Seq( + "out <= in" + ) + ) + } + it("(E.c) BundleMap example can use programmatic excluding") { + class MyModule extends Module { + def ab = new BundleMap( + SeqMap( + "a" -> (() => UInt(2.W)), + "b" -> (() => UInt(2.W)) + ) + ) + def bc = new BundleMap( + SeqMap( + "b" -> (() => UInt(2.W)), + "c" -> (() => UInt(2.W)) + ) + ) + val in = IO(Flipped(new DecoupledGen(() => ab))) + val out = IO(new DecoupledGen(() => bc)) + //Programmatic + val (cout, cin) = BundleMap.onlyIncludeUnion(out, in) + cout :<>= cin + } + testCheck( + ChiselStage.emitCHIRRTL({ new MyModule() }), + Seq( + "out.valid <= in.valid", + "in.ready <= out.ready", + "out.data.b <= in.data.b" + ), + Nil + ) + } + it("(E.e) Mismatched aggregate containing backpressure can be excluded only if actually connecting") { + class OnlyBackPressure(width: Int) extends Bundle { + val ready = Flipped(UInt(width.W)) + } + class MyModule extends Module { + // Have to nest in bundle because it calls the connecting-to-seq version + val in2 = IO(Flipped(new Bundle { val v = Vec(2, new OnlyBackPressure(2)) })) + val out3 = IO(new Bundle { val v = Vec(3, new OnlyBackPressure(2)) }) + // Should do nothing, but also doesn't error, which is good + out3.exclude(_.v(2)) :<= in2 + out3.exclude(_.v(2)) :>= in2 + } + testCheck( + ChiselStage.emitCHIRRTL({ new MyModule() }), + Seq( + "in2.v[0].ready <= out3.v[0].ready", + "in2.v[1].ready <= out3.v[1].ready" + ), + Nil + ) + } + it("(E.f) exclude works on OpaqueType") { + class OpaqueRecord(width: Int) extends Record with OpaqueType { + private val underlying = UInt(width.W) + val elements = SeqMap("" -> underlying) + } + class MyModule extends Module { + val in = IO(Input(new OpaqueRecord(4))) + val out = IO(Output(new OpaqueRecord(2))) + out.exclude :<>= in.exclude + } + testCheck( + ChiselStage.emitCHIRRTL({ new MyModule() }), + Nil, + Seq( + "out <= in" + ) + ) + } + it("(E.g) matched fields with one side excluded errors") { + class MyModule extends Module { + val in = IO(Flipped(new Decoupled(true))) + val out = IO(new Decoupled(true)) + out.exclude(_.data) :<>= in + } + val e = intercept[ChiselException] { + ChiselStage.emitCHIRRTL({ new MyModule() }, args = Array("--full-stacktrace", "--throw-on-first-error")) + } + assert( + e.getMessage.contains( + "excluded field MyModule.out.data: IO[UInt<32>] has matching non-excluded field MyModule.in.data: IO[UInt<32>]" + ) + ) + } + } describe("(6): Connectable and DataView") { it("(6.o) :<>= works with DataView to connect a bundle that is a subtype") { import chisel3.experimental.dataview._