From bfa9f7465e6069b1e624126f9e14245b69e7c0a9 Mon Sep 17 00:00:00 2001 From: mergify[bot] Date: Tue, 8 Nov 2022 17:27:07 +0000 Subject: Switch to using experimental trait for OpaqueTypes (backport #2783) (#2836) * Switch to using experimental trait for OpaqueTypes (#2783) This makes it more clear that the feature is experimental. Users may still override the opaqueType method for more dynamic control over when instances of a given Record are OpaqueTypes or not, but they are discouraged from doing so. (cherry picked from commit 7525dc71ccc2050d8e4a68b38f3b76920ba693fc) * Fix cloneType in RecordSpec Co-authored-by: Jack Koenig --- core/src/main/scala/chisel3/Aggregate.scala | 20 ++--- .../scala/chisel3/experimental/OpaqueType.scala | 27 +++++++ .../scala/chisel3/internal/firrtl/Converter.scala | 2 +- src/test/scala/chiselTests/Direction.scala | 4 +- src/test/scala/chiselTests/RecordSpec.scala | 85 ++++++++++++++++++---- 5 files changed, 106 insertions(+), 32 deletions(-) create mode 100644 core/src/main/scala/chisel3/experimental/OpaqueType.scala diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 16611277..f22f5e63 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -8,7 +8,7 @@ import chisel3.experimental.dataview.{isView, reifySingleData, InvalidViewExcept import scala.collection.immutable.{SeqMap, VectorMap} import scala.collection.mutable.{HashSet, LinkedHashMap} import scala.language.experimental.macros -import chisel3.experimental.{BaseModule, BundleLiteralException, ChiselEnum, EnumType, VecLiteralException} +import chisel3.experimental.{BaseModule, BundleLiteralException, ChiselEnum, EnumType, OpaqueType, VecLiteralException} import chisel3.internal._ import chisel3.internal.Builder.pushCommand import chisel3.internal.firrtl._ @@ -881,23 +881,15 @@ trait VecLike[T <: Data] extends IndexedSeq[T] with HasId with SourceInfoDoc { */ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptions) extends Aggregate { - /** Indicates if this Record represents an "Opaque Type" - * - * Opaque types provide a mechanism for user-defined types - * that do not impose any "boxing" overhead in the emitted FIRRTL and Verilog. - * You can think about an opaque type Record as a box around - * a single element that only exists at Chisel elaboration time. - * Put another way, if opaqueType is overridden to true, - * The Record may only contain a single element with an empty name - * and there will be no `_` in the name for that element in the emitted Verilog. - * - * @see RecordSpec in Chisel's tests for example usage and expected output - */ - def opaqueType: Boolean = false + private[chisel3] def _isOpaqueType: Boolean = this match { + case maybe: OpaqueType => maybe.opaqueType + case _ => false + } // Doing this earlier than onModuleClose allows field names to be available for prefixing the names // of hardware created when connecting to one of these elements private def setElementRefs(): Unit = { + val opaqueType = this._isOpaqueType // Since elements is a map, it is impossible for two elements to have the same // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog // which can cause collisions diff --git a/core/src/main/scala/chisel3/experimental/OpaqueType.scala b/core/src/main/scala/chisel3/experimental/OpaqueType.scala new file mode 100644 index 00000000..e7a2a15d --- /dev/null +++ b/core/src/main/scala/chisel3/experimental/OpaqueType.scala @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: Apache-2.0 + +package chisel3.experimental + +import chisel3._ + +/** Indicates if this Record represents an "Opaque Type" + * + * Opaque types provide a mechanism for user-defined types + * that do not impose any "boxing" overhead in the emitted FIRRTL and Verilog. + * You can think about an opaque type Record as a box around + * a single element that only exists at Chisel elaboration time. + * Put another way, if this trait is mixed into a Record, + * the Record may only contain a single element with an empty name + * and there will be no `_` in the name for that element in the emitted Verilog. + * + * @see RecordSpec in Chisel's tests for example usage and expected output + */ +trait OpaqueType { self: Record => + + /** If set to true, indicates that this Record is an OpaqueType + * + * Users can override this if they need more dynamic control over the behavior for when + * instances of this type are considered opaque + */ + def opaqueType: Boolean = true +} diff --git a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala index f73e85d2..3d6e0d79 100644 --- a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala +++ b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala @@ -316,7 +316,7 @@ private[chisel3] object Converter { case (false, SpecifiedDirection.Flip | SpecifiedDirection.Input) => fir.Field(getRef(elt, info).name, fir.Flip, extractType(elt, false, info)) } - if (!d.opaqueType) + if (!d._isOpaqueType) fir.BundleType(d.elements.toIndexedSeq.reverse.map { case (_, e) => eltField(e) }) else extractType(d.elements.head._2, childClearDir, info) diff --git a/src/test/scala/chiselTests/Direction.scala b/src/test/scala/chiselTests/Direction.scala index 03755e83..642a507c 100644 --- a/src/test/scala/chiselTests/Direction.scala +++ b/src/test/scala/chiselTests/Direction.scala @@ -4,6 +4,7 @@ package chiselTests import org.scalatest._ import chisel3._ +import chisel3.experimental.OpaqueType import chisel3.stage.ChiselStage import org.scalatest.matchers.should.Matchers @@ -370,10 +371,9 @@ class DirectionSpec extends ChiselPropSpec with Matchers with Utils { val valid = Bool() val ready = Flipped(Bool()) } - class MyOpaqueType extends Record { + class MyOpaqueType extends Record with OpaqueType { val k = new Decoupled() val elements = SeqMap("" -> k) - override def opaqueType = elements.size == 1 override def cloneType: this.type = (new MyOpaqueType).asInstanceOf[this.type] } class MyModule extends RawModule { diff --git a/src/test/scala/chiselTests/RecordSpec.scala b/src/test/scala/chiselTests/RecordSpec.scala index 509edbbc..3414ec8a 100644 --- a/src/test/scala/chiselTests/RecordSpec.scala +++ b/src/test/scala/chiselTests/RecordSpec.scala @@ -6,7 +6,7 @@ import chisel3._ import chisel3.stage.ChiselStage import chisel3.testers.BasicTester import chisel3.util.{Counter, Queue} -import chisel3.experimental.DataMirror +import chisel3.experimental.{DataMirror, OpaqueType} import scala.collection.immutable.SeqMap @@ -109,10 +109,9 @@ trait RecordSpecUtils { require(!DataMirror.checkTypeEquivalence(wire1, wire2)) } - class SingleElementRecord extends Record { + class SingleElementRecord extends Record with OpaqueType { private val underlying = UInt(8.W) val elements = SeqMap("" -> underlying) - override def opaqueType = elements.size == 1 override def cloneType: this.type = (new SingleElementRecord).asInstanceOf[this.type] def +(that: SingleElementRecord): SingleElementRecord = { @@ -132,17 +131,15 @@ trait RecordSpecUtils { out := in1 + in2 } - class InnerRecord extends Record { + class InnerRecord extends Record with OpaqueType { val k = new InnerInnerRecord val elements = SeqMap("" -> k) - override def opaqueType = elements.size == 1 override def cloneType: this.type = (new InnerRecord).asInstanceOf[this.type] } - class InnerInnerRecord extends Record { + class InnerInnerRecord extends Record with OpaqueType { val k = new SingleElementRecord val elements = SeqMap("" -> k) - override def opaqueType = elements.size == 1 override def cloneType: this.type = (new InnerInnerRecord).asInstanceOf[this.type] } @@ -163,11 +160,10 @@ trait RecordSpecUtils { io.bar.elements.head._2 := io.foo.elements.head._2 } - class NamedSingleElementRecord extends Record { + class NamedSingleElementRecord extends Record with OpaqueType { private val underlying = UInt(8.W) val elements = SeqMap("unused" -> underlying) - override def opaqueType = elements.size == 1 override def cloneType: this.type = (new NamedSingleElementRecord).asInstanceOf[this.type] } @@ -177,7 +173,7 @@ trait RecordSpecUtils { out := in } - class ErroneousOverride extends Record { + class ErroneousOverride extends Record with OpaqueType { private val underlyingA = UInt(8.W) private val underlyingB = UInt(8.W) val elements = SeqMap("x" -> underlyingA, "y" -> underlyingB) @@ -191,6 +187,44 @@ trait RecordSpecUtils { val out = IO(Output(new ErroneousOverride)) out := in } + + class NotActuallyOpaqueType extends Record with OpaqueType { + private val underlyingA = UInt(8.W) + private val underlyingB = UInt(8.W) + val elements = SeqMap("x" -> underlyingA, "y" -> underlyingB) + + override def opaqueType = false + override def cloneType: this.type = (new NotActuallyOpaqueType).asInstanceOf[this.type] + } + + class NotActuallyOpaqueTypeModule extends Module { + val in = IO(Input(new NotActuallyOpaqueType)) + val out = IO(Output(new NotActuallyOpaqueType)) + out := in + } + + // Illustrate how to dyanmically decide between OpaqueType or not + sealed trait MaybeBoxed[T <: Data] extends Record { + def underlying: T + def boxed: Boolean + } + object MaybeBoxed { + def apply[T <: Data](gen: T, boxed: Boolean): MaybeBoxed[T] = { + if (boxed) new Boxed(gen) else new Unboxed(gen) + } + } + class Boxed[T <: Data](gen: T) extends MaybeBoxed[T] { + def boxed = true + lazy val elements = SeqMap("underlying" -> gen.cloneType) + def underlying = elements.head._2 + override def cloneType: this.type = (new Boxed(gen)).asInstanceOf[this.type] + } + class Unboxed[T <: Data](gen: T) extends MaybeBoxed[T] with OpaqueType { + def boxed = false + lazy val elements = SeqMap("" -> gen.cloneType) + def underlying = elements.head._2 + override def cloneType: this.type = (new Unboxed(gen)).asInstanceOf[this.type] + } } class RecordSpec extends ChiselFlatSpec with RecordSpecUtils with Utils { @@ -229,14 +263,14 @@ class RecordSpec extends ChiselFlatSpec with RecordSpecUtils with Utils { e.getMessage should include("contains aliased fields named (bar,foo)") } - they should "be OpaqueType for maps with single unnamed elements" in { + they should "support OpaqueType for maps with single unnamed elements" in { val singleElementChirrtl = ChiselStage.emitChirrtl { new SingleElementRecordModule } singleElementChirrtl should include("input in1 : UInt<8>") singleElementChirrtl should include("input in2 : UInt<8>") singleElementChirrtl should include("add(in1, in2)") } - they should "work correctly for toTarget in nested opaque type Records" in { + they should "work correctly for toTarget in nested OpaqueType Records" in { var mod: NestedRecordModule = null ChiselStage.elaborate { mod = new NestedRecordModule; mod } val testStrings = Seq( @@ -250,7 +284,7 @@ class RecordSpec extends ChiselFlatSpec with RecordSpecUtils with Utils { testStrings.foreach(x => assert(x == "~NestedRecordModule|InnerModule>io.foo")) } - they should "work correctly when connecting nested opaque type elements" in { + they should "work correctly when connecting nested OpaqueType elements" in { val nestedRecordChirrtl = ChiselStage.emitChirrtl { new NestedRecordModule } nestedRecordChirrtl should include("input in : UInt<8>") nestedRecordChirrtl should include("output out : UInt<8>") @@ -260,18 +294,39 @@ class RecordSpec extends ChiselFlatSpec with RecordSpecUtils with Utils { nestedRecordChirrtl should include("io.bar <= io.foo") } - they should "throw an error when map contains a named element and opaqueType is overriden to true" in { + they should "throw an error when map contains a named element and OpaqueType is mixed in" in { (the[Exception] thrownBy extractCause[Exception] { ChiselStage.elaborate { new NamedSingleElementModule } }).getMessage should include("Opaque types must have exactly one element with an empty name") } - they should "throw an error when map contains more than one element and opaqueType is overriden to true" in { + they should "throw an error when map contains more than one element and OpaqueType is mixed in" in { (the[Exception] thrownBy extractCause[Exception] { ChiselStage.elaborate { new ErroneousOverrideModule } }).getMessage should include("Opaque types must have exactly one element with an empty name") } + they should "work correctly when an OpaqueType overrides the def as false" in { + val chirrtl = ChiselStage.emitChirrtl(new NotActuallyOpaqueTypeModule) + chirrtl should include("input in : { y : UInt<8>, x : UInt<8>}") + chirrtl should include("output out : { y : UInt<8>, x : UInt<8>}") + chirrtl should include("out <= in") + } + + they should "support conditional OpaqueTypes via traits and factory methods" in { + class MyModule extends Module { + val in0 = IO(Input(MaybeBoxed(UInt(8.W), true))) + val out0 = IO(Output(MaybeBoxed(UInt(8.W), true))) + val in1 = IO(Input(MaybeBoxed(UInt(8.W), false))) + val out1 = IO(Output(MaybeBoxed(UInt(8.W), false))) + out0 := in0 + out1 := in1 + } + val chirrtl = ChiselStage.emitChirrtl(new MyModule) + chirrtl should include("input in0 : { underlying : UInt<8>}") + chirrtl should include("input in1 : UInt<8>") + } + they should "work with .toTarget" in { var m: SingleElementRecordModule = null ChiselStage.elaborate { m = new SingleElementRecordModule; m } -- cgit v1.2.3