diff options
| author | Richard Lin | 2018-01-02 13:51:36 -0800 |
|---|---|---|
| committer | GitHub | 2018-01-02 13:51:36 -0800 |
| commit | 678df42aa6bf2f4715be2d051c3c29f058948d0c (patch) | |
| tree | 0b3fb00565e1c442abd7885d490cd4324f02d3fe | |
| parent | cb7fcd2b18135230dc40f3c7bb98685e7ffde9d5 (diff) | |
| parent | ade792ee7c5bb718f738f5e4c3886b2e87c68756 (diff) | |
Merge pull request #723 from freechipsproject/autoclonetype
Auto clone type 🦆
12 files changed, 401 insertions, 31 deletions
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index b89961aa..14011cd9 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -556,31 +556,173 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { case _ => None } + // If this Data is an instance of an inner class, record a *guess* at the outer object. + // This is only used for cloneType, and is mutable to allow autoCloneType to try guesses other + // than the module that was current when the Bundle was constructed. + private var _outerInst: Option[Object] = Builder.currentModule + override def cloneType : this.type = { - // If the user did not provide a cloneType method, try invoking one of - // the following constructors, not all of which necessarily exist: - // - A zero-parameter constructor - // - A one-paramater constructor, with null as the argument - // - A one-parameter constructor for a nested Bundle, with the enclosing - // parent Module as the argument - val constructor = this.getClass.getConstructors.head - try { - val args = Seq.fill(constructor.getParameterTypes.size)(null) - constructor.newInstance(args:_*).asInstanceOf[this.type] + // This attempts to infer constructor and arguments to clone this Bundle subtype without + // requiring the user explicitly overriding cloneType. + import scala.language.existentials + + def reflectError(desc: String): Nothing = { + Builder.exception(s"Unable to automatically infer cloneType on $this: $desc").asInstanceOf[Nothing] + } + + import scala.reflect.runtime.universe._ + + // Check if this is an inner class, and if so, try to get the outer instance + val clazz = this.getClass + val outerClassInstance = Option(clazz.getEnclosingClass).map { outerClass => + def canAssignOuterClass(x: Object) = outerClass.isAssignableFrom(x.getClass) + val outerInstance = try { + clazz.getDeclaredField("$outer").get(this) // doesn't work in all cases, namely anonymous inner Bundles + } catch { + case _: NoSuchFieldException => + // First check if this Bundle is bound and an anonymous inner Bundle of another Bundle + this.bindingOpt.collect { case ChildBinding(p) if canAssignOuterClass(p) => p } + .orElse(this._outerInst) + .getOrElse(reflectError(s"Unable to determine instance of outer class $outerClass")) + } + if (!canAssignOuterClass(outerInstance)) { + reflectError(s"Unable to determine instance of outer class $outerClass," + + s" guessed $outerInstance, but is not assignable to outer class $outerClass") + } + // Record the outer instance for future cloning + this._outerInst = Some(outerInstance) + (outerClass, outerInstance) + } + + // If possible (constructor with no arguments), try Java reflection first + // This handles two cases that Scala reflection doesn't: + // 1. getting the ClassSymbol of a class with an anonymous outer class fails with a + // CyclicReference exception + // 2. invoking the constructor of an anonymous inner class seems broken (it expects the outer + // class as an argument, but fails because the number of arguments passed in is incorrect) + if (clazz.getConstructors.size == 1) { + var ctor = clazz.getConstructors.head + val argTypes = ctor.getParameterTypes.toList + val clone = (argTypes, outerClassInstance) match { + case (Nil, None) => // no arguments, no outer class, invoke constructor directly + Some(ctor.newInstance().asInstanceOf[this.type]) + case (argType :: Nil, Some((_, outerInstance))) => + if (argType isAssignableFrom outerInstance.getClass) { + Some(ctor.newInstance(outerInstance).asInstanceOf[this.type]) + } else { + None + } + case _ => None + } + clone match { + case Some(clone) => + clone._outerInst = this._outerInst + if (!clone.typeEquivalent(this)) { + reflectError(s"Automatically cloned $clone not type-equivalent to base $this." + + " Constructor argument values were not inferred, ensure constructor is deterministic.") + } + return clone.asInstanceOf[this.type] + case None => + } + } + + // Get constructor parameters and accessible fields + val mirror = runtimeMirror(clazz.getClassLoader) + val classSymbol = try { + mirror.reflect(this).symbol } catch { - case e: java.lang.reflect.InvocationTargetException if e.getCause.isInstanceOf[java.lang.NullPointerException] => - try { - constructor.newInstance(_parent.get).asInstanceOf[this.type] - } catch { - case _: java.lang.reflect.InvocationTargetException | _: java.lang.IllegalArgumentException => - Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method. You are probably using " + - "an anonymous Bundle object that captures external state and hence is un-cloneTypeable") - this - } - case _: java.lang.reflect.InvocationTargetException | _: java.lang.IllegalArgumentException => - Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method") - this + case e: scala.reflect.internal.Symbols#CyclicReference => reflectError(s"Scala cannot reflect on $this, got exception $e." + + " This is known to occur with inner classes on anonymous outer classes." + + " In those cases, autoclonetype only works with no-argument constructors, or you can define a custom cloneType.") + } + + val decls = classSymbol.typeSignature.decls + val ctors = decls.collect { case meth: MethodSymbol if meth.isConstructor => meth } + if (ctors.size != 1) { + reflectError(s"found multiple constructors ($ctors)." + + " Either remove all but the default constructor, or define a custom cloneType method.") + } + val ctor = ctors.head + val ctorParamss = ctor.paramLists + val ctorParams = ctorParamss match { + case Nil => List() + case ctorParams :: Nil => ctorParams + case ctorParams :: ctorImplicits :: Nil => ctorParams ++ ctorImplicits + case _ => reflectError(s"internal error, unexpected ctorParamss = $ctorParamss") } + val ctorParamsNames = ctorParams.map(_.name.toString) + + // Special case for anonymous inner classes: their constructor consists of just the outer class reference + // Scala reflection on anonymous inner class constructors seems broken + if (ctorParams.size == 1 && outerClassInstance.isDefined && + ctorParams.head.typeSignature == mirror.classSymbol(outerClassInstance.get._1).toType) { + // Fall back onto Java reflection + val ctors = clazz.getConstructors + require(ctors.size == 1) // should be consistent with Scala constructors + try { + val clone = ctors.head.newInstance(outerClassInstance.get._2).asInstanceOf[this.type] + clone._outerInst = this._outerInst + return clone + } catch { + case e @ (_: java.lang.reflect.InvocationTargetException | _: IllegalArgumentException) => + reflectError(s"Unexpected failure at constructor invocation, got $e.") + } + } + + // Get all the class symbols up to (but not including) Bundle and get all the accessors. + // (each ClassSymbol's decls only includes those declared in the class itself) + val bundleClassSymbol = mirror.classSymbol(classOf[Bundle]) + val superClassSymbols = classSymbol.baseClasses.takeWhile(_ != bundleClassSymbol) + val superClassDecls = superClassSymbols.map(_.typeSignature.decls).flatten + val accessors = superClassDecls.collect { case meth: MethodSymbol if meth.isParamAccessor => meth } + + // Get constructor argument values + // Check that all ctor params are immutable and accessible. Immutability is required to avoid + // potential subtle bugs (like values changing after cloning). + // This also generates better error messages (all missing elements shown at once) instead of + // failing at the use site one at a time. + val accessorsName = accessors.filter(_.isStable).map(_.name.toString) + val paramsDiff = ctorParamsNames.toSet -- accessorsName.toSet + if (!paramsDiff.isEmpty) { + reflectError(s"constructor has parameters $paramsDiff that are not both immutable and accessible." + + " Either make all parameters immutable and accessible (vals) so cloneType can be inferred, or define a custom cloneType method.") + } + + // Get all the argument values + val accessorsMap = accessors.map(accessor => accessor.name.toString -> accessor).toMap + val instanceReflect = mirror.reflect(this) + val ctorParamsNameVals = ctorParamsNames.map { + paramName => paramName -> instanceReflect.reflectMethod(accessorsMap(paramName)).apply() + } + + // Opportunistic sanity check: ensure any arguments of type Data is not bound + // (which could lead to data conflicts, since it's likely the user didn't know to re-bind them). + // This is not guaranteed to catch all cases (for example, Data in Tuples or Iterables). + val boundDataParamNames = ctorParamsNameVals.collect { + case (paramName, paramVal: Data) if paramVal.hasBinding => paramName + } + if (boundDataParamNames.nonEmpty) { + reflectError(s"constructor parameters ($boundDataParamNames) have values that are hardware types, which is likely to cause subtle errors." + + " Use chisel types instead: use the value before it is turned to a hardware type (with Wire(...), Reg(...), etc) or use chiselTypeOf(...) to extract the chisel type.") + } + + val ctorParamsVals = ctorParamsNameVals.map{ case (_, paramVal) => paramVal } + + // Invoke ctor + val classMirror = outerClassInstance match { + case Some((_, outerInstance)) => mirror.reflect(outerInstance).reflectClass(classSymbol) + case None => mirror.reflectClass(classSymbol) + } + val clone = classMirror.reflectConstructor(ctor).apply(ctorParamsVals:_*).asInstanceOf[this.type] + clone._outerInst = this._outerInst + + if (!clone.typeEquivalent(this)) { + reflectError(s"Automatically cloned $clone not type-equivalent to base $this." + + " Constructor argument values were inferred: ensure that variable names are consistent and have the same value throughout the constructor chain," + + " and that the constructor is deterministic.") + } + + clone } /** Default "pretty-print" implementation diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala index 19adf01b..1cf50e9f 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala @@ -240,6 +240,7 @@ abstract class Data extends HasId { // This information is supplemental (more than is necessary to generate FIRRTL) and is used to // perform checks in Chisel, where more informative error messages are possible. private var _binding: Option[Binding] = None + private[core] def bindingOpt = _binding private[core] def hasBinding = _binding.isDefined // Only valid after node is bound (synthesizable), crashes otherwise private[core] def binding = _binding.get diff --git a/src/main/scala/chisel3/testers/BasicTester.scala b/src/main/scala/chisel3/testers/BasicTester.scala index 6d1a4913..7bb441ba 100644 --- a/src/main/scala/chisel3/testers/BasicTester.scala +++ b/src/main/scala/chisel3/testers/BasicTester.scala @@ -13,7 +13,7 @@ import internal.sourceinfo.SourceInfo class BasicTester extends Module() { // The testbench has no IOs, rather it should communicate using printf, assert, and stop. - val io = IO(new Bundle()) + val io = IO(new Bundle() {}) def popCount(n: Long): Int = n.toBinaryString.count(_=='1') diff --git a/src/test/scala/chiselTests/AnalogIntegrationSpec.scala b/src/test/scala/chiselTests/AnalogIntegrationSpec.scala index 952d3872..a3e6e643 100644 --- a/src/test/scala/chiselTests/AnalogIntegrationSpec.scala +++ b/src/test/scala/chiselTests/AnalogIntegrationSpec.scala @@ -19,7 +19,7 @@ class AnalogBlackBoxPort extends Bundle { // This IO can be used for a single BlackBox or to group multiple // Has multiple ports for driving and checking but only one shared bus -class AnalogBlackBoxIO(n: Int) extends Bundle { +class AnalogBlackBoxIO(val n: Int) extends Bundle { require(n > 0) val bus = Analog(32.W) val port = Vec(n, new AnalogBlackBoxPort) diff --git a/src/test/scala/chiselTests/AutoClonetypeSpec.scala b/src/test/scala/chiselTests/AutoClonetypeSpec.scala new file mode 100644 index 00000000..1170ed12 --- /dev/null +++ b/src/test/scala/chiselTests/AutoClonetypeSpec.scala @@ -0,0 +1,115 @@ +// See LICENSE for license details. + +package chiselTests + +import chisel3._ + +import chisel3.testers.BasicTester + +class BundleWithIntArg(val i: Int) extends Bundle { + val out = UInt(i.W) +} + +class BundleWithImplicit()(implicit val ii: Int) extends Bundle { + val out = UInt(ii.W) +} + +class BundleWithArgAndImplicit(val i: Int)(implicit val ii: Int) extends Bundle { + val out1 = UInt(i.W) + val out2 = UInt(ii.W) +} + +class BaseBundleVal(val i: Int) extends Bundle { + val inner = UInt(i.W) +} +class SubBundle(i: Int, val i2: Int) extends BaseBundleVal(i) { + val inner2 = UInt(i2.W) +} +class SubBundleInvalid(i: Int, val i2: Int) extends BaseBundleVal(i+1) { + val inner2 = UInt(i2.W) +} + +class BaseBundleNonVal(i: Int) extends Bundle { + val inner = UInt(i.W) +} +class SubBundleVal(val i: Int, val i2: Int) extends BaseBundleNonVal(i) { + val inner2 = UInt(i2.W) +} + +class ModuleWithInner extends Module { + class InnerBundle(val i: Int) extends Bundle { + val out = UInt(i.W) + } + + val io = IO(new Bundle{}) + + val myWire = Wire(new InnerBundle(14)) + require(myWire.i == 14) +} + + +class AutoClonetypeSpec extends ChiselFlatSpec { + "Bundles with Scala args" should "not need clonetype" in { + elaborate { new Module { + val io = IO(new Bundle{}) + + val myWire = Wire(new BundleWithIntArg(8)) + assert(myWire.i == 8) + } } + } + + "Bundles with Scala implicit args" should "not need clonetype" in { + elaborate { new Module { + val io = IO(new Bundle{}) + + implicit val implicitInt: Int = 4 + val myWire = Wire(new BundleWithImplicit()) + + assert(myWire.ii == 4) + } } + } + + "Bundles with Scala explicit and impicit args" should "not need clonetype" in { + elaborate { new Module { + val io = IO(new Bundle{}) + + implicit val implicitInt: Int = 4 + val myWire = Wire(new BundleWithArgAndImplicit(8)) + + assert(myWire.i == 8) + assert(myWire.ii == 4) + } } + } + + "Subtyped Bundles" should "not need clonetype" in { + elaborate { new Module { + val io = IO(new Bundle{}) + + val myWire = Wire(new SubBundle(8, 4)) + + assert(myWire.i == 8) + assert(myWire.i2 == 4) + } } + elaborate { new Module { + val io = IO(new Bundle{}) + + val myWire = Wire(new SubBundleVal(8, 4)) + + assert(myWire.i == 8) + assert(myWire.i2 == 4) + } } + } + + "Subtyped Bundles that don't clone well" should "be caught" in { + a [ChiselException] should be thrownBy { + elaborate { new Module { + val io = IO(new Bundle{}) + val myWire = Wire(new SubBundleInvalid(8, 4)) + } } + } + } + + "Inner bundles with Scala args" should "not need clonetype" in { + elaborate { new ModuleWithInner } + } +} diff --git a/src/test/scala/chiselTests/AutoNestedCloneSpec.scala b/src/test/scala/chiselTests/AutoNestedCloneSpec.scala new file mode 100644 index 00000000..746780be --- /dev/null +++ b/src/test/scala/chiselTests/AutoNestedCloneSpec.scala @@ -0,0 +1,108 @@ +// See LICENSE for license details. + +package chiselTests +import Chisel.ChiselException +import org.scalatest._ +import chisel3._ + +class BundleWithAnonymousInner(val w: Int) extends Bundle { + val inner = new Bundle { + val foo = Input(UInt(w.W)) + } +} + +class AutoNestedCloneSpec extends ChiselFlatSpec with Matchers { + behavior of "autoCloneType of inner Bundle in Chisel3" + + it should "clone a doubly-nested inner bundle successfully" in { + elaborate { + class Outer(val w: Int) extends Module { + class Middle(val w: Int) { + class InnerIOType extends Bundle { + val in = Input(UInt(w.W)) + } + def getIO = new InnerIOType + } + val io = IO(new Bundle {}) + val myWire = Wire((new Middle(w)).getIO) + } + new Outer(2) + } + } + + it should "clone an anonymous inner bundle successfully" in { + elaborate { + class TestTop(val w: Int) extends Module { + val io = IO(new Bundle {}) + val myWire = Wire(new Bundle{ val a = UInt(w.W) }) + } + new TestTop(2) + } + } + + it should "pick the correct $outer instance for an anonymous inner bundle" in { + elaborate { + class Inner(val w: Int) extends Module { + val io = IO(new Bundle{ + val in = Input(UInt(w.W)) + val out = Output(UInt(w.W)) + }) + } + class Outer(val w: Int) extends Module { + val io = IO(new Bundle{ + val in = Input(UInt(w.W)) + val out = Output(UInt(w.W)) + }) + val i = Module(new Inner(w)) + val iw = Wire(chiselTypeOf(i.io)) + iw <> io + i.io <> iw + } + new Outer(2) + } + } + + it should "clone an anonymous, bound, inner bundle of another bundle successfully" in { + elaborate { + class TestModule(w: Int) extends Module { + val io = IO(new BundleWithAnonymousInner(w) ) + val w0 = WireInit(io) + val w1 = WireInit(io.inner) + } + new TestModule(8) + } + } + + it should "clone an anonymous, inner bundle of a Module, bound to another bundle successfully" in { + elaborate { + class TestModule(w: Int) extends Module { + val bun = new Bundle { + val foo = UInt(w.W) + } + val io = IO(new Bundle { + val inner = Input(bun) + }) + val w0 = WireInit(io) + val w1 = WireInit(io.inner) + } + new TestModule(8) + } + } + + behavior of "anonymous doubly-nested inner bundle fails with clear error" + ( the[ChiselException] thrownBy { + elaborate { + class Outer(val w: Int) extends Module { + class Middle(val w: Int) { + def getIO = new Bundle { + val in = Input(UInt(w.W)) + } + } + val io = IO(new Bundle {}) + val myWire = Wire((new Middle(w)).getIO) + } + new Outer(2) + } + }).getMessage should include("Unable to determine instance") + +} diff --git a/src/test/scala/chiselTests/CompatibilityInteroperabilitySpec.scala b/src/test/scala/chiselTests/CompatibilityInteroperabilitySpec.scala index 457f26de..95dc87c1 100644 --- a/src/test/scala/chiselTests/CompatibilityInteroperabilitySpec.scala +++ b/src/test/scala/chiselTests/CompatibilityInteroperabilitySpec.scala @@ -12,6 +12,8 @@ object CompatibilityComponents { class ChiselBundle extends Bundle { val a = UInt(width = 32) val b = UInt(width = 32).flip + + override def cloneType = (new ChiselBundle).asInstanceOf[this.type] } class ChiselRecord extends Record { val elements = ListMap("a" -> UInt(width = 32), "b" -> UInt(width = 32).flip) @@ -46,6 +48,8 @@ object Chisel3Components { class Chisel3Bundle extends Bundle { val a = Output(UInt(32.W)) val b = Input(UInt(32.W)) + + override def cloneType = (new Chisel3Bundle).asInstanceOf[this.type] } class Chisel3Record extends Record { diff --git a/src/test/scala/chiselTests/MissingCloneBindingExceptionSpec.scala b/src/test/scala/chiselTests/MissingCloneBindingExceptionSpec.scala index 43f2b0fd..fc9346a0 100644 --- a/src/test/scala/chiselTests/MissingCloneBindingExceptionSpec.scala +++ b/src/test/scala/chiselTests/MissingCloneBindingExceptionSpec.scala @@ -28,7 +28,7 @@ class MissingCloneBindingExceptionSpec extends ChiselFlatSpec with Matchers { } elaborate(new TestTop) - }).getMessage should include("needs cloneType method") + }).getMessage should include("make all parameters immutable") behavior of "missing cloneType in Chisel2" ( the[ChiselException] thrownBy { @@ -53,5 +53,5 @@ class MissingCloneBindingExceptionSpec extends ChiselFlatSpec with Matchers { } elaborate(new TestTop) - }).getMessage should include("needs cloneType method") + }).getMessage should include("make all parameters immutable") } diff --git a/src/test/scala/chiselTests/NamingAnnotationTest.scala b/src/test/scala/chiselTests/NamingAnnotationTest.scala index 699dc0fb..a7b9b75c 100644 --- a/src/test/scala/chiselTests/NamingAnnotationTest.scala +++ b/src/test/scala/chiselTests/NamingAnnotationTest.scala @@ -12,7 +12,7 @@ import chisel3.testers.BasicTester import scala.collection.mutable.ListBuffer trait NamedModuleTester extends Module { - val io = IO(new Bundle()) // Named module testers don't need IO + val io = IO(new Bundle() {}) // Named module testers don't need IO val expectedNameMap = ListBuffer[(InstanceId, String)]() val expectedModuleNameMap = ListBuffer[(Module, String)]() diff --git a/src/test/scala/chiselTests/OptionBundle.scala b/src/test/scala/chiselTests/OptionBundle.scala index 2ac661ea..03b08385 100644 --- a/src/test/scala/chiselTests/OptionBundle.scala +++ b/src/test/scala/chiselTests/OptionBundle.scala @@ -6,7 +6,7 @@ import org.scalatest._ import chisel3._ import chisel3.testers.BasicTester -class OptionBundle(hasIn: Boolean) extends Bundle { +class OptionBundle(val hasIn: Boolean) extends Bundle { val in = if (hasIn) { Some(Input(Bool())) } else { @@ -15,7 +15,7 @@ class OptionBundle(hasIn: Boolean) extends Bundle { val out = Output(Bool()) } -class OptionBundleModule(hasIn: Boolean) extends Module { +class OptionBundleModule(val hasIn: Boolean) extends Module { val io = IO(new OptionBundle(hasIn)) if (hasIn) { io.out := io.in.get diff --git a/src/test/scala/chiselTests/VectorPacketIO.scala b/src/test/scala/chiselTests/VectorPacketIO.scala index 7745db57..4a8c6f78 100644 --- a/src/test/scala/chiselTests/VectorPacketIO.scala +++ b/src/test/scala/chiselTests/VectorPacketIO.scala @@ -27,7 +27,7 @@ class Packet extends Bundle { * lines also. * The problem does not occur if the Vec is taken out */ -class VectorPacketIO(n: Int) extends Bundle { +class VectorPacketIO(val n: Int) extends Bundle { val ins = Vec(n, chisel3.util.DeqIO(new Packet())) val outs = Vec(n, chisel3.util.EnqIO(new Packet())) } diff --git a/src/test/scala/examples/VendingMachineGenerator.scala b/src/test/scala/examples/VendingMachineGenerator.scala index 55a17d9f..0a9dc3e6 100644 --- a/src/test/scala/examples/VendingMachineGenerator.scala +++ b/src/test/scala/examples/VendingMachineGenerator.scala @@ -9,7 +9,7 @@ import chisel3.util._ import VendingMachineUtils._ -class VendingMachineIO(legalCoins: Seq[Coin]) extends Bundle { +class VendingMachineIO(val legalCoins: Seq[Coin]) extends Bundle { require(legalCoins.size >= 1, "The vending machine must accept at least 1 coin!") // Order of coins by value val coins: Seq[Coin] = legalCoins sortBy (_.value) |
