From 3f485634f52e7eb069bae8a64eacc86042a39d38 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Tue, 11 Jan 2022 21:16:34 -0800 Subject: Set mimaPreviousArtifacts for binary compatibility checking --- build.sbt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/build.sbt b/build.sbt index 7cad3e6d..b9406c8f 100644 --- a/build.sbt +++ b/build.sbt @@ -134,7 +134,7 @@ lazy val plugin = (project in file("plugin")). ). settings( mimaPreviousArtifacts := { - Set() + Set("edu.berkeley.cs" % "chisel3-plugin" % "3.5.0" cross CrossVersion.full) } ) @@ -153,7 +153,7 @@ lazy val macros = (project in file("macros")). settings(name := "chisel3-macros"). settings(commonSettings: _*). settings(publishSettings: _*). - settings(mimaPreviousArtifacts := Set()) + settings(mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3-macros" % "3.5.0")) lazy val firrtlRef = ProjectRef(workspaceDirectory / "firrtl", "firrtl") @@ -167,7 +167,7 @@ lazy val core = (project in file("core")). buildInfoKeys := Seq[BuildInfoKey](buildInfoPackage, version, scalaVersion, sbtVersion) ). settings(publishSettings: _*). - settings(mimaPreviousArtifacts := Set()). + settings(mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3-core" % "3.5.0")). settings( name := "chisel3-core", scalacOptions := scalacOptions.value ++ Seq( @@ -196,7 +196,7 @@ lazy val chisel = (project in file(".")). dependsOn(core). aggregate(macros, core, plugin). settings( - mimaPreviousArtifacts := Set(), + mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3" % "3.5.0"), libraryDependencies += defaultVersions("treadle") % "test", scalacOptions in Test ++= Seq("-language:reflectiveCalls"), scalacOptions in Compile in doc ++= Seq( -- cgit v1.2.3 From 548fe4265b556742785a07daf261672b72769e0c Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Wed, 12 Jan 2022 20:35:14 +0000 Subject: Update README and PR template for 3.5.0 release (#2339) (#2342) (cherry picked from commit d6710acbb969d0608693cb53142ea485062771d2) Co-authored-by: Jack Koenig --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- README.md | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 1077ec29..e430afbe 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -44,7 +44,7 @@ Text from here to the end of the body will be considered for inclusion in the re ### Reviewer Checklist (only modified by reviewer) - [ ] Did you add the appropriate labels? -- [ ] Did you mark the proper milestone (Bug fix: `3.3.x`, [small] API extension: `3.4.x`, API modification or big change: `3.5.0`)? +- [ ] Did you mark the proper milestone (Bug fix: `3.4.x`, [small] API extension: `3.5.x`, API modification or big change: `3.6.0`)? - [ ] Did you review? - [ ] Did you check whether all relevant Contributor checkboxes have been checked? - [ ] Did you mark as `Please Merge`? diff --git a/README.md b/README.md index 412b93e4..746cfd8a 100644 --- a/README.md +++ b/README.md @@ -121,10 +121,10 @@ For example, in SBT this could be expressed as: ```scala // build.sbt scalaVersion := "2.13.7" -addCompilerPlugin("edu.berkeley.cs" % "chisel3-plugin" % "3.5.0-RC2" cross CrossVersion.full) -libraryDependencies += "edu.berkeley.cs" %% "chisel3" % "3.5.0-RC2" +addCompilerPlugin("edu.berkeley.cs" % "chisel3-plugin" % "3.5.0" cross CrossVersion.full) +libraryDependencies += "edu.berkeley.cs" %% "chisel3" % "3.5.0" // We also recommend using chiseltest for writing unit tests -libraryDependencies += "edu.berkeley.cs" %% "chiseltest" % "0.5.0-RC2" % "test" +libraryDependencies += "edu.berkeley.cs" %% "chiseltest" % "0.5.0" % "test" ``` ### Guide For New Contributors If you are trying to make a contribution to this project, please read [CONTRIBUTING.md](https://github.com/Burnleydev1/chisel3/blob/recent_PR/CONTRIBUTING.md) @@ -144,7 +144,7 @@ These simulation-based verification tools are available for Chisel: - [**ScalaDoc**](https://www.chisel-lang.org/api/latest/chisel3/index.html), a listing, description, and examples of the functionality exposed by Chisel - [**Gitter**](https://gitter.im/freechipsproject/chisel3), where you can ask questions or discuss anything Chisel - [**Website**](https://www.chisel-lang.org) ([source](https://github.com/freechipsproject/www.chisel-lang.org/)) -- [**Scastie (3.5.0-RC2)**](https://scastie.scala-lang.org/R6wV80vUTr2CwhvouEdidg) +- [**Scastie (3.5.0)**](https://scastie.scala-lang.org/9ga9i2DvQymKlA5JjS1ieA) - [**asic-world**](http://www.asic-world.com/verilog/veritut.html) If you aren't familiar with verilog, this is a good tutorial. If you are migrating from Chisel2, see [the migration guide](https://www.chisel-lang.org/chisel3/chisel3-vs-chisel2.html). -- cgit v1.2.3 From bf0f0d8545747c4aa1e5e2abe4e27e339006f86f Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Mon, 17 Jan 2022 17:17:20 -0800 Subject: Add 3.5.x to Github Actions push triggers --- .github/workflows/test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dc7376a3..c0249269 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,6 +5,7 @@ on: push: branches: - master + - 3.5.x - 3.4.x - 3.3.x - 3.2.x -- cgit v1.2.3 From 042be4b2d8a846d12c5c5dfd64f10b5caee93125 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Wed, 19 Jan 2022 01:41:59 +0000 Subject: util: add GrayCode (#2353) (#2354) (cherry picked from commit 91d7baa8e7162d736f46b6d4964b09016d2dd172) Co-authored-by: Kevin Laeufer --- .../scala/chiselTests/util/GrayCodeTests.scala | 42 ++++++++++++++++++++++ src/main/scala/chisel3/util/GrayCode.scala | 24 +++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 integration-tests/src/test/scala/chiselTests/util/GrayCodeTests.scala create mode 100644 src/main/scala/chisel3/util/GrayCode.scala diff --git a/integration-tests/src/test/scala/chiselTests/util/GrayCodeTests.scala b/integration-tests/src/test/scala/chiselTests/util/GrayCodeTests.scala new file mode 100644 index 00000000..9562abb4 --- /dev/null +++ b/integration-tests/src/test/scala/chiselTests/util/GrayCodeTests.scala @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chiselTests.util + +import chisel3._ +import chisel3.util._ +import chiseltest._ +import chiseltest.formal._ +import org.scalatest.flatspec.AnyFlatSpec + +class GrayCodeTests extends AnyFlatSpec with ChiselScalatestTester with Formal { + behavior.of("GrayCode") + + val Widths = Seq(1, 2, 3, 5, 8, 17, 65) + Widths.foreach { w => + it should s"maintain identity (width=$w)" in { + verify(new GrayCodeIdentityCheck(w), Seq(BoundedCheck(1))) + } + + it should s"ensure hamming distance of one (width=$w)" in { + verify(new GrayCodeHammingCheck(w), Seq(BoundedCheck(1))) + } + } +} + +/** Checks that when we go from binary -> gray -> binary the result is always the same as the input. */ +private class GrayCodeIdentityCheck(width: Int) extends Module { + val in = IO(Input(UInt(width.W))) + val gray = BinaryToGray(in) + val out = GrayToBinary(gray) + assert(in === out, "%b -> %b -> %b", in, gray, out) +} + +/** Checks that if we increment the binary number, the gray code equivalent only changes by one bit. */ +private class GrayCodeHammingCheck(width: Int) extends Module { + val a = IO(Input(UInt(width.W))) + val b = a + 1.U + val aGray = BinaryToGray(a) + val bGray = BinaryToGray(b) + val hamming = PopCount(aGray ^ bGray) + assert(hamming === 1.U, "%b ^ %b = %b", aGray, bGray, hamming) +} diff --git a/src/main/scala/chisel3/util/GrayCode.scala b/src/main/scala/chisel3/util/GrayCode.scala new file mode 100644 index 00000000..ef310ee9 --- /dev/null +++ b/src/main/scala/chisel3/util/GrayCode.scala @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chisel3.util + +import chisel3._ + +object BinaryToGray { + + /** Turns a binary number into gray code. */ + def apply(in: UInt): UInt = in ^ (in >> 1) +} + +object GrayToBinary { + + /** Inverts the [[BinaryToGray]] operation. */ + def apply(in: UInt, width: Int): UInt = apply(in(width - 1, 0)) + + /** Inverts the [[BinaryToGray]] operation. */ + def apply(in: UInt): UInt = if (in.getWidth < 2) { in } + else { + val bits = in.getWidth - 2 to 0 by -1 + Cat(bits.scanLeft(in.head(1)) { case (prev, ii) => prev ^ in(ii) }) + } +} -- cgit v1.2.3 From a0798dceae8734bc273692edec585ed072b01b47 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Thu, 20 Jan 2022 07:42:53 +0000 Subject: Fix link to Naming Cookbook (#2356) (#2357) #naming implies within the current file. Removing the # refers to a file in the current directory. (cherry picked from commit 512ddd6c8b079dd83f2a3ed793e6595822a382cb) Co-authored-by: Jack Koenig --- docs/src/cookbooks/cookbook.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/cookbooks/cookbook.md b/docs/src/cookbooks/cookbook.md index d4cf3030..ec7e9ed2 100644 --- a/docs/src/cookbooks/cookbook.md +++ b/docs/src/cookbooks/cookbook.md @@ -440,7 +440,7 @@ chisel3.stage.ChiselStage.emitVerilog(new CountBits(4)) ### How do I get Chisel to name signals properly in blocks like when/withClockAndReset? -Use the compiler plugin, and check out the [Naming Cookbook](#naming) if that still does not do what you want. +Use the compiler plugin, and check out the [Naming Cookbook](naming) if that still does not do what you want. ### How do I get Chisel to name the results of vector reads properly? Currently, name information is lost when using dynamic indexing. For example: -- cgit v1.2.3 From a737281f670aa34152ce971b57f926ecc9307a8c Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Thu, 20 Jan 2022 19:44:41 +0000 Subject: Fix Compatibility Module io wrapping (#2355) (#2358) The new reflection based IO autowrapping for compatibility mode Modules would previously throw a NullPointerExceptions if any hardware were constructed in the Module before "val io" was initialized. The logic is now more robust for this case. Co-authored-by: Schuyler Eldridge (cherry picked from commit 50e6099fbecc041973564514e55f67ffe069459b) Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/BlackBox.scala | 21 +++++++----- core/src/main/scala/chisel3/RawModule.scala | 39 +++++++++++++--------- src/test/scala/chiselTests/CompatibilitySpec.scala | 23 +++++++++++-- 3 files changed, 56 insertions(+), 27 deletions(-) diff --git a/core/src/main/scala/chisel3/BlackBox.scala b/core/src/main/scala/chisel3/BlackBox.scala index 89c4ccd3..2f04fb2e 100644 --- a/core/src/main/scala/chisel3/BlackBox.scala +++ b/core/src/main/scala/chisel3/BlackBox.scala @@ -145,27 +145,30 @@ abstract class BlackBox( extends BaseBlackBox { // Find a Record port named "io" for purposes of stripping the prefix - private[chisel3] lazy val _io: Record = + private[chisel3] lazy val _io: Option[Record] = this .findPort("io") .collect { case r: Record => r } // Must be a Record - .getOrElse(null) // null handling occurs in generateComponent // Allow access to bindings from the compatibility package - protected def _compatIoPortBound() = portsContains(_io) + protected def _compatIoPortBound() = _io.exists(portsContains(_)) private[chisel3] override def generateComponent(): Option[Component] = { _compatAutoWrapPorts() // pre-IO(...) compatibility hack // Restrict IO to just io, clock, and reset - require(_io != null, "BlackBox must have a port named 'io' of type Record!") - require(portsContains(_io), "BlackBox must have io wrapped in IO(...)") + if (!_io.forall(portsContains)) { + throwException(s"BlackBox '$this' must have a port named 'io' of type Record wrapped in IO(...)!") + } + require(portsSize == 1, "BlackBox must only have one IO, called `io`") require(!_closed, "Can't generate module more than once") _closed = true - val namedPorts = _io.elements.toSeq.reverse // ListMaps are stored in reverse order + val io = _io.get + + val namedPorts = io.elements.toSeq.reverse // ListMaps are stored in reverse order // There is a risk of user improperly attempting to connect directly with io // Long term solution will be to define BlackBox IO differently as part of @@ -181,18 +184,18 @@ abstract class BlackBox( // of the io bundle, but NOT on the io bundle itself. // Doing so would cause the wrong names to be assigned, since their parent // is now the module itself instead of the io bundle. - for (id <- getIds; if id ne _io) { + for (id <- getIds; if id ne io) { id._onModuleClose } val firrtlPorts = namedPorts.map { namedPort => Port(namedPort._2, namedPort._2.specifiedDirection) } - val component = DefBlackBox(this, name, firrtlPorts, _io.specifiedDirection, params) + val component = DefBlackBox(this, name, firrtlPorts, io.specifiedDirection, params) _component = Some(component) _component } private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit = { - for ((_, port) <- _io.elements) { + for ((_, port) <- _io.map(_.elements).getOrElse(Nil)) { pushCommand(DefInvalid(UnlocatableSourceInfo, port.ref)) } } diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala index b81372d8..12b6a76a 100644 --- a/core/src/main/scala/chisel3/RawModule.scala +++ b/core/src/main/scala/chisel3/RawModule.scala @@ -178,11 +178,12 @@ package object internal { // Private reflective version of "val io" to maintain Chisel.Module semantics without having // io as a virtual method. See https://github.com/freechipsproject/chisel3/pull/1550 for more // information about the removal of "val io" - private def reflectivelyFindValIO(self: BaseModule): Record = { + private def reflectivelyFindValIO(self: BaseModule): Option[Record] = { // Java reflection is faster and works for the common case def tryJavaReflect: Option[Record] = Try { self.getClass.getMethod("io").invoke(self).asInstanceOf[Record] }.toOption + .filter(_ != null) // Anonymous subclasses don't work with Java reflection, so try slower, Scala reflection def tryScalaReflect: Option[Record] = { val ru = scala.reflect.runtime.universe @@ -199,6 +200,7 @@ package object internal { Try { im.reflectField(term).get.asInstanceOf[Record] }.toOption + .filter(_ != null) } } @@ -209,13 +211,6 @@ package object internal { // Fallback if reflection fails, user can wrap in IO(...) self.findPort("io").collect { case r: Record => r } } - .getOrElse( - throwException( - s"Compatibility mode Module '$this' must have a 'val io' Bundle. " + - "If there is such a field and you still see this error, autowrapping has failed (sorry!). " + - "Please wrap the Bundle declaration in IO(...)." - ) - ) } /** Legacy Module class that restricts IOs to just io, clock, and reset, and provides a constructor @@ -243,17 +238,29 @@ package object internal { def this(_clock: Clock, _reset: Bool)(implicit moduleCompileOptions: CompileOptions) = this(Option(_clock), Option(_reset))(moduleCompileOptions) - private lazy val _io: Record = reflectivelyFindValIO(this) + // Sort of a DIY lazy val because if the user tries to construct hardware before val io is + // constructed, _compatAutoWrapPorts will try to access it but it will be null + // In that case, we basically need to delay setting this var until later + private var _ioValue: Option[Record] = None + private def _io: Option[Record] = _ioValue.orElse { + _ioValue = reflectivelyFindValIO(this) + _ioValue + } // Allow access to bindings from the compatibility package - protected def _compatIoPortBound() = portsContains(_io) + protected def _compatIoPortBound() = _io.exists(portsContains(_)) private[chisel3] override def generateComponent(): Option[Component] = { _compatAutoWrapPorts() // pre-IO(...) compatibility hack // Restrict IO to just io, clock, and reset - require(_io != null, "Module must have io") - require(portsContains(_io), "Module must have io wrapped in IO(...)") + if (_io.isEmpty || !_compatIoPortBound) { + throwException( + s"Compatibility mode Module '$this' must have a 'val io' Bundle. " + + "If there is such a field and you still see this error, autowrapping has failed (sorry!). " + + "Please wrap the Bundle declaration in IO(...)." + ) + } require( (portsContains(clock)) && (portsContains(reset)), "Internal error, module did not have clock or reset as IO" @@ -264,8 +271,8 @@ package object internal { } override def _compatAutoWrapPorts(): Unit = { - if (!_compatIoPortBound() && _io != null) { - _bindIoInPlace(_io) + if (!_compatIoPortBound()) { + _io.foreach(_bindIoInPlace(_)) } } } @@ -283,13 +290,13 @@ package object internal { implicit moduleCompileOptions: CompileOptions) extends chisel3.BlackBox(params) { - override private[chisel3] lazy val _io: Record = reflectivelyFindValIO(this) + override private[chisel3] lazy val _io: Option[Record] = reflectivelyFindValIO(this) // This class auto-wraps the BlackBox with IO(...), allowing legacy code (where IO(...) wasn't // required) to build. override def _compatAutoWrapPorts(): Unit = { if (!_compatIoPortBound()) { - _bindIoInPlace(_io) + _io.foreach(_bindIoInPlace(_)) } } } diff --git a/src/test/scala/chiselTests/CompatibilitySpec.scala b/src/test/scala/chiselTests/CompatibilitySpec.scala index d134c380..41cfbec4 100644 --- a/src/test/scala/chiselTests/CompatibilitySpec.scala +++ b/src/test/scala/chiselTests/CompatibilitySpec.scala @@ -238,10 +238,10 @@ class CompatibiltySpec extends ChiselFlatSpec with ScalaCheckDrivenPropertyCheck "A Module without val io" should "throw an exception" in { class ModuleWithoutValIO extends Module { - val foo = new Bundle { + val foo = IO(new Bundle { val in = UInt(width = 32).asInput val out = Bool().asOutput - } + }) foo.out := foo.in(1) } val e = intercept[Exception]( @@ -595,4 +595,23 @@ class CompatibiltySpec extends ChiselFlatSpec with ScalaCheckDrivenPropertyCheck verilog should include("output [7:0] io_bar") } + it should "properly handle hardware construction before val io is initialized" in { + class MyModule extends Module { + val foo = Wire(init = UInt(8)) + val io = new Bundle { + val in = UInt(INPUT, width = 8) + val en = Bool(INPUT) + val out = UInt(OUTPUT, width = 8) + } + io.out := foo + when(io.en) { + io.out := io.in + } + } + // Just check that this doesn't crash during elaboration. For more info see: + // https://github.com/chipsalliance/chisel3/issues/1802 + // + ChiselStage.elaborate(new MyModule) + } + } -- cgit v1.2.3 From db0fe3fb23c3f50be0aeb859b19cb96967e15186 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Wed, 26 Jan 2022 19:52:36 +0000 Subject: Expand supported val modifiers for @public (#2365) (#2367) (cherry picked from commit 13fb4595b3b21beadaad5ba5c5d131138099d6e1) Co-authored-by: Adam Izraelevitz --- .../scala/chisel3/internal/InstantiableMacro.scala | 34 ++++++++++++---------- .../experimental/hierarchy/InstanceSpec.scala | 22 ++++++++++++++ 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/macros/src/main/scala/chisel3/internal/InstantiableMacro.scala b/macros/src/main/scala/chisel3/internal/InstantiableMacro.scala index d66b51ac..a001b284 100644 --- a/macros/src/main/scala/chisel3/internal/InstantiableMacro.scala +++ b/macros/src/main/scala/chisel3/internal/InstantiableMacro.scala @@ -16,21 +16,25 @@ private[chisel3] object instantiableMacro { // Note the triple `_` prefixing `module` is to avoid conflicts if a user marks a 'val module' // with @public; in this case, the lookup code is ambiguous between the generated `def module` // function and the argument to the generated implicit class. - val resultStats = stats.flatMap { - case x @ q"@public val $tpname: $tpe = $name" if tpname.toString() == name.toString() => - extensions += atPos(x.pos)(q"def $tpname = ___module._lookup(_.$tpname)") - Nil - case x @ q"@public val $tpname: $tpe = $_" => - extensions += atPos(x.pos)(q"def $tpname = ___module._lookup(_.$tpname)") - Seq(x) - case x @ q"@public val $tpname: $tpe" => - extensions += atPos(x.pos)(q"def $tpname = ___module._lookup(_.$tpname)") - Seq(x) - case x @ q"@public lazy val $tpname: $tpe = $_" => - extensions += atPos(x.pos)(q"def $tpname = ___module._lookup(_.$tpname)") - Seq(x) - case other => - Seq(other) + val resultStats = stats.flatMap { stat => + stat match { + case hasPublic: ValOrDefDef if hasPublic.mods.annotations.toString.contains("new public()") => + hasPublic match { + case aDef: DefDef => + c.error(aDef.pos, s"Cannot mark a def as @public") + Nil + // For now, we only omit protected/private vals + case aVal: ValDef + if aVal.mods.hasFlag(c.universe.Flag.PRIVATE) || aVal.mods.hasFlag(c.universe.Flag.PROTECTED) => + c.error(aVal.pos, s"Cannot mark a private or protected val as @public") + Nil + case aVal: ValDef => + extensions += atPos(aVal.pos)(q"def ${aVal.name} = ___module._lookup(_.${aVal.name})") + if (aVal.name.toString == aVal.children.last.toString) Nil else Seq(aVal) + case other => Seq(other) + } + case other => Seq(other) + } } (resultStats, extensions) } diff --git a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala index f62d1e49..45d1f85f 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala @@ -298,6 +298,28 @@ class InstanceSpec extends ChiselFunSpec with Utils { annos should contain(MarkAnnotation("~Top|Top/i:HasEither>x".rt, "xright")) annos should contain(MarkAnnotation("~Top|Top/i:HasEither>y".rt, "yleft")) } + it("3.12: should properly support val modifiers") { + class SupClass extends Module { + val value = 10 + val overriddenVal = 10 + } + trait SupTrait { + def x: Int + def y: Int + } + @instantiable class SubClass() extends SupClass with SupTrait { + // This errors + //@public private val privateVal = 10 + // This errors + //@public protected val protectedVal = 10 + @public override val overriddenVal = 12 + @public final val finalVal = 12 + @public lazy val lazyValue = 12 + @public val value = value + @public final override lazy val x: Int = 3 + @public override final lazy val y: Int = 4 + } + } } describe("4: toInstance") { it("4.0: should work on modules") { -- cgit v1.2.3 From 91ad2312cab72442fcfec9c28576b5464669f8fd Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Fri, 28 Jan 2022 00:20:24 +0000 Subject: Fix Decoder bug for constant 0 and DC (#2363) (#2371) * Fix the QMC bug for constant and dontcare output. * Fix the Espresso bug for constant and dontcare output. (cherry picked from commit 46b53320e9ec09fc26d63259f1821f4234640839) Co-authored-by: Jiuyang Liu --- .../experimental/minimizer/MinimizerSpec.scala | 35 ++++++++++++++++ .../experimental/decode/EspressoMinimizer.scala | 11 ++++- .../util/experimental/decode/QMCMinimizer.scala | 49 ++++++++++++++-------- 3 files changed, 76 insertions(+), 19 deletions(-) diff --git a/integration-tests/src/test/scala/chiselTests/util/experimental/minimizer/MinimizerSpec.scala b/integration-tests/src/test/scala/chiselTests/util/experimental/minimizer/MinimizerSpec.scala index 07afd074..2d3e073c 100644 --- a/integration-tests/src/test/scala/chiselTests/util/experimental/minimizer/MinimizerSpec.scala +++ b/integration-tests/src/test/scala/chiselTests/util/experimental/minimizer/MinimizerSpec.scala @@ -272,4 +272,39 @@ trait MinimizerSpec extends AnyFlatSpec with ChiselScalatestTester with Formal { BitPat(s"b${Seq(N, X, X, X, X, X, X, X, X, A2_X, A1_X, IMM_X, DW_X, FN_X, N, M_X, X, X, X, X, X, X, X, CSR_X, X, X, X, X).reduce(_ + _)}") )) } + + "output is 0" should "pass" in { + minimizerTest(TruthTable.fromString( + """00->0 + |01->? + |10->0 + |11->0 + | ? + |""".stripMargin + + )) + } + "output is 1" should "pass" in { + minimizerTest(TruthTable.fromString( + """00->1 + |01->? + |10->1 + |11->1 + | ? + |""".stripMargin + + )) + } + // I know this seems to be crazy, but if user is crazy as well... + "output is dont care" should "pass" in { + minimizerTest(TruthTable.fromString( + """00->? + |01->? + |10->? + |11->? + | ? + |""".stripMargin + + )) + } } diff --git a/src/main/scala/chisel3/util/experimental/decode/EspressoMinimizer.scala b/src/main/scala/chisel3/util/experimental/decode/EspressoMinimizer.scala index de2f207b..86973e5b 100644 --- a/src/main/scala/chisel3/util/experimental/decode/EspressoMinimizer.scala +++ b/src/main/scala/chisel3/util/experimental/decode/EspressoMinimizer.scala @@ -57,11 +57,20 @@ object EspressoMinimizer extends Minimizer with LazyLogging { def readTable(espressoTable: String) = { def bitPat(espresso: String): BitPat = BitPat("b" + espresso.replace('-', '?')) - espressoTable + val out = espressoTable .split("\n") .filterNot(_.startsWith(".")) .map(_.split(' ')) .map(row => bitPat(row(0)) -> bitPat(row(1))) + // special case for 0 and DontCare, if output is not couple to input + if (out.isEmpty) + Array( + ( + BitPat(s"b${"?" * table.inputWidth}"), + BitPat(s"b${"0" * table.outputWidth}") + ) + ) + else out } val input = writeTable(table) diff --git a/src/main/scala/chisel3/util/experimental/decode/QMCMinimizer.scala b/src/main/scala/chisel3/util/experimental/decode/QMCMinimizer.scala index a3481869..26a072f1 100644 --- a/src/main/scala/chisel3/util/experimental/decode/QMCMinimizer.scala +++ b/src/main/scala/chisel3/util/experimental/decode/QMCMinimizer.scala @@ -304,24 +304,37 @@ object QMCMinimizer extends Minimizer { ) }) - minimized.tail.foldLeft(table.copy(table = Seq(minimized.head))) { - case (tb, t) => - if (tb.table.exists(x => x._1 == t._1)) { - tb.copy(table = tb.table.map { - case (k, v) => - if (k == t._1) { - def ones(bitPat: BitPat) = bitPat.rawString.zipWithIndex.collect { case ('1', x) => x } - ( - k, - BitPat( - "b" + (0 until v.getWidth).map(i => if ((ones(v) ++ ones(t._2)).contains(i)) "1" else "?").mkString + // special case for 0 and DontCare, if output is not couple to input + if (minimized.isEmpty) + table.copy( + Seq( + ( + BitPat(s"b${"?" * table.inputWidth}"), + BitPat(s"b${"0" * table.outputWidth}") + ) + ) + ) + else + minimized.tail.foldLeft(table.copy(table = Seq(minimized.head))) { + case (tb, t) => + if (tb.table.exists(x => x._1 == t._1)) { + tb.copy(table = tb.table.map { + case (k, v) => + if (k == t._1) { + def ones(bitPat: BitPat) = bitPat.rawString.zipWithIndex.collect { case ('1', x) => x } + ( + k, + BitPat( + "b" + (0 until v.getWidth) + .map(i => if ((ones(v) ++ ones(t._2)).contains(i)) "1" else "?") + .mkString + ) ) - ) - } else (k, v) - }) - } else { - tb.copy(table = tb.table :+ t) - } - } + } else (k, v) + }) + } else { + tb.copy(table = tb.table :+ t) + } + } } } -- cgit v1.2.3 From ea1ced34b5c9e42412cc0ac3e7431cd3194ccbc3 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Tue, 1 Feb 2022 19:56:13 +0000 Subject: Chisel plugin bundle elements handler (#2306) (#2380) Adds generation of `Bundle.elements` method to the chores done by the compiler plugin For each `Bundle` find the relevant visible Chisel field members and construct a hard-coded list of the elements and their names implemented as `_elementsImpl` For more details: See plugins/README.md - Should be no change in API - Handles inheritance and mixins - Handles Seq[Data] - Tests in BundleElementSpec Co-authored-by: chick Co-authored-by: Jack Koenig (cherry picked from commit 237200a420581519f29149cbae9b3e968c0d01fc) Co-authored-by: Chick Markley --- build.sbt | 1 + build.sc | 4 + core/src/main/scala/chisel3/Aggregate.scala | 50 +- core/src/main/scala/chisel3/Data.scala | 2 +- plugin/README.md | 99 ++++ .../chisel3/internal/plugin/BundleComponent.scala | 190 +++++-- .../chisel3/internal/plugin/ChiselPlugin.scala | 12 +- .../scala/chiselTests/BundleElementsSpec.scala | 564 +++++++++++++++++++++ src/test/scala/chiselTests/BundleSpec.scala | 10 +- 9 files changed, 887 insertions(+), 45 deletions(-) create mode 100644 plugin/README.md create mode 100644 src/test/scala/chiselTests/BundleElementsSpec.scala diff --git a/build.sbt b/build.sbt index b9406c8f..f5f178fd 100644 --- a/build.sbt +++ b/build.sbt @@ -198,6 +198,7 @@ lazy val chisel = (project in file(".")). settings( mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3" % "3.5.0"), libraryDependencies += defaultVersions("treadle") % "test", + Test / scalacOptions += "-P:chiselplugin:genBundleElements", scalacOptions in Test ++= Seq("-language:reflectiveCalls"), scalacOptions in Compile in doc ++= Seq( "-diagrams", diff --git a/build.sc b/build.sc index 1e665a9f..d7dbc6d7 100644 --- a/build.sc +++ b/build.sc @@ -107,6 +107,10 @@ class chisel3CrossModule(val crossScalaVersion: String) extends CommonModule wit object test extends Tests { override def scalacPluginClasspath = m.scalacPluginClasspath + override def scalacOptions = T { + super.scalacOptions() ++ Agg("-P:chiselplugin:genBundleElements") + } + override def ivyDeps = m.ivyDeps() ++ Agg( ivy"org.scalatest::scalatest:3.2.10", ivy"org.scalatestplus::scalacheck-1-14:3.2.2.0", diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 15cdf428..75966a16 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1208,12 +1208,58 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { * }}} */ final lazy val elements: SeqMap[String, Data] = { + val hardwareFields = _elementsImpl.flatMap { + case (name, data: Data) => + if (data.isSynthesizable) { + Some(s"$name: $data") + } else { + None + } + case (name, Some(data: Data)) => + if (data.isSynthesizable) { + Some(s"$name: $data") + } else { + None + } + case (name, s: scala.collection.Seq[Any]) if s.nonEmpty => + s.head match { + // Ignore empty Seq() + case d: Data => + throwException( + "Public Seq members cannot be used to define Bundle elements " + + s"(found public Seq member '${name}'). " + + "Either use a Vec if all elements are of the same type, or MixedVec if the elements " + + "are of different types. If this Seq member is not intended to construct RTL, mix in the trait " + + "IgnoreSeqInBundle." + ) + case _ => // don't care about non-Data Seq + } + None + + case _ => None + } + if (hardwareFields.nonEmpty) { + throw ExpectedChiselTypeException(s"Bundle: $this contains hardware fields: " + hardwareFields.mkString(",")) + } + VectorMap(_elementsImpl.toSeq.flatMap { + case (name, data: Data) => + Some(name -> data) + case (name, Some(data: Data)) => + Some(name -> data) + case _ => None + }.sortWith { + case ((an, a), (bn, b)) => (a._id > b._id) || ((a eq b) && (an > bn)) + }: _*) + } + /* + * This method will be overwritten by the Chisel-Plugin + */ + protected def _elementsImpl: SeqMap[String, Any] = { val nameMap = LinkedHashMap[String, Data]() for (m <- getPublicFields(classOf[Bundle])) { getBundleField(m) match { case Some(d: Data) => - requireIsChiselType(d) - + // Checking for chiselType is done in elements method if (nameMap contains m.getName) { require(nameMap(m.getName) eq d) } else { diff --git a/core/src/main/scala/chisel3/Data.scala b/core/src/main/scala/chisel3/Data.scala index 9e9f5dbb..ef43a3b0 100644 --- a/core/src/main/scala/chisel3/Data.scala +++ b/core/src/main/scala/chisel3/Data.scala @@ -480,7 +480,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { protected[chisel3] def binding: Option[Binding] = _binding protected def binding_=(target: Binding) { if (_binding.isDefined) { - throw RebindingException(s"Attempted reassignment of binding to $this") + throw RebindingException(s"Attempted reassignment of binding to $this, from: ${target}") } _binding = Some(target) } diff --git a/plugin/README.md b/plugin/README.md new file mode 100644 index 00000000..762f4822 --- /dev/null +++ b/plugin/README.md @@ -0,0 +1,99 @@ +# Notes on the Compiler Plug-in + +The Chisel plugin provides some operations that are too difficult, or not possbile, +to implement through regular Scala code. + +# This documentation is for developers working on chisel internals. + +## Compiler plugin operations +These are the two things that the compile plugin does. + +1. Automatically generates the `cloneType` methods of Bundle +2. Changes the underlying mechanics of the `Bundle`s `elements` method in a way +that does not require the use of **reflection** +3. Future work: Make having a Seq[Data] in a bundle be a compiler error. See "Detecting Bundles with Seq[Data]" below. + +### 1. Generating `cloneType` method +As of Mar 18, 2021, PR #1826, generating the `cloneType` method (1. above) is now the default behavior. +The cloneType method used to be a tricky thing to write for chisel developers. +For historical purposes, here is the flag was used to control that prior to full adoption. +``` +-P:chiselplugin:useBundlePlugin +``` + +### 2. Changing `Bundle#elements` method + +A `Bundle` has a default `elements` method that relies on **reflection**, which is slow and brittle, to access the list of +*fields* the bundle contains. +When enabled this second operation of the plugin examines +the `Bundle`s AST in order to determine the fields and then re-writes the underlying code of `elements`. +Technically, rewriting a lower level private method `_elementsImpl`. +It is expected that the using this feature will shortly become the default. + +>The plugin should not be enabled for the `main` chisel3 project because of internal considerations. +> It is enabled for the `Test` section. + +In the meantime, advanced users can try using the feature by adding the following flag to the scalac options in their +chisel projects. + +``` +-P:chiselplugin:buildElementAccessor +``` + +For example in an `build.sbt` file adding the line +``` +scalacOptions += "-P:chiselplugin:genBundleElements", +``` +in the appropriate place. + +## Future work +### Detecting Bundles with Seq[Data] +Trying to have a `val Seq[Data]` (as opposed to a `val Vec[Data]` in a `Bundle` is a run time error. +Here is a block of code that could be added to the plugin to detect this case at compile time (with some refinement in +the detection mechanism): +```scala + if (member.isAccessor && typeIsSeqOfData(member.tpe) && !isIgnoreSeqInBundle(bundleSymbol)) { + global.reporter.error( + member.pos, + s"Bundle.field ${bundleSymbol.name}.${member.name} cannot be a Seq[Data]. " + + "Use Vec or MixedVec or mix in trait IgnoreSeqInBundle" + ) + } +``` +### Notes about working on the `_elementsImpl` generator for the plugin in `BundleComponent.scala` +In general the easiest way to develop and debug new code in the plugin is to use `println` statements. +Naively this can result in reams of text that can be very hard to look through. + +What I found to be useful was creating some wrappers for `println` that only printed when the `Bundles` had a particular name pattern. +- Create a regular expression string in the `BundleComponent` class +- Add a printf wrapper name `show` that checks the `Bundle`'s name against the regex +- For recursive code in `getAllBundleFields` create a different wrapper `indentShow` that indents debug lines +- Sprinkle calls to these wrappers as needed for debugging + +#### Bundle Regex +```scala + val bundleNameDebugRegex = "MyBundle.*" +``` +#### Add `show` wrapper +`show` should be inside `case bundle` block of the `transform` method in order to have access to the current `Bundle` + +```scala +def show(string: => String): Unit = { + if (bundle.symbol.name.toString.matches(bundleNameDebugRegex)) { + println(string) + } +} +``` +#### Add `indentShow` wrapper +This method can be added into `BundleComponent.scala` in the `transform` method after `case Bundle` +Inside of `getAllBundleFields` I added the following code that indented for each recursion up the current +`Bundle`'s hierarchy. +```scala +def indentShow(s: => String): Unit = { + val indentString = ("-" * depth) * 2 + "> " + s.split("\n").foreach { line => + show(indentString + line) + } +} +``` + diff --git a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala index 2d3a2cae..e92bbb23 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala @@ -9,7 +9,16 @@ import scala.tools.nsc.plugins.PluginComponent import scala.tools.nsc.symtab.Flags import scala.tools.nsc.transform.TypingTransformers -// TODO This component could also implement val elements in Bundles +/** Performs three operations + * 1) Records that this plugin ran on a bundle by adding a method + * `override protected def _usingPlugin: Boolean = true` + * 2) Constructs a cloneType method + * 3) Builds a `def elements` that is computed once in this plugin + * Eliminates needing reflection to discover the hardware fields of a `Bundle` + * + * @param global the environment + * @param arguments run time parameters to code + */ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPluginArguments) extends PluginComponent with TypingTransformers { @@ -32,14 +41,41 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi def inferType(t: Tree): Type = localTyper.typed(t, nsc.Mode.TYPEmode).tpe - val bundleTpe = inferType(tq"chisel3.Bundle") - val dataTpe = inferType(tq"chisel3.Data") + val bundleTpe: Type = inferType(tq"chisel3.Bundle") + val dataTpe: Type = inferType(tq"chisel3.Data") + val ignoreSeqTpe: Type = inferType(tq"chisel3.IgnoreSeqInBundle") + val seqOfDataTpe: Type = inferType(tq"scala.collection.Seq[chisel3.Data]") + val someOfDataTpe: Type = inferType(tq"scala.Option[chisel3.Data]") + val seqMapTpe: Type = inferType(tq"scala.collection.immutable.SeqMap[String,Any]") // Not cached because it should only be run once per class (thus once per Type) - def isBundle(sym: Symbol): Boolean = sym.tpe <:< bundleTpe + def isBundle(sym: Symbol): Boolean = { sym.tpe <:< bundleTpe } + + def isIgnoreSeqInBundle(sym: Symbol): Boolean = { sym.tpe <:< ignoreSeqTpe } + + def isSeqOfData(sym: Symbol): Boolean = { + val tpe = sym.tpe + tpe match { + case NullaryMethodType(resultType) => + resultType <:< seqOfDataTpe + case _ => + false + } + } + + def isOptionOfData(symbol: Symbol): Boolean = { + val tpe = symbol.tpe + tpe match { + case NullaryMethodType(resultType) => + resultType <:< someOfDataTpe + case _ => + false + } + } + def isExactBundle(sym: Symbol): Boolean = { sym.tpe =:= bundleTpe } - val isDataCache = new mutable.HashMap[Type, Boolean] // Cached because this is run on every argument to every Bundle + val isDataCache = new mutable.HashMap[Type, Boolean] def isData(sym: Symbol): Boolean = isDataCache.getOrElseUpdate(sym.tpe, sym.tpe <:< dataTpe) def cloneTypeFull(tree: Tree): Tree = @@ -63,7 +99,7 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi val msg = "Users cannot override _usingPlugin, it is for the compiler plugin's use only." global.globalError(d.pos, msg) case d: DefDef if isNullaryMethodNamed("cloneType", d) => - val msg = "Users cannot override cloneType. Let the compiler plugin generate it." + val msg = "Users cannot override cloneType. Let the compiler plugin generate it." global.globalError(d.pos, msg) case _ => } @@ -72,52 +108,136 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi override def transform(tree: Tree): Tree = tree match { - case bundle: ClassDef if isBundle(bundle.symbol) && !bundle.mods.hasFlag(Flag.ABSTRACT) => + case bundle: ClassDef if isBundle(bundle.symbol) => // ==================== Generate _cloneTypeImpl ==================== val (con, params) = getConstructorAndParams(bundle.impl.body) if (con.isEmpty) { global.reporter.warning(bundle.pos, "Unable to determine primary constructor!") return super.transform(tree) } - val constructor = con.get + val constructor = con.get val thiz = gen.mkAttributedThis(bundle.symbol) // The params have spaces after them (Scalac implementation detail) val paramLookup: String => Symbol = params.map(sym => sym.name.toString.trim -> sym).toMap - // Create a this. for each field matching order of constructor arguments - // List of Lists because we can have multiple parameter lists - val conArgs: List[List[Tree]] = - constructor.vparamss.map(_.map { vp => - val p = paramLookup(vp.name.toString) - // Make this. - val select = gen.mkAttributedSelect(thiz, p) - // Clone any Data parameters to avoid field aliasing, need full clone to include direction - if (isData(vp.symbol)) cloneTypeFull(select) else select - }) - - val tparamList = bundle.tparams.map { t => Ident(t.symbol) } - val ttpe = if (tparamList.nonEmpty) AppliedTypeTree(Ident(bundle.symbol), tparamList) else Ident(bundle.symbol) - val newUntyped = New(ttpe, conArgs) - val neww = localTyper.typed(newUntyped) - - // Create the symbol for the method and have it be associated with the Bundle class - val cloneTypeSym = - bundle.symbol.newMethod(TermName("_cloneTypeImpl"), bundle.symbol.pos.focus, Flag.OVERRIDE | Flag.PROTECTED) - // Handwritten cloneTypes don't have the Method flag set, unclear if it matters - cloneTypeSym.resetFlag(Flags.METHOD) - // Need to set the type to chisel3.Bundle for the override to work - cloneTypeSym.setInfo(NullaryMethodType(bundleTpe)) - - val cloneTypeImpl = localTyper.typed(DefDef(cloneTypeSym, neww)) + val cloneTypeImplOpt = if (!bundle.mods.hasFlag(Flag.ABSTRACT)) { + // Create a this. for each field matching order of constructor arguments + // List of Lists because we can have multiple parameter lists + val conArgs: List[List[Tree]] = + constructor.vparamss.map(_.map { vp => + val p = paramLookup(vp.name.toString) + // Make this. + val select = gen.mkAttributedSelect(thiz.asInstanceOf[Tree], p) + // Clone any Data parameters to avoid field aliasing, need full clone to include direction + if (isData(vp.symbol)) cloneTypeFull(select.asInstanceOf[Tree]) else select + }) + + val tparamList = bundle.tparams.map { t => Ident(t.symbol) } + val ttpe = + if (tparamList.nonEmpty) AppliedTypeTree(Ident(bundle.symbol), tparamList) else Ident(bundle.symbol) + val newUntyped = New(ttpe, conArgs) + val neww = localTyper.typed(newUntyped) + + // Create the symbol for the method and have it be associated with the Bundle class + val cloneTypeSym = + bundle.symbol.newMethod(TermName("_cloneTypeImpl"), bundle.symbol.pos.focus, Flag.OVERRIDE | Flag.PROTECTED) + // Handwritten cloneTypes don't have the Method flag set, unclear if it matters + cloneTypeSym.resetFlag(Flags.METHOD) + // Need to set the type to chisel3.Bundle for the override to work + cloneTypeSym.setInfo(NullaryMethodType(bundleTpe)) + + Some(localTyper.typed(DefDef(cloneTypeSym, neww))) + } else { + // Don't create if this Bundle is abstract + None + } + + // ==================== Generate val elements ==================== + + /* Test to see if the bundle found is amenable to having it's elements + * converted to an immediate form that will not require reflection + */ + def isSupportedBundleType: Boolean = { + arguments.genBundleElements && !bundle.mods.hasFlag(Flag.ABSTRACT) + } + + val elementsImplOpt = if (isSupportedBundleType) { + /* extract the true fields from the super classes a given bundle + * depth argument can be helpful for debugging + */ + def getAllBundleFields(bundleSymbol: Symbol, depth: Int = 0): List[(String, Tree)] = { + + def isBundleField(member: Symbol): Boolean = { + if (!member.isAccessor) { + false + } else if (isData(member.tpe.typeSymbol)) { + true + } else if (isOptionOfData(member)) { + true + } else if (isSeqOfData(member)) { + // This field is passed along, even though it is illegal + // An error for this will be generated in `Bundle.elements` + // It would be possible here to check for Seq[Data] and make a compiler error, but + // that would be a API error difference. See reference in docs/chisel-plugin.md + // If Bundle is subclass of IgnoreSeqInBundle then don't pass this field along + + !isIgnoreSeqInBundle(bundleSymbol) + } else { + // none of the above + false + } + } + + val currentFields = bundleSymbol.info.members.flatMap { + + case member if member.isPublic => + if (isBundleField(member)) { + // The params have spaces after them (Scalac implementation detail) + Some(member.name.toString.trim -> gen.mkAttributedSelect(thiz.asInstanceOf[Tree], member)) + } else { + None + } + + case _ => None + }.toList + + val allParentFields = bundleSymbol.parentSymbols.flatMap { parentSymbol => + val fieldsFromParent = if (depth < 1 && !isExactBundle(bundleSymbol)) { + val foundFields = getAllBundleFields(parentSymbol, depth + 1) + foundFields + } else { + List() + } + fieldsFromParent + } + allParentFields ++ currentFields + } + + val elementArgs = getAllBundleFields(bundle.symbol) + + val elementsImplSym = + bundle.symbol.newMethod(TermName("_elementsImpl"), bundle.symbol.pos.focus, Flag.OVERRIDE | Flag.PROTECTED) + elementsImplSym.resetFlag(Flags.METHOD) + elementsImplSym.setInfo(NullaryMethodType(seqMapTpe)) + + val elementsImpl = localTyper.typed( + DefDef(elementsImplSym, q"scala.collection.immutable.SeqMap.apply[String, Any](..$elementArgs)") + ) + + Some(elementsImpl) + } else { + // No code generated for elements accessor + None + } // ==================== Generate _usingPlugin ==================== // Unclear why quasiquotes work here but didn't for cloneTypeSym, maybe they could. - val usingPlugin = localTyper.typed(q"override protected def _usingPlugin: Boolean = true") + val usingPluginOpt = Some(localTyper.typed(q"override protected def _usingPlugin: Boolean = true")) val withMethods = deriveClassDef(bundle) { t => - deriveTemplate(t)(_ :+ cloneTypeImpl :+ usingPlugin) + deriveTemplate(t)(_ ++ cloneTypeImplOpt ++ usingPluginOpt ++ elementsImplOpt) } super.transform(localTyper.typed(withMethods)) diff --git a/plugin/src/main/scala/chisel3/internal/plugin/ChiselPlugin.scala b/plugin/src/main/scala/chisel3/internal/plugin/ChiselPlugin.scala index bd02d50c..9bf8c657 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/ChiselPlugin.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/ChiselPlugin.scala @@ -8,10 +8,13 @@ import nsc.plugins.{Plugin, PluginComponent} import scala.reflect.internal.util.NoPosition import scala.collection.mutable -private[plugin] case class ChiselPluginArguments(val skipFiles: mutable.HashSet[String] = mutable.HashSet.empty) { +private[plugin] case class ChiselPluginArguments( + val skipFiles: mutable.HashSet[String] = mutable.HashSet.empty, + var genBundleElements: Boolean = false) { def useBundlePluginOpt = "useBundlePlugin" def useBundlePluginFullOpt = s"-P:${ChiselPlugin.name}:$useBundlePluginOpt" - + def genBundleElementsOpt = "genBundleElements" + def genBundleElementsFullOpt = s"-P:${ChiselPlugin.name}:$genBundleElementsOpt" // Annoying because this shouldn't be used by users def skipFilePluginOpt = "INTERNALskipFile:" def skipFilePluginFullOpt = s"-P:${ChiselPlugin.name}:$skipFilePluginOpt" @@ -20,7 +23,7 @@ private[plugin] case class ChiselPluginArguments(val skipFiles: mutable.HashSet[ object ChiselPlugin { val name = "chiselplugin" - // Also logs why the compoennt was not run + // Also logs why the component was not run private[plugin] def runComponent( global: Global, arguments: ChiselPluginArguments @@ -67,11 +70,12 @@ class ChiselPlugin(val global: Global) extends Plugin { // Be annoying and warn because users are not supposed to use this val msg = s"Option -P:${ChiselPlugin.name}:$option should only be used for internal chisel3 compiler purposes!" global.reporter.warning(NoPosition, msg) + } else if (option == arguments.genBundleElementsOpt) { + arguments.genBundleElements = true } else { error(s"Option not understood: '$option'") } } true } - } diff --git a/src/test/scala/chiselTests/BundleElementsSpec.scala b/src/test/scala/chiselTests/BundleElementsSpec.scala new file mode 100644 index 00000000..fab2e733 --- /dev/null +++ b/src/test/scala/chiselTests/BundleElementsSpec.scala @@ -0,0 +1,564 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chiselTests + +import chisel3._ +import chisel3.experimental.{ChiselEnum, FixedPoint} +import chisel3.stage.ChiselStage +import chisel3.util.Decoupled +import org.scalatest.exceptions.TestFailedException +import org.scalatest.freespec.AnyFreeSpec +import org.scalatest.matchers.should.Matchers + +import scala.language.reflectiveCalls + +/* Rich and complicated bundle examples + */ +trait BpipSuperTraitWithField { + val bpipSuperTraitGood = SInt(17.W) + def bpipSuperTraitBad = SInt(22.W) +} + +trait BpipTraitWithField extends BpipSuperTraitWithField { + val bpipTraitGood = SInt(17.W) + def bpipTraitBad = SInt(22.W) +} + +class BpipOneField extends Bundle with BpipTraitWithField { + val bpipOneFieldOne = SInt(8.W) + val bpipOneFieldTwo = SInt(8.W) +} + +class BpipTwoField extends BpipOneField { + val bpipTwoFieldOne = SInt(8.W) + val bpipTwoFieldTwo = Vec(4, UInt(12.W)) + val myInt = 7 + val baz = Decoupled(UInt(16.W)) +} + +class BpipDecoupled extends BpipOneField { + val bpipDecoupledSInt = SInt(8.W) + val bpipDecoupledVec = Vec(4, UInt(12.W)) + val bpipDecoupledDecoupled = Decoupled(UInt(16.W)) +} + +class HasDecoupledBundleByInheritance extends Module { + val out1 = IO(Output(new BpipDecoupled)) + assertElementsMatchExpected(out1)( + "bpipDecoupledDecoupled" -> _.bpipDecoupledDecoupled, + "bpipDecoupledVec" -> _.bpipDecoupledVec, + "bpipDecoupledSInt" -> _.bpipDecoupledSInt, + "bpipOneFieldTwo" -> _.bpipOneFieldTwo, + "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipTraitGood" -> _.bpipTraitGood, + "bpipSuperTraitGood" -> _.bpipSuperTraitGood + ) +} + +/* plugin should not affect the seq detection */ +class DebugProblem3 extends Module { + val out1 = IO(Output(new BpipTwoField)) + assertElementsMatchExpected(out1)( + "baz" -> _.baz, + "bpipTwoFieldTwo" -> _.bpipTwoFieldTwo, + "bpipTwoFieldOne" -> _.bpipTwoFieldOne, + "bpipOneFieldTwo" -> _.bpipOneFieldTwo, + "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipTraitGood" -> _.bpipTraitGood, + "bpipSuperTraitGood" -> _.bpipSuperTraitGood + ) +} + +class BpipBadSeqBundle extends Bundle { + val bpipBadSeqBundleGood = UInt(999.W) + val bpipBadSeqBundleBad = Seq(UInt(16.W), UInt(8.W), UInt(4.W)) +} + +class HasBadSeqBundle extends Module { + val out1 = IO(Output(new BpipBadSeqBundle)) +} + +class BpipBadSeqBundleWithIgnore extends Bundle with IgnoreSeqInBundle { + val goodFieldWithIgnore = UInt(999.W) + val badSeqFieldWithIgnore = Seq(UInt(16.W), UInt(8.W), UInt(4.W)) +} + +class UsesIgnoreSeqInBundle extends Module { + val out1 = IO(Output(new BpipBadSeqBundleWithIgnore)) +} + +/* This is mostly a test of the field order */ +class BpipP8_1 extends Bundle { + val field_1_1 = UInt(11.W) + val field_1_2 = UInt(12.W) +} + +class BpipP8_2 extends BpipP8_1 { + val field_2_1 = UInt(11.W) + val field_2_2 = UInt(12.W) +} + +class BpipP8_3 extends BpipP8_2 { + val field_3_1 = UInt(11.W) + val field_3_2 = UInt(12.W) +} + +/* plugin should not affect the seq detection */ +class ForFieldOrderingTest extends Module { + val out1 = IO(Output(new BpipP8_3)) + out1 := DontCare + assertElementsMatchExpected(out1)( + "field_3_2" -> _.field_3_2, + "field_3_1" -> _.field_3_1, + "field_2_2" -> _.field_2_2, + "field_2_1" -> _.field_2_1, + "field_1_2" -> _.field_1_2, + "field_1_1" -> _.field_1_1 + ) +} + +/* plugin should allow parameter var fields */ +class HasValParamsToBundle extends Module { + // The following block does not work, suggesting that ParamIsField is not a case we need to solve + class BpipParamIsField0(val paramField0: UInt) extends Bundle + class BpipParamIsField1(val paramField1: UInt) extends BpipParamIsField0(UInt(66.W)) + + val out3 = IO(Output(new BpipParamIsField1(UInt(10.W)))) + val out4 = IO(Output(new BpipParamIsField1(UInt(10.W)))) + out3 := DontCare + assertElementsMatchExpected(out3)("paramField0" -> _.paramField0, "paramField1" -> _.paramField1) + assertElementsMatchExpected(out4)("paramField0" -> _.paramField0, "paramField1" -> _.paramField1) +} + +class HasGenParamsPassedToSuperclasses extends Module { + + class OtherBundle extends Bundle { + val otherField = UInt(55.W) + } + + class BpipWithGen[T <: Data, TT <: Data](gen: T, gen2: => TT) extends Bundle { + val superFoo = gen + val superQux = gen2 + } + +// class BpipDemoBundle[T <: Data](gen: T, gen2: => T) extends BpipTwoField with BpipVarmint { + class BpipUsesWithGen[T <: Data](gen: T, gen2: => T) extends BpipWithGen(gen, gen2) { + // val foo = gen + val bar = Bool() + val qux = gen2 + val bad = 444 + val baz = Decoupled(UInt(16.W)) + } + + val out1 = IO(Output(new BpipUsesWithGen(UInt(4.W), new OtherBundle))) + val out2 = IO(Output(new BpipUsesWithGen(UInt(4.W), FixedPoint(10.W, 4.BP)))) + + out1 := DontCare + + assertElementsMatchExpected(out1)( + "baz" -> _.baz, + "qux" -> _.qux, + "bar" -> _.bar, + "superQux" -> _.superQux, + "superFoo" -> _.superFoo + ) + assertElementsMatchExpected(out2)( + "baz" -> _.baz, + "qux" -> _.qux, + "bar" -> _.bar, + "superQux" -> _.superQux, + "superFoo" -> _.superFoo + ) +} + +/* Testing whether gen fields superFoo and superQux can be found when they are + * declared in a superclass + * + */ +class UsesGenFiedldsInSuperClass extends Module { + class BpipWithGen[T <: Data](gen: T) extends Bundle { + val superFoo = gen + val superQux = gen + } + + class BpipUsesWithGen[T <: Data](gen: T) extends BpipWithGen(gen) {} + + val out = IO(Output(new BpipUsesWithGen(UInt(4.W)))) + + out := DontCare + + assertElementsMatchExpected(out)() +} + +/* Testing whether gen fields superFoo and superQux can be found when they are + * declared in a superclass + * + */ +class BpipBadBundleWithHardware extends Bundle { + val bpipWithHardwareGood = UInt(8.W) + val bpipWithHardwareBad = 244.U(16.W) +} + +class HasHardwareFieldsInBundle extends Module { + val out = IO(Output(new BpipBadBundleWithHardware)) + assertElementsMatchExpected(out)() +} + +/* This is legal because of => + */ +class UsesBundleWithGeneratorField extends Module { + class BpipWithGen[T <: Data](gen: => T) extends Bundle { + val superFoo = gen + val superQux = gen + } + + class BpipUsesWithGen[T <: Data](gen: => T) extends BpipWithGen(gen) + + val out = IO(Output(new BpipUsesWithGen(UInt(4.W)))) + + out := DontCare + + assertElementsMatchExpected(out)("superQux" -> _.superQux, "superFoo" -> _.superFoo) +} + +/* Testing whether gen fields superFoo and superQux can be found when they are + * declared in a superclass + * + */ +class BundleElementsSpec extends AnyFreeSpec with Matchers { + + /** Tests a whole bunch of different Bundle constructions + */ + class BpipIsComplexBundle extends Module { + + trait BpipVarmint { + val varmint = Bool() + + def vermin = Bool() + + private val puppy = Bool() + } + + abstract class BpipAbstractBundle extends Bundle { + def doNothing: Unit + + val fromAbstractBundle = UInt(22.W) + } + + class BpipOneField extends Bundle { + val fieldOne = SInt(8.W) + } + + class BpipTwoField extends BpipOneField { + val fieldTwo = SInt(8.W) + val fieldThree = Vec(4, UInt(12.W)) + } + + class BpipAnimalBundle(w1: Int, w2: Int) extends Bundle { + val dog = SInt(w1.W) + val fox = UInt(w2.W) + } + + class BpipDemoBundle[T <: Data](gen: T, gen2: => T) extends BpipTwoField with BpipVarmint { + val foo = gen + val bar = Bool() + val qux = gen2 + val bad = 44 + val baz = Decoupled(UInt(16.W)) + val animals = new BpipAnimalBundle(4, 8) + } + + val out = IO(Output(new BpipDemoBundle(UInt(4.W), FixedPoint(10.W, 4.BP)))) + + val out2 = IO(Output(new BpipAbstractBundle { + override def doNothing: Unit = () + + val notAbstract: Bool = Bool() + })) + + val out4 = IO(Output(new BpipAnimalBundle(99, 100))) + val out5 = IO(Output(new BpipTwoField)) + + out := DontCare + out5 := DontCare + + assertElementsMatchExpected(out)( + "animals" -> _.animals, + "baz" -> _.baz, + "qux" -> _.qux, + "bar" -> _.bar, + "varmint" -> _.varmint, + "fieldThree" -> _.fieldThree, + "fieldTwo" -> _.fieldTwo, + "fieldOne" -> _.fieldOne, + "foo" -> _.foo + ) + assertElementsMatchExpected(out5)("fieldThree" -> _.fieldThree, "fieldTwo" -> _.fieldTwo, "fieldOne" -> _.fieldOne) + assertElementsMatchExpected(out2)("notAbstract" -> _.notAbstract, "fromAbstractBundle" -> _.fromAbstractBundle) + assertElementsMatchExpected(out4)("fox" -> _.fox, "dog" -> _.dog) + } + + "Complex Bundle with inheritance, traits and params. DebugProblem1" in { + ChiselStage.emitFirrtl(new BpipIsComplexBundle) + } + + "Decoupled Bundle with inheritance" in { + ChiselStage.emitFirrtl(new HasDecoupledBundleByInheritance) + } + + "Simple bundle inheritance. DebugProblem3" in { + ChiselStage.emitFirrtl(new DebugProblem3) + } + + "Bundles containing Seq[Data] should be compile erorr. HasBadSeqBundle" in { + intercept[ChiselException] { + ChiselStage.emitFirrtl(new HasBadSeqBundle) + } + } + + "IgnoreSeqInBundle allows Seq[Data] in bundle" in { + ChiselStage.emitFirrtl(new UsesIgnoreSeqInBundle) + } + + "Simple field ordering test." in { + ChiselStage.emitFirrtl(new ForFieldOrderingTest) + } + + "Val params to Bundle should be an Exception." in { + ChiselStage.emitFirrtl(new HasValParamsToBundle) + } + + "Should handle gen params passed to superclasses" in { + ChiselStage.emitFirrtl(new HasGenParamsPassedToSuperclasses) + } + + "Aliased fields should be detected and throw an exception, because gen: Data, with no =>" in { + intercept[AliasedAggregateFieldException] { + ChiselStage.emitFirrtl(new UsesGenFiedldsInSuperClass) + } + } + + "Error when bundle fields are hardware, such as literal values. HasHardwareFieldsInBundle" in { + val e = intercept[ExpectedChiselTypeException] { + ChiselStage.emitFirrtl(new HasHardwareFieldsInBundle) + } + e.getMessage should include( + "Bundle: BpipBadBundleWithHardware contains hardware fields: bpipWithHardwareBad: UInt<16>(244)" + ) + } + + "Aliased fields not created when using gen: => Data" in { + ChiselStage.emitFirrtl(new UsesBundleWithGeneratorField) + } + + class OptionBundle(val hasIn: Boolean) extends Bundle { + val in = if (hasIn) { + Some(Input(Bool())) + } else { + None + } + val out = Output(Bool()) + } + + class UsesBundleWithOptionFields extends Module { + val outTrue = IO(Output(new OptionBundle(hasIn = true))) + val outFalse = IO(Output(new OptionBundle(hasIn = false))) + //NOTE: The _.in.get _.in is an optional field + assertElementsMatchExpected(outTrue)("out" -> _.out, "in" -> _.in.get) + assertElementsMatchExpected(outFalse)("out" -> _.out) + } + + "plugin should work with Bundles with Option fields" in { + ChiselStage.emitFirrtl(new UsesBundleWithOptionFields) + } + + "plugin should work with enums in bundles" in { + object Enum0 extends ChiselEnum { + val s0, s1, s2 = Value + } + + ChiselStage.emitFirrtl(new Module { + val out = IO(Output(new Bundle { + val a = UInt(8.W) + val b = Bool() + val c = Enum0.Type + })) + assertElementsMatchExpected(out)("b" -> _.b, "a" -> _.a) + }) + } + + "plugin will NOT see fields that are Data but declared in some way as Any" in { + //This is not incompatible with chisel not using the plugin, but this code is considered bad practice + + ChiselStage.emitFirrtl(new Module { + val out = IO(Output(new Bundle { + val a = UInt(8.W) + val b: Any = Bool() + })) + + intercept[TestFailedException] { + assert(out.elements.keys.exists(_ == "b")) + } + }) + } + + "plugin tests should fail properly in the following cases" - { + + class BundleAB extends Bundle { + val a = Output(UInt(8.W)) + val b = Output(Bool()) + } + + def checkAssertion(checks: (BundleAB => (String, Data))*)(expectedMessage: String): Unit = { + intercept[AssertionError] { + ChiselStage.emitFirrtl(new Module { + val out = IO(new BundleAB) + assertElementsMatchExpected(out)(checks: _*) + }) + }.getMessage should include(expectedMessage) + } + + "one of the expected data values is wrong" in { + checkAssertion("b" -> _.b, "a" -> _.b)("field 'a' data field BundleElementsSpec_Anon.out.a") + } + + "one of the expected field names in wrong" in { + checkAssertion("b" -> _.b, "z" -> _.a)("field: 'a' did not match expected 'z'") + } + + "fields that are expected are not returned by the elements method" in { + checkAssertion("b" -> _.b, "a" -> _.a, "c" -> _.a)("#elements is missing the 'c' field") + } + + "fields returned by the element are not specified in the expected fields" in { + checkAssertion("b" -> _.b)("expected fields did not include 'a' field found in #elements") + } + + "multiple errors between elements method and expected fields are shown in the assertion error message" in { + checkAssertion()( + "expected fields did not include 'b' field found in #elements," + + " expected fields did not include 'a' field found in #elements" + ) + } + } + + "plugin should error correctly when bundles contain only a Option field" in { + ChiselStage.emitFirrtl(new Module { + val io = IO(new Bundle { + val foo = Input(UInt(8.W)) + val x = new Bundle { + val y = if (false) Some(Input(UInt(8.W))) else None + } + }) + assertElementsMatchExpected(io)("x" -> _.x, "foo" -> _.foo) + assertElementsMatchExpected(io.x)() + }) + } + + "plugin should handle fields using the boolean to option construct" in { + case class ALUConfig( + xLen: Int, + mul: Boolean, + b: Boolean) + + class OptionalBundle extends Bundle { + val optionBundleA = Input(UInt(3.W)) + val optionBundleB = Input(UInt(7.W)) + } + + class ALU(c: ALUConfig) extends Module { + + class BpipOptionBundle extends Bundle with IgnoreSeqInBundle { + val bpipUIntVal = Input(UInt(8.W)) + lazy val bpipUIntLazyVal = Input(UInt(8.W)) + var bpipUIntVar = Input(UInt(8.W)) + + def bpipUIntDef = Input(UInt(8.W)) + + val bpipOptionUInt = Some(Input(UInt(16.W))) + val bpipOptionOfBundle = if (c.b) Some(new OptionalBundle) else None + val bpipSeqData = Seq(UInt(8.W), UInt(8.W)) + } + + val io = IO(new BpipOptionBundle) + assertElementsMatchExpected(io)( + "bpipUIntLazyVal" -> _.bpipUIntLazyVal, + "bpipOptionUInt" -> _.bpipOptionUInt.get, + "bpipUIntVar" -> _.bpipUIntVar, + "bpipUIntVal" -> _.bpipUIntVal + ) + } + + ChiselStage.emitFirrtl(new ALU(ALUConfig(10, mul = true, b = false))) + } + + "TraceSpec test, different version found in TraceSpec.scala" in { + class Bundle0 extends Bundle { + val a = UInt(8.W) + val b = Bool() + val c = Enum0.Type + } + + class Bundle1 extends Bundle { + val a = new Bundle0 + val b = Vec(4, Vec(4, Bool())) + } + + class Module0 extends Module { + val i = IO(Input(new Bundle1)) + val o = IO(Output(new Bundle1)) + val r = Reg(new Bundle1) + o := r + r := i + + assertElementsMatchExpected(i)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(o)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(r)("b" -> _.b, "a" -> _.a) + } + + class Module1 extends Module { + val i = IO(Input(new Bundle1)) + val m0 = Module(new Module0) + m0.i := i + m0.o := DontCare + assertElementsMatchExpected(i)("b" -> _.b, "a" -> _.a) + } + + object Enum0 extends ChiselEnum { + val s0, s1, s2 = Value + } + + ChiselStage.emitFirrtl(new Module1) + } +} +/* Checks that the elements method of a bundle matches the testers idea of what the bundle field names and their + * associated data match and that they have the same number of fields in the same order + */ +object assertElementsMatchExpected { + def apply[T <: Bundle](bun: T)(checks: (T => (String, Data))*): Unit = { + val expected = checks.map { fn => fn(bun) } + val elements = bun.elements + val missingMsg = "missing field in #elements" + val extraMsg = "extra field in #elements" + val paired = elements.toSeq.zipAll(expected, missingMsg -> UInt(1.W), extraMsg -> UInt(1.W)) + val errorsStrings = paired.flatMap { + case (element, expected) => + val (elementName, elementData) = element + val (expectedName, expectedData) = expected + if (elementName == missingMsg) { + Some(s"#elements is missing the '$expectedName' field") + } else if (expectedName == extraMsg) { + Some(s"expected fields did not include '$elementName' field found in #elements") + } else if (elementName != expectedName) { + Some(s"field: '$elementName' did not match expected '$expectedName'") + } else if (elementData != expectedData) { + Some( + s"field '$elementName' data field ${elementData}(${elementData.hashCode}) did not match expected $expectedData(${expectedData.hashCode})" + ) + } else { + None + } + } + assert(errorsStrings.isEmpty, s"Bundle: ${bun.getClass.getName}: " + errorsStrings.mkString(", ")) + } +} diff --git a/src/test/scala/chiselTests/BundleSpec.scala b/src/test/scala/chiselTests/BundleSpec.scala index 720f877f..5dcbbefa 100644 --- a/src/test/scala/chiselTests/BundleSpec.scala +++ b/src/test/scala/chiselTests/BundleSpec.scala @@ -26,6 +26,10 @@ trait BundleSpecUtils { val bar = Seq(UInt(16.W), UInt(8.W), UInt(4.W)) } + class BadSeqBundleWithIgnoreSeqInBundle extends Bundle with IgnoreSeqInBundle { + val bar = Seq(UInt(16.W), UInt(8.W), UInt(4.W)) + } + class MyModule(output: Bundle, input: Bundle) extends Module { val io = IO(new Bundle { val in = Input(input) @@ -87,7 +91,7 @@ class BundleSpec extends ChiselFlatSpec with BundleSpecUtils with Utils { new BasicTester { val m = Module(new Module { val io = IO(new Bundle { - val b = new BadSeqBundle with IgnoreSeqInBundle + val b = new BadSeqBundleWithIgnoreSeqInBundle }) }) stop() @@ -141,7 +145,7 @@ class BundleSpec extends ChiselFlatSpec with BundleSpecUtils with Utils { out := in } } - }).getMessage should include("must be a Chisel type, not hardware") + }).getMessage should include("MyBundle contains hardware fields: foo: UInt<7>(123)") } "Bundles" should "not recursively contain aggregates with bound hardware" in { (the[ChiselException] thrownBy extractCause[ChiselException] { @@ -153,7 +157,7 @@ class BundleSpec extends ChiselFlatSpec with BundleSpecUtils with Utils { out := in } } - }).getMessage should include("must be a Chisel type, not hardware") + }).getMessage should include("Bundle: MyBundle contains hardware fields: foo: BundleSpec_Anon.out") } "Unbound bundles sharing a field" should "not error" in { ChiselStage.elaborate { -- cgit v1.2.3 From cb77b061e835044b4f3a2b718fb7ce3971b5d06e Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Tue, 1 Feb 2022 20:55:35 +0000 Subject: Optional clock param for memory ports (#2333) (#2382) Warn if clock at memory instantiation differs from clock bound at port creation and port clock is not manually passed Co-authored-by: Jack Koenig (cherry picked from commit 465805ec7b2696a985eaa12cf9c6868f11ac2931) Co-authored-by: Aditya Naik <91489422+adkian-sifive@users.noreply.github.com>--- core/src/main/scala/chisel3/Mem.scala | 161 ++++++++++++++++++--- .../internal/sourceinfo/SourceInfoTransform.scala | 4 + src/test/scala/chiselTests/MultiClockSpec.scala | 110 +++++++++++++- 3 files changed, 255 insertions(+), 20 deletions(-) diff --git a/core/src/main/scala/chisel3/Mem.scala b/core/src/main/scala/chisel3/Mem.scala index ff5072ad..4b7c0815 100644 --- a/core/src/main/scala/chisel3/Mem.scala +++ b/core/src/main/scala/chisel3/Mem.scala @@ -56,6 +56,7 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) with SourceInfoDoc { _parent.foreach(_.addId(this)) + private val clockInst: Clock = Builder.forcedClock // REVIEW TODO: make accessors (static/dynamic, read/write) combinations consistent. /** Creates a read accessor into the memory with static addressing. See the @@ -63,17 +64,17 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) */ def apply(x: BigInt): T = macro SourceInfoTransform.xArg - /** Creates a read accessor into the memory with static addressing. See the - * class documentation of the memory for more detailed information. - */ - def apply(x: Int): T = macro SourceInfoTransform.xArg - /** @group SourceInfoTransformMacro */ def do_apply(idx: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = { require(idx >= 0 && idx < length) - apply(idx.asUInt) + apply(idx.asUInt, Builder.forcedClock) } + /** Creates a read accessor into the memory with static addressing. See the + * class documentation of the memory for more detailed information. + */ + def apply(x: Int): T = macro SourceInfoTransform.xArg + /** @group SourceInfoTransformMacro */ def do_apply(idx: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = do_apply(BigInt(idx))(sourceInfo, compileOptions) @@ -85,7 +86,12 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) /** @group SourceInfoTransformMacro */ def do_apply(idx: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = - makePort(sourceInfo, idx, MemPortDirection.INFER) + do_apply_impl(idx, Builder.forcedClock, MemPortDirection.INFER, true) + + def apply(x: UInt, y: Clock): T = macro SourceInfoTransform.xyArg + + def do_apply(idx: UInt, clock: Clock)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = + do_apply_impl(idx, clock, MemPortDirection.INFER, false) /** Creates a read accessor into the memory with dynamic addressing. See the * class documentation of the memory for more detailed information. @@ -94,16 +100,71 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) /** @group SourceInfoTransformMacro */ def do_read(idx: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = - makePort(sourceInfo, idx, MemPortDirection.READ) + do_apply_impl(idx, Builder.forcedClock, MemPortDirection.READ, true) + + /** Creates a read accessor into the memory with dynamic addressing. + * Takes a clock parameter to bind a clock that may be different + * from the implicit clock. See the class documentation of the memory + * for more detailed information. + */ + def read(x: UInt, y: Clock): T = macro SourceInfoTransform.xyArg + + /** @group SourceInfoTransformMacro */ + def do_read(idx: UInt, clock: Clock)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = + do_apply_impl(idx, clock, MemPortDirection.READ, false) + + protected def do_apply_impl( + idx: UInt, + clock: Clock, + dir: MemPortDirection, + warn: Boolean + )( + implicit sourceInfo: SourceInfo, + compileOptions: CompileOptions + ): T = { + if (warn && clock != clockInst) { + Builder.warning( + "The clock used to initialize the memory is different than the one used to initialize the port. " + + "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0" + ) + } + makePort(sourceInfo, idx, dir, clock) + } /** Creates a write accessor into the memory. * * @param idx memory element index to write into * @param data new data to write */ - def write(idx: UInt, data: T)(implicit compileOptions: CompileOptions): Unit = { + def write(idx: UInt, data: T)(implicit compileOptions: CompileOptions): Unit = + write_impl(idx, data, Builder.forcedClock, true) + + /** Creates a write accessor into the memory with a clock + * that may be different from the implicit clock. + * + * @param idx memory element index to write into + * @param data new data to write + * @param clock clock to bind to this accessor + */ + def write(idx: UInt, data: T, clock: Clock)(implicit compileOptions: CompileOptions): Unit = + write_impl(idx, data, clock, false) + + private def write_impl( + idx: UInt, + data: T, + clock: Clock, + warn: Boolean + )( + implicit compileOptions: CompileOptions + ): Unit = { + if (warn && clock != clockInst) { + Builder.warning( + "The clock used to initialize the memory is different than the one used to initialize the port. " + + "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0" + ) + } implicit val sourceInfo = UnlocatableSourceInfo - makePort(UnlocatableSourceInfo, idx, MemPortDirection.WRITE) := data + makePort(UnlocatableSourceInfo, idx, MemPortDirection.WRITE, clock) := data } /** Creates a masked write accessor into the memory. @@ -122,9 +183,49 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) )( implicit evidence: T <:< Vec[_], compileOptions: CompileOptions + ): Unit = + masked_write_impl(idx, data, mask, Builder.forcedClock, true) + + /** Creates a masked write accessor into the memory with a clock + * that may be different from the implicit clock. + * + * @param idx memory element index to write into + * @param data new data to write + * @param mask write mask as a Seq of Bool: a write to the Vec element in + * memory is only performed if the corresponding mask index is true. + * @param clock clock to bind to this accessor + * + * @note this is only allowed if the memory's element data type is a Vec + */ + def write( + idx: UInt, + data: T, + mask: Seq[Bool], + clock: Clock + )( + implicit evidence: T <:< Vec[_], + compileOptions: CompileOptions + ): Unit = + masked_write_impl(idx, data, mask, clock, false) + + private def masked_write_impl( + idx: UInt, + data: T, + mask: Seq[Bool], + clock: Clock, + warn: Boolean + )( + implicit evidence: T <:< Vec[_], + compileOptions: CompileOptions ): Unit = { implicit val sourceInfo = UnlocatableSourceInfo - val accessor = makePort(sourceInfo, idx, MemPortDirection.WRITE).asInstanceOf[Vec[Data]] + if (warn && clock != clockInst) { + Builder.warning( + "The clock used to initialize the memory is different than the one used to initialize the port. " + + "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0" + ) + } + val accessor = makePort(sourceInfo, idx, MemPortDirection.WRITE, clock).asInstanceOf[Vec[Data]] val dataVec = data.asInstanceOf[Vec[Data]] if (accessor.length != dataVec.length) { Builder.error(s"Mem write data must contain ${accessor.length} elements (found ${dataVec.length})") @@ -139,7 +240,8 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) private def makePort( sourceInfo: SourceInfo, idx: UInt, - dir: MemPortDirection + dir: MemPortDirection, + clock: Clock )( implicit compileOptions: CompileOptions ): T = { @@ -147,7 +249,7 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) val i = Vec.truncateIndex(idx, length)(sourceInfo, compileOptions) val port = pushCommand( - DefMemPort(sourceInfo, t.cloneTypeFull, Node(this), dir, i.ref, Builder.forcedClock.ref) + DefMemPort(sourceInfo, t.cloneTypeFull, Node(this), dir, i.ref, clock.ref) ).id // Bind each element of port to being a MemoryPort port.bind(MemoryPortBinding(Builder.forcedUserModule, Builder.currentWhen)) @@ -244,23 +346,44 @@ object SyncReadMem { */ sealed class SyncReadMem[T <: Data] private (t: T, n: BigInt, val readUnderWrite: SyncReadMem.ReadUnderWrite) extends MemBase[T](t, n) { + + override def read(x: UInt): T = macro SourceInfoTransform.xArg + + /** @group SourceInfoTransformMacro */ + override def do_read(idx: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = + do_read(idx = idx, en = true.B) + def read(x: UInt, en: Bool): T = macro SourceInfoTransform.xEnArg /** @group SourceInfoTransformMacro */ - def do_read(addr: UInt, enable: Bool)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = { + def do_read(idx: UInt, en: Bool)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = + _read_impl(idx, en, Builder.forcedClock, true) + + def read(idx: UInt, en: Bool, clock: Clock): T = macro SourceInfoTransform.xyzArg + + /** @group SourceInfoTransformMacro */ + def do_read(idx: UInt, en: Bool, clock: Clock)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = + _read_impl(idx, en, clock, false) + + /** @group SourceInfoTransformMacro */ + private def _read_impl( + addr: UInt, + enable: Bool, + clock: Clock, + warn: Boolean + )( + implicit sourceInfo: SourceInfo, + compileOptions: CompileOptions + ): T = { val a = Wire(UInt()) a := DontCare var port: Option[T] = None when(enable) { a := addr - port = Some(super.do_read(a)) + port = Some(super.do_apply_impl(a, clock, MemPortDirection.READ, warn)) } port.get } - - /** @group SourceInfoTransformMacro */ - override def do_read(idx: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions) = - do_read(addr = idx, enable = true.B) // note: we implement do_read(addr) for SyncReadMem in terms of do_read(addr, en) in order to ensure that // `mem.read(addr)` will always behave the same as `mem.read(addr, true.B)` } diff --git a/macros/src/main/scala/chisel3/internal/sourceinfo/SourceInfoTransform.scala b/macros/src/main/scala/chisel3/internal/sourceinfo/SourceInfoTransform.scala index 01e0acd6..3e310774 100644 --- a/macros/src/main/scala/chisel3/internal/sourceinfo/SourceInfoTransform.scala +++ b/macros/src/main/scala/chisel3/internal/sourceinfo/SourceInfoTransform.scala @@ -200,6 +200,10 @@ class SourceInfoTransform(val c: Context) extends AutoSourceTransform { q"$thisObj.$doFuncTerm($x, $y)($implicitSourceInfo, $implicitCompileOptions)" } + def xyzArg(idx: c.Tree, en: c.Tree, clock: c.Tree): c.Tree = { + q"$thisObj.$doFuncTerm($idx, $en, $clock)($implicitSourceInfo, $implicitCompileOptions)" + } + def xEnArg(x: c.Tree, en: c.Tree): c.Tree = { q"$thisObj.$doFuncTerm($x, $en)($implicitSourceInfo, $implicitCompileOptions)" } diff --git a/src/test/scala/chiselTests/MultiClockSpec.scala b/src/test/scala/chiselTests/MultiClockSpec.scala index 2553f3b3..381b4009 100644 --- a/src/test/scala/chiselTests/MultiClockSpec.scala +++ b/src/test/scala/chiselTests/MultiClockSpec.scala @@ -113,7 +113,7 @@ class MultiClockMemTest extends BasicTester { when(done) { stop() } } -class MultiClockSpec extends ChiselFlatSpec { +class MultiClockSpec extends ChiselFlatSpec with Utils { "withClock" should "scope the clock of registers" in { assertTesterPasses(new ClockDividerTest) @@ -129,6 +129,114 @@ class MultiClockSpec extends ChiselFlatSpec { }) } + "Differing clocks at memory and port instantiation" should "warn" in { + class modMemDifferingClock extends Module { + val myClock = IO(Input(Clock())) + val mem = withClock(myClock) { Mem(4, UInt(8.W)) } + val port0 = mem(0.U) + } + val (logMemDifferingClock, _) = grabLog(ChiselStage.elaborate(new modMemDifferingClock)) + logMemDifferingClock should include("memory is different") + + class modSyncReadMemDifferingClock extends Module { + val myClock = IO(Input(Clock())) + val mem = withClock(myClock) { SyncReadMem(4, UInt(8.W)) } + val port0 = mem(0.U) + } + val (logSyncReadMemDifferingClock, _) = grabLog(ChiselStage.elaborate(new modSyncReadMemDifferingClock)) + logSyncReadMemDifferingClock should include("memory is different") + } + + "Differing clocks at memory and write accessor instantiation" should "warn" in { + class modMemWriteDifferingClock extends Module { + val myClock = IO(Input(Clock())) + val mem = withClock(myClock) { Mem(4, UInt(8.W)) } + mem(1.U) := 1.U + } + val (logMemWriteDifferingClock, _) = grabLog(ChiselStage.elaborate(new modMemWriteDifferingClock)) + logMemWriteDifferingClock should include("memory is different") + + class modSyncReadMemWriteDifferingClock extends Module { + val myClock = IO(Input(Clock())) + val mem = withClock(myClock) { SyncReadMem(4, UInt(8.W)) } + mem.write(1.U, 1.U) + } + val (logSyncReadMemWriteDifferingClock, _) = grabLog(ChiselStage.elaborate(new modSyncReadMemWriteDifferingClock)) + logSyncReadMemWriteDifferingClock should include("memory is different") + } + + "Differing clocks at memory and read accessor instantiation" should "warn" in { + class modMemReadDifferingClock extends Module { + val myClock = IO(Input(Clock())) + val mem = withClock(myClock) { Mem(4, UInt(8.W)) } + val readVal = mem.read(0.U) + } + val (logMemReadDifferingClock, _) = grabLog(ChiselStage.elaborate(new modMemReadDifferingClock)) + logMemReadDifferingClock should include("memory is different") + + class modSyncReadMemReadDifferingClock extends Module { + val myClock = IO(Input(Clock())) + val mem = withClock(myClock) { SyncReadMem(4, UInt(8.W)) } + val readVal = mem.read(0.U) + } + val (logSyncReadMemReadDifferingClock, _) = grabLog(ChiselStage.elaborate(new modSyncReadMemReadDifferingClock)) + logSyncReadMemReadDifferingClock should include("memory is different") + } + + "Passing clock parameter to memory port instantiation" should "not warn" in { + class modMemPortClock extends Module { + val myClock = IO(Input(Clock())) + val mem = Mem(4, UInt(8.W)) + val port0 = mem(0.U, myClock) + } + val (logMemPortClock, _) = grabLog(ChiselStage.elaborate(new modMemPortClock)) + (logMemPortClock should not).include("memory is different") + + class modSyncReadMemPortClock extends Module { + val myClock = IO(Input(Clock())) + val mem = SyncReadMem(4, UInt(8.W)) + val port0 = mem(0.U, myClock) + } + val (logSyncReadMemPortClock, _) = grabLog(ChiselStage.elaborate(new modSyncReadMemPortClock)) + (logSyncReadMemPortClock should not).include("memory is different") + } + + "Passing clock parameter to memory write accessor" should "not warn" in { + class modMemWriteClock extends Module { + val myClock = IO(Input(Clock())) + val mem = Mem(4, UInt(8.W)) + mem.write(0.U, 0.U, myClock) + } + val (logMemWriteClock, _) = grabLog(ChiselStage.elaborate(new modMemWriteClock)) + (logMemWriteClock should not).include("memory is different") + + class modSyncReadMemWriteClock extends Module { + val myClock = IO(Input(Clock())) + val mem = SyncReadMem(4, UInt(8.W)) + mem.write(0.U, 0.U, myClock) + } + val (logSyncReadMemWriteClock, _) = grabLog(ChiselStage.elaborate(new modSyncReadMemWriteClock)) + (logSyncReadMemWriteClock should not).include("memory is different") + } + + "Passing clock parameter to memory read accessor" should "not warn" in { + class modMemReadClock extends Module { + val myClock = IO(Input(Clock())) + val mem = Mem(4, UInt(8.W)) + val readVal = mem.read(0.U, myClock) + } + val (logMemReadClock, _) = grabLog(ChiselStage.elaborate(new modMemReadClock)) + (logMemReadClock should not).include("memory is different") + + class modSyncReadMemReadClock extends Module { + val myClock = IO(Input(Clock())) + val mem = SyncReadMem(4, UInt(8.W)) + val readVal = mem.read(0.U, myClock) + } + val (logSyncReadMemReadClock, _) = grabLog(ChiselStage.elaborate(new modSyncReadMemReadClock)) + (logSyncReadMemReadClock should not).include("memory is different") + } + "withReset" should "scope the reset of registers" in { assertTesterPasses(new WithResetTest) } -- cgit v1.2.3 From 0eba2eb109910c1e4b980ccfc7f3ae85a8d0f223 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Tue, 1 Feb 2022 21:37:08 +0000 Subject: Improve error reporting (backport #2376) (#2379) * Improve error reporting (#2376) * Do not trim stack traces of exceptions with no stack trace This prevents us from accidentally giving stack traces to exceptions that don't have them and giving misleading messages telling users to use --full-stacktrace when it won't actually do anything. Also deprecate ChiselException.chiselStackTrace which is no longer being used anywhere in this codebase. * Add exception class for multiple-errors reported New chisel3.internal.Errors replaces old anonymous class that would show up as chisel3.internal.ErrorLog$$anon$1 in error messages. * Add new option --throw-on-first-error This tells Chisel not to aggregate recoverable errors but instead to throw an exception on the first one. This gives a stack trace for users who need it for debugging. (cherry picked from commit ff2e9c92247b3848659fa09fdd53ddde2120036a) * Waive MiMa false positives The waived change is to a package private constructor. Co-authored-by: Jack Koenig Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>--- build.sbt | 6 ++ .../experimental/hierarchy/Definition.scala | 2 +- core/src/main/scala/chisel3/internal/Builder.scala | 5 +- core/src/main/scala/chisel3/internal/Error.scala | 72 +++++++++++-------- .../chisel3/aop/injecting/InjectingAspect.scala | 6 +- .../scala/chisel3/stage/ChiselAnnotations.scala | 18 +++++ src/main/scala/chisel3/stage/ChiselCli.scala | 1 + src/main/scala/chisel3/stage/ChiselOptions.scala | 3 + src/main/scala/chisel3/stage/package.scala | 5 +- .../scala/chisel3/stage/phases/Elaborate.scala | 14 +++- .../scala/chiselTests/stage/ChiselStageSpec.scala | 83 ++++++++++++++++++++++ 11 files changed, 176 insertions(+), 39 deletions(-) diff --git a/build.sbt b/build.sbt index f5f178fd..e63496a5 100644 --- a/build.sbt +++ b/build.sbt @@ -1,5 +1,7 @@ // See LICENSE for license details. +import com.typesafe.tools.mima.core._ + enablePlugins(SiteScaladocPlugin) val defaultVersions = Map( @@ -197,6 +199,10 @@ lazy val chisel = (project in file(".")). aggregate(macros, core, plugin). settings( mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3" % "3.5.0"), + mimaBinaryIssueFilters ++= Seq( + // Modified package private methods (https://github.com/lightbend/mima/issues/53) + ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.stage.ChiselOptions.this"), + ), libraryDependencies += defaultVersions("treadle") % "test", Test / scalacOptions += "-P:chiselplugin:genBundleElements", scalacOptions in Test ++= Seq("-language:reflectiveCalls"), diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala index b498daf0..59b4c692 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala @@ -100,7 +100,7 @@ object Definition extends SourceInfoDoc { implicit sourceInfo: SourceInfo, compileOptions: CompileOptions ): Definition[T] = { - val dynamicContext = new DynamicContext(Nil) + val dynamicContext = new DynamicContext(Nil, Builder.captureContext().throwOnFirstError) 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 fb6ebcc7..a00505a8 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -361,7 +361,7 @@ private[chisel3] class ChiselContext() { val viewNamespace = Namespace.empty } -private[chisel3] class DynamicContext(val annotationSeq: AnnotationSeq) { +private[chisel3] class DynamicContext(val annotationSeq: AnnotationSeq, val throwOnFirstError: Boolean) { val globalNamespace = Namespace.empty val components = ArrayBuffer[Component]() val annotations = ArrayBuffer[ChiselAnnotation]() @@ -653,7 +653,8 @@ private[chisel3] object Builder extends LazyLogging { def errors: ErrorLog = dynamicContext.errors def error(m: => String): Unit = { - if (dynamicContextVar.value.isDefined) { + // If --throw-on-first-error is requested, throw an exception instead of aggregating errors + if (dynamicContextVar.value.isDefined && !dynamicContextVar.value.get.throwOnFirstError) { errors.error(m) } else { throwException(m) diff --git a/core/src/main/scala/chisel3/internal/Error.scala b/core/src/main/scala/chisel3/internal/Error.scala index c42f39ed..62086870 100644 --- a/core/src/main/scala/chisel3/internal/Error.scala +++ b/core/src/main/scala/chisel3/internal/Error.scala @@ -4,6 +4,7 @@ package chisel3.internal import scala.annotation.tailrec import scala.collection.mutable.{ArrayBuffer, LinkedHashMap} +import scala.util.control.NoStackTrace import _root_.logger.Logger object ExceptionHelpers { @@ -46,31 +47,36 @@ object ExceptionHelpers { } // Step 1: Remove elements from the top in the package trimlist - ((a: Array[StackTraceElement]) => a.dropWhile(inTrimlist)) - // Step 2: Optionally remove elements from the bottom until the anchor - .andThen(_.reverse) - .andThen(a => - anchor match { - case Some(b) => a.dropWhile(ste => !ste.getClassName.startsWith(b)) - case None => a - } - ) - // Step 3: Remove elements from the bottom in the package trimlist - .andThen(_.dropWhile(inTrimlist)) - // Step 4: Reverse back to the original order - .andThen(_.reverse.toArray) - // Step 5: Add ellipsis stack trace elements and "--full-stacktrace" info - .andThen(a => - ellipsis() +: - a :+ - ellipsis() :+ - ellipsis( - Some("Stack trace trimmed to user code only. Rerun with --full-stacktrace to see the full stack trace") - ) - ) - // Step 5: Mutate the stack trace in this exception - .andThen(throwable.setStackTrace(_)) - .apply(throwable.getStackTrace) + val trimStackTrace = + ((a: Array[StackTraceElement]) => a.dropWhile(inTrimlist)) + // Step 2: Optionally remove elements from the bottom until the anchor + .andThen(_.reverse) + .andThen(a => + anchor match { + case Some(b) => a.dropWhile(ste => !ste.getClassName.startsWith(b)) + case None => a + } + ) + // Step 3: Remove elements from the bottom in the package trimlist + .andThen(_.dropWhile(inTrimlist)) + // Step 4: Reverse back to the original order + .andThen(_.reverse.toArray) + // Step 5: Add ellipsis stack trace elements and "--full-stacktrace" info + .andThen(a => + ellipsis() +: + a :+ + ellipsis() :+ + ellipsis( + Some("Stack trace trimmed to user code only. Rerun with --full-stacktrace to see the full stack trace") + ) + ) + // Step 5: Mutate the stack trace in this exception + .andThen(throwable.setStackTrace(_)) + + val stackTrace = throwable.getStackTrace + if (stackTrace.nonEmpty) { + trimStackTrace(stackTrace) + } } } @@ -80,11 +86,11 @@ object ExceptionHelpers { class ChiselException(message: String, cause: Throwable = null) extends Exception(message, cause, true, true) { /** Package names whose stack trace elements should be trimmed when generating a trimmed stack trace */ - @deprecated("Use ExceptionHelpers.packageTrimlist. This will be removed in Chisel 3.6", "3.5") + @deprecated("Use ExceptionHelpers.packageTrimlist. This will be removed in Chisel 3.6", "Chisel 3.5") val blacklistPackages: Set[String] = Set("chisel3", "scala", "java", "sun", "sbt") /** The object name of Chisel's internal `Builder`. Everything stack trace element after this will be trimmed. */ - @deprecated("Use ExceptionHelpers.builderName. This will be removed in Chisel 3.6", "3.5") + @deprecated("Use ExceptionHelpers.builderName. This will be removed in Chisel 3.6", "Chisel 3.5") val builderName: String = chisel3.internal.Builder.getClass.getName /** Examine a [[Throwable]], to extract all its causes. Innermost cause is first. @@ -138,6 +144,11 @@ class ChiselException(message: String, cause: Throwable = null) extends Exceptio trimmedReverse.toIndexedSeq.reverse.toArray } + @deprecated( + "Use extension method trimStackTraceToUserCode defined in ExceptionHelpers.ThrowableHelpers. " + + "This will be removed in Chisel 3.6", + "Chisel 3.5.0" + ) def chiselStackTrace: String = { val trimmed = trimmedStackTrace(likelyCause) @@ -151,6 +162,7 @@ class ChiselException(message: String, cause: Throwable = null) extends Exceptio sw.toString } } +private[chisel3] class Errors(message: String) extends ChiselException(message) with NoStackTrace private[chisel3] object throwException { def apply(s: String, t: Throwable = null): Nothing = @@ -234,8 +246,10 @@ private[chisel3] class ErrorLog { } if (!allErrors.isEmpty) { - throw new ChiselException("Fatal errors during hardware elaboration. Look above for error list.") - with scala.util.control.NoStackTrace + throw new Errors( + "Fatal errors during hardware elaboration. Look above for error list. " + + "Rerun with --throw-on-first-error if you wish to see a stack trace." + ) } else { // No fatal errors, clear accumulated warnings since they've been reported errors.clear() diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala index abe208cf..ba873c23 100644 --- a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala +++ b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala @@ -6,9 +6,10 @@ import chisel3.{withClockAndReset, Module, ModuleAspect, RawModule} import chisel3.aop._ import chisel3.internal.{Builder, DynamicContext} import chisel3.internal.firrtl.DefModule -import chisel3.stage.DesignAnnotation +import chisel3.stage.{ChiselOptions, DesignAnnotation} import firrtl.annotations.ModuleTarget import firrtl.stage.RunFirrtlTransformAnnotation +import firrtl.options.Viewer.view import firrtl.{ir, _} import scala.collection.mutable @@ -56,7 +57,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 dynamicContext = new DynamicContext(annotationsInAspect) + val chiselOptions = view[ChiselOptions](annotationsInAspect) + val dynamicContext = new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError) // 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 c5811813..e046319d 100644 --- a/src/main/scala/chisel3/stage/ChiselAnnotations.scala +++ b/src/main/scala/chisel3/stage/ChiselAnnotations.scala @@ -61,6 +61,24 @@ case object PrintFullStackTraceAnnotation } +/** On recoverable errors, this will cause Chisel to throw an exception instead of continuing. + */ +case object ThrowOnFirstErrorAnnotation + extends NoTargetAnnotation + with ChiselOption + with HasShellOptions + with Unserializable { + + val options = Seq( + new ShellOption[Unit]( + longOption = "throw-on-first-error", + toAnnotationSeq = _ => Seq(ThrowOnFirstErrorAnnotation), + helpText = "Throw an exception on the first error instead of continuing" + ) + ) + +} + /** 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 26b032bf..f38bf50c 100644 --- a/src/main/scala/chisel3/stage/ChiselCli.scala +++ b/src/main/scala/chisel3/stage/ChiselCli.scala @@ -9,6 +9,7 @@ trait ChiselCli { this: Shell => Seq( NoRunFirrtlCompilerAnnotation, PrintFullStackTraceAnnotation, + ThrowOnFirstErrorAnnotation, ChiselOutputFileAnnotation, ChiselGeneratorAnnotation ) diff --git a/src/main/scala/chisel3/stage/ChiselOptions.scala b/src/main/scala/chisel3/stage/ChiselOptions.scala index ed4d0a2f..7e4305fa 100644 --- a/src/main/scala/chisel3/stage/ChiselOptions.scala +++ b/src/main/scala/chisel3/stage/ChiselOptions.scala @@ -7,12 +7,14 @@ 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) { private[stage] def copy( runFirrtlCompiler: Boolean = runFirrtlCompiler, printFullStackTrace: Boolean = printFullStackTrace, + throwOnFirstError: Boolean = throwOnFirstError, outputFile: Option[String] = outputFile, chiselCircuit: Option[Circuit] = chiselCircuit ): ChiselOptions = { @@ -20,6 +22,7 @@ class ChiselOptions private[stage] ( new ChiselOptions( runFirrtlCompiler = runFirrtlCompiler, printFullStackTrace = printFullStackTrace, + throwOnFirstError = throwOnFirstError, outputFile = outputFile, chiselCircuit = chiselCircuit ) diff --git a/src/main/scala/chisel3/stage/package.scala b/src/main/scala/chisel3/stage/package.scala index bf03e2df..b1064c05 100644 --- a/src/main/scala/chisel3/stage/package.scala +++ b/src/main/scala/chisel3/stage/package.scala @@ -15,8 +15,9 @@ package object stage { def view(options: AnnotationSeq): ChiselOptions = options.collect { case a: ChiselOption => a } .foldLeft(new ChiselOptions()) { (c, x) => x match { - case _: NoRunFirrtlCompilerAnnotation.type => c.copy(runFirrtlCompiler = false) - case _: PrintFullStackTraceAnnotation.type => c.copy(printFullStackTrace = true) + 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)) } diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/src/main/scala/chisel3/stage/phases/Elaborate.scala index 2cfb3200..55331cb4 100644 --- a/src/main/scala/chisel3/stage/phases/Elaborate.scala +++ b/src/main/scala/chisel3/stage/phases/Elaborate.scala @@ -5,7 +5,13 @@ package chisel3.stage.phases import chisel3.Module import chisel3.internal.ExceptionHelpers.ThrowableHelpers import chisel3.internal.{Builder, DynamicContext} -import chisel3.stage.{ChiselCircuitAnnotation, ChiselGeneratorAnnotation, ChiselOptions, DesignAnnotation} +import chisel3.stage.{ + ChiselCircuitAnnotation, + ChiselGeneratorAnnotation, + ChiselOptions, + DesignAnnotation, + ThrowOnFirstErrorAnnotation +} import firrtl.AnnotationSeq import firrtl.options.Phase import firrtl.options.Viewer.view @@ -21,14 +27,16 @@ class Elaborate extends Phase { def transform(annotations: AnnotationSeq): AnnotationSeq = annotations.flatMap { case ChiselGeneratorAnnotation(gen) => + val chiselOptions = view[ChiselOptions](annotations) try { - val (circuit, dut) = Builder.build(Module(gen()), new DynamicContext(annotations)) + val (circuit, dut) = + Builder.build(Module(gen()), new DynamicContext(annotations, chiselOptions.throwOnFirstError)) 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 */ case scala.util.control.NonFatal(a) => - if (!view[ChiselOptions](annotations).printFullStackTrace) { + if (!chiselOptions.printFullStackTrace) { a.trimStackTraceToUserCode() } throw (a) diff --git a/src/test/scala/chiselTests/stage/ChiselStageSpec.scala b/src/test/scala/chiselTests/stage/ChiselStageSpec.scala index e88a2385..f0f383da 100644 --- a/src/test/scala/chiselTests/stage/ChiselStageSpec.scala +++ b/src/test/scala/chiselTests/stage/ChiselStageSpec.scala @@ -34,6 +34,14 @@ object ChiselStageSpec { assert(false, "User threw an exception") } + class UserExceptionNoStackTrace extends RawModule { + throw new Exception("Something bad happened") with scala.util.control.NoStackTrace + } + + class RecoverableError extends RawModule { + 3.U >> -1 + } + } class ChiselStageSpec extends AnyFlatSpec with Matchers with Utils { @@ -169,6 +177,32 @@ class ChiselStageSpec extends AnyFlatSpec with Matchers with Utils { (exception.getStackTrace.mkString("\n") should not).include("java") } + it should "NOT add a stack trace to an exception with no stack trace" in { + val exception = intercept[java.lang.Exception] { + ChiselStage.emitChirrtl(new UserExceptionNoStackTrace) + } + + val message = exception.getMessage + info("The exception includes the user's message") + message should include("Something bad happened") + + info("The exception should not contain a stack trace") + exception.getStackTrace should be(Array()) + } + + it should "NOT include a stack trace for recoverable errors" in { + val exception = intercept[java.lang.Exception] { + ChiselStage.emitChirrtl(new RecoverableError) + } + + val message = exception.getMessage + info("The exception includes the standard error message") + message should include("Fatal errors during hardware elaboration. Look above for error list.") + + info("The exception should not contain a stack trace") + exception.getStackTrace should be(Array()) + } + behavior.of("ChiselStage exception handling") it should "truncate a user exception" in { @@ -207,4 +241,53 @@ class ChiselStageSpec extends AnyFlatSpec with Matchers with Utils { exception.getStackTrace.mkString("\n") should include("java") } + it should "NOT add a stack trace to an exception with no stack trace" in { + val exception = intercept[java.lang.Exception] { + (new ChiselStage).emitChirrtl(new UserExceptionNoStackTrace) + } + + val message = exception.getMessage + info("The exception includes the user's message") + message should include("Something bad happened") + + info("The exception should not contain a stack trace") + exception.getStackTrace should be(Array()) + } + + it should "NOT include a stack trace for recoverable errors" in { + val exception = intercept[java.lang.Exception] { + (new ChiselStage).emitChirrtl(new RecoverableError) + } + + val message = exception.getMessage + info("The exception includes the standard error message") + message should include("Fatal errors during hardware elaboration. Look above for error list.") + + info("The exception should not contain a stack trace") + exception.getStackTrace should be(Array()) + } + + it should "include a stack trace for recoverable errors with '--throw-on-first-error'" in { + val exception = intercept[java.lang.Exception] { + (new ChiselStage).emitChirrtl(new RecoverableError, Array("--throw-on-first-error")) + } + + val stackTrace = exception.getStackTrace.mkString("\n") + info("The exception should contain a truncated stack trace") + stackTrace shouldNot include("java") + + info("The stack trace include information about running --full-stacktrace") + stackTrace should include("--full-stacktrace") + } + + it should "include an untruncated stack trace for recoverable errors when given both '--throw-on-first-error' and '--full-stacktrace'" in { + val exception = intercept[java.lang.Exception] { + val args = Array("--throw-on-first-error", "--full-stacktrace") + (new ChiselStage).emitChirrtl(new RecoverableError, args) + } + + val stackTrace = exception.getStackTrace.mkString("\n") + info("The exception should contain a truncated stack trace") + stackTrace should include("java") + } } -- cgit v1.2.3 From f5c51d7de7b5fca48106d117b9e4ac2ee488f334 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Wed, 2 Feb 2022 01:13:30 +0000 Subject: Add Scala 2.13.8 to plugin cross-compilation (#2385) (#2386) (cherry picked from commit 551854b223954f359156582c4219948dbb328857) Co-authored-by: Jack Koenig --- build.sbt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/build.sbt b/build.sbt index e63496a5..4fe7122f 100644 --- a/build.sbt +++ b/build.sbt @@ -116,7 +116,8 @@ lazy val pluginScalaVersions = Seq( "2.13.4", "2.13.5", "2.13.6", - "2.13.7" + "2.13.7", + "2.13.8" ) lazy val plugin = (project in file("plugin")). -- cgit v1.2.3 From 6048c973f0c1c6e80a7a9e8ef6cec71ef0695e68 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Thu, 3 Feb 2022 02:42:14 +0000 Subject: Tweak new mem port clock warnings (#2389) (#2391) Use Builder.deprecated instead of Builder.warning so that the warnings are aggregated by source locator to prevent spamming the screen with duplicated warnings. (cherry picked from commit 538e223ae81c8b66a4123303f6dab61c874aaa1e) Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/Mem.scala | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/core/src/main/scala/chisel3/Mem.scala b/core/src/main/scala/chisel3/Mem.scala index 4b7c0815..3f37308c 100644 --- a/core/src/main/scala/chisel3/Mem.scala +++ b/core/src/main/scala/chisel3/Mem.scala @@ -9,7 +9,7 @@ import firrtl.{ir => fir} import chisel3.internal._ import chisel3.internal.Builder.pushCommand import chisel3.internal.firrtl._ -import chisel3.internal.sourceinfo.{MemTransform, SourceInfo, SourceInfoTransform, UnlocatableSourceInfo} +import chisel3.internal.sourceinfo.{MemTransform, SourceInfo, SourceInfoTransform, SourceLine, UnlocatableSourceInfo} object Mem { @@ -57,6 +57,16 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) _parent.foreach(_.addId(this)) private val clockInst: Clock = Builder.forcedClock + + protected def clockWarning(sourceInfo: Option[SourceInfo]): Unit = { + // Turn into pretty String if possible, if not, Builder.deprecated will find one via stack trace + val infoStr = sourceInfo.collect { case SourceLine(file, line, col) => s"$file:$line:$col" } + Builder.deprecated( + "The clock used to initialize the memory is different than the one used to initialize the port. " + + "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0", + infoStr + ) + } // REVIEW TODO: make accessors (static/dynamic, read/write) combinations consistent. /** Creates a read accessor into the memory with static addressing. See the @@ -123,10 +133,7 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) compileOptions: CompileOptions ): T = { if (warn && clock != clockInst) { - Builder.warning( - "The clock used to initialize the memory is different than the one used to initialize the port. " + - "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0" - ) + clockWarning(Some(sourceInfo)) } makePort(sourceInfo, idx, dir, clock) } @@ -158,10 +165,7 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) implicit compileOptions: CompileOptions ): Unit = { if (warn && clock != clockInst) { - Builder.warning( - "The clock used to initialize the memory is different than the one used to initialize the port. " + - "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0" - ) + clockWarning(None) } implicit val sourceInfo = UnlocatableSourceInfo makePort(UnlocatableSourceInfo, idx, MemPortDirection.WRITE, clock) := data @@ -220,10 +224,7 @@ sealed abstract class MemBase[T <: Data](val t: T, val length: BigInt) ): Unit = { implicit val sourceInfo = UnlocatableSourceInfo if (warn && clock != clockInst) { - Builder.warning( - "The clock used to initialize the memory is different than the one used to initialize the port. " + - "If this is intentional, please pass the clock explicitly when creating the port. This behavior will be an error in 3.6.0" - ) + clockWarning(None) } val accessor = makePort(sourceInfo, idx, MemPortDirection.WRITE, clock).asInstanceOf[Vec[Data]] val dataVec = data.asInstanceOf[Vec[Data]] -- cgit v1.2.3 From 8776e58ff91cd88562b957d7a09322ec16610b81 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Thu, 3 Feb 2022 04:23:48 +0000 Subject: Tweak Bundle._elementsImpl (#2390) (#2392) * Change type of Bundle._elementsImpl to Iterable It was previously SeqMap (ListMap on Scala 2.12). This change gives us more freedom to optimize the implementation without breaking binary compatibility. It is scala.collection.Iterable because it is perfectly fine to return mutable collections (like Arrays) since the only use is to Iterate on them. * Disallow users implementing Bundle._elementsImpl Currently, it would result in a runtime linkage error. This turns it into a compile-time error. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 1b05a14ad6d5784f3b91ab510dc1095423c23ea8) Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/Aggregate.scala | 2 +- .../chisel3/internal/plugin/BundleComponent.scala | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 75966a16..ec64f28b 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1254,7 +1254,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { /* * This method will be overwritten by the Chisel-Plugin */ - protected def _elementsImpl: SeqMap[String, Any] = { + protected def _elementsImpl: Iterable[(String, Any)] = { val nameMap = LinkedHashMap[String, Data]() for (m <- getPublicFields(classOf[Bundle])) { getBundleField(m) match { diff --git a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala index e92bbb23..d768175d 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala @@ -41,12 +41,12 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi def inferType(t: Tree): Type = localTyper.typed(t, nsc.Mode.TYPEmode).tpe - val bundleTpe: Type = inferType(tq"chisel3.Bundle") - val dataTpe: Type = inferType(tq"chisel3.Data") - val ignoreSeqTpe: Type = inferType(tq"chisel3.IgnoreSeqInBundle") - val seqOfDataTpe: Type = inferType(tq"scala.collection.Seq[chisel3.Data]") - val someOfDataTpe: Type = inferType(tq"scala.Option[chisel3.Data]") - val seqMapTpe: Type = inferType(tq"scala.collection.immutable.SeqMap[String,Any]") + val bundleTpe: Type = inferType(tq"chisel3.Bundle") + val dataTpe: Type = inferType(tq"chisel3.Data") + val ignoreSeqTpe: Type = inferType(tq"chisel3.IgnoreSeqInBundle") + val seqOfDataTpe: Type = inferType(tq"scala.collection.Seq[chisel3.Data]") + val someOfDataTpe: Type = inferType(tq"scala.Option[chisel3.Data]") + val itStringAnyTpe: Type = inferType(tq"scala.collection.Iterable[(String,Any)]") // Not cached because it should only be run once per class (thus once per Type) def isBundle(sym: Symbol): Boolean = { sym.tpe <:< bundleTpe } @@ -95,6 +95,9 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi case d: DefDef if isNullaryMethodNamed("_cloneTypeImpl", d) => val msg = "Users cannot override _cloneTypeImpl. Let the compiler plugin generate it." global.globalError(d.pos, msg) + case d: DefDef if isNullaryMethodNamed("_elementsImpl", d) => + val msg = "Users cannot override _elementsImpl. Let the compiler plugin generate it." + global.globalError(d.pos, msg) case d: DefDef if isNullaryMethodNamed("_usingPlugin", d) => val msg = "Users cannot override _usingPlugin, it is for the compiler plugin's use only." global.globalError(d.pos, msg) @@ -220,10 +223,10 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi val elementsImplSym = bundle.symbol.newMethod(TermName("_elementsImpl"), bundle.symbol.pos.focus, Flag.OVERRIDE | Flag.PROTECTED) elementsImplSym.resetFlag(Flags.METHOD) - elementsImplSym.setInfo(NullaryMethodType(seqMapTpe)) + elementsImplSym.setInfo(NullaryMethodType(itStringAnyTpe)) val elementsImpl = localTyper.typed( - DefDef(elementsImplSym, q"scala.collection.immutable.SeqMap.apply[String, Any](..$elementArgs)") + DefDef(elementsImplSym, q"scala.collection.immutable.Vector.apply[(String, Any)](..$elementArgs)") ) Some(elementsImpl) -- cgit v1.2.3 From 56d789c17cec32cb985ebce2651bf7269f95b5d1 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Thu, 3 Feb 2022 17:24:53 +0000 Subject: FillInterleaved documentation: swap order of elements in Seq example (#2393) (#2395) (cherry picked from commit 055a85298c46c6734880cd828af436adbded1d0a) Co-authored-by: John Ingalls <43973001+ingallsj@users.noreply.github.com>--- src/main/scala/chisel3/util/Bitwise.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/scala/chisel3/util/Bitwise.scala b/src/main/scala/chisel3/util/Bitwise.scala index 0d8318bf..8abe3645 100644 --- a/src/main/scala/chisel3/util/Bitwise.scala +++ b/src/main/scala/chisel3/util/Bitwise.scala @@ -14,7 +14,7 @@ import chisel3._ * FillInterleaved(2, "b1 0 0 1".U) // equivalent to "b11 00 00 11".U * FillInterleaved(2, myUIntWire) // dynamic interleaved fill * - * FillInterleaved(2, Seq(true.B, false.B, false.B, false.B)) // equivalent to "b11 00 00 00".U + * FillInterleaved(2, Seq(false.B, false.B, false.B, true.B)) // equivalent to "b11 00 00 00".U * FillInterleaved(2, Seq(true.B, false.B, false.B, true.B)) // equivalent to "b11 00 00 11".U * }}} */ -- cgit v1.2.3 From dd5c643ae86c234041092efee4386cc82e59331d Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Fri, 4 Feb 2022 02:26:06 +0000 Subject: Fix bundle elements performance regression (#2396) (#2398) * Only call _elementsImpl once in Bundle.elements * Distinguish compiler plugin and reflective _elementsImpl Bundle.elements now will only do post-processing if the user is using plugin-generated _elementsImpl. This improves performance for the case where the user does not opt-in to using the plugin to generate _elementsImpl. (cherry picked from commit 5fead89ee0132355e551bcb6b87cc2a6db679bee) Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/Aggregate.scala | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index ec64f28b..06ae36f3 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1207,8 +1207,19 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { * assert(uint === "h12345678".U) // This will pass * }}} */ - final lazy val elements: SeqMap[String, Data] = { - val hardwareFields = _elementsImpl.flatMap { + final lazy val elements: SeqMap[String, Data] = + // _elementsImpl is a method, only call it once + _elementsImpl match { + // Those not using plugin-generated _elementsImpl use the old reflective implementation + case oldElements: VectorMap[_, _] => oldElements.asInstanceOf[VectorMap[String, Data]] + // Plugin-generated _elementsImpl are incomplete and need some processing + case rawElements => _processRawElements(rawElements) + } + + // The compiler plugin is imperfect at picking out elements statically so we process at runtime + // checking for errors and filtering out mistakes + private def _processRawElements(rawElements: Iterable[(String, Any)]): SeqMap[String, Data] = { + val hardwareFields = rawElements.flatMap { case (name, data: Data) => if (data.isSynthesizable) { Some(s"$name: $data") @@ -1241,7 +1252,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { if (hardwareFields.nonEmpty) { throw ExpectedChiselTypeException(s"Bundle: $this contains hardware fields: " + hardwareFields.mkString(",")) } - VectorMap(_elementsImpl.toSeq.flatMap { + VectorMap(rawElements.toSeq.flatMap { case (name, data: Data) => Some(name -> data) case (name, Some(data: Data)) => @@ -1251,15 +1262,17 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { case ((an, a), (bn, b)) => (a._id > b._id) || ((a eq b) && (an > bn)) }: _*) } - /* - * This method will be overwritten by the Chisel-Plugin + + /* The old, reflective implementation of Bundle.elements + * This method is optionally overwritten by the compiler plugin for much better performance */ protected def _elementsImpl: Iterable[(String, Any)] = { val nameMap = LinkedHashMap[String, Data]() for (m <- getPublicFields(classOf[Bundle])) { getBundleField(m) match { case Some(d: Data) => - // Checking for chiselType is done in elements method + requireIsChiselType(d) + if (nameMap contains m.getName) { require(nameMap(m.getName) eq d) } else { -- cgit v1.2.3 From 9d1e2082df4ecb2942a28b7039eb2ff36953380c Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Fri, 4 Feb 2022 04:38:30 +0000 Subject: Fix variable-name typo (#2397) (#2400) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 015755749caa8a05f3809d446b023df80c7419d1) Co-authored-by: Tynan McAuley --- core/src/main/scala/chisel3/internal/Builder.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala index a00505a8..247be57a 100644 --- a/core/src/main/scala/chisel3/internal/Builder.scala +++ b/core/src/main/scala/chisel3/internal/Builder.scala @@ -504,7 +504,7 @@ private[chisel3] object Builder extends LazyLogging { def getPrefix: Prefix = chiselContext.get().prefixStack def currentModule: Option[BaseModule] = dynamicContextVar.value match { - case Some(dyanmicContext) => dynamicContext.currentModule + case Some(dynamicContext) => dynamicContext.currentModule case _ => None } def currentModule_=(target: Option[BaseModule]): Unit = { -- cgit v1.2.3