From 7c3c18de2ffd56af51b99030c7ae7d3a321aed5f Mon Sep 17 00:00:00 2001 From: ducky Date: Fri, 3 Nov 2017 18:34:46 -0700 Subject: Autoclonetype initial prototype --- .../src/main/scala/chisel3/core/Aggregate.scala | 123 +++++++++++++++++---- .../src/main/scala/chisel3/core/Binding.scala | 2 +- .../src/main/scala/chisel3/core/BlackBox.scala | 8 +- .../src/main/scala/chisel3/core/Module.scala | 62 ++++++----- .../src/main/scala/chisel3/core/UserModule.scala | 6 +- 5 files changed, 143 insertions(+), 58 deletions(-) (limited to 'chiselFrontend') diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index b89961aa..d4d67018 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -557,30 +557,107 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { } 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] - } 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 + // Automatic Bundle cloning is inferred as follows: + // - User-overloaded cloneType is called instead of this (if supplied, so this method is never called) + // - Attempt to automatically fill in constructor parameters by using Scala reflection: + // - If multiple constructors, error out + // - If the constructor argument list can be matched to param accessors, call the constructor with + // the result of those accessors + // - Otherwise, error out + // - If Scala reflection crashes, fall back to Java reflection and inspect the constructor argument list: + // - If no parameters, invoke constructor directly. That was easy! + // - If the first parameter's type is equal to this Module, assume the Bundle is an inner class + // and invoke it with the current containing Module + // - Error out to the user + + def reflectError(desc: String) { + Builder.exception(s"Unable to automatically infer cloneType on $this: $desc") + } + + // Try Scala reflection + import scala.reflect.runtime.universe._ + + val mirror = runtimeMirror(this.getClass.getClassLoader) + val decls = mirror.classSymbol(this.getClass).typeSignature.decls + println(s"$this decls: $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 accessors = decls.collect { case meth: MethodSymbol if meth.isParamAccessor => meth } + val ctorParamss = ctors.head.paramLists + val ctorParams = ctorParamss match { + case Nil => List() + case ctorParams :: Nil => ctorParams + case _ => reflectError(s"internal error, unexpected ctorParamss = $ctorParamss") + List() // make the type system happy + } + + // 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 ctorParamsName = ctorParams.map(_.name.toString) + val accessorsName = accessors.filter(_.isStable).map(_.name.toString) + val paramsDiff = ctorParamsName.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 ctorParamsVals = ctorParamsName.map { + paramName => instanceReflect.reflectMethod(accessorsMap(paramName)).apply() + } + + // Invoke ctor + val classReflect = mirror.reflectClass(mirror.classSymbol(this.getClass)) + classReflect.reflectConstructor(ctors.head).apply(ctorParamsVals:_*).asInstanceOf[this.type] + + + +// val constructor = this.getClass.getConstructors.head +// val args = Seq.fill(constructor.getParameterTypes.size)(null) +// constructor.newInstance(args:_*).asInstanceOf[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 classMirror = runtimeMirror.reflectClass(classSymbol) +//// val constructors = classSymbol.typeSignature.members.filter(_.isConstructor).toList +//// println(constructors) +//// +// val constructor = this.getClass.getConstructors.head +// println(constructor) +// println(constructor.getParameterTypes) +// +// try { +// val args = Seq.fill(constructor.getParameterTypes.size)(null) +// constructor.newInstance(args:_*).asInstanceOf[this.type] +// } 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 +// } } /** Default "pretty-print" implementation diff --git a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala index 3fdc383c..a6cc8bac 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala @@ -23,7 +23,7 @@ object Binding { object requireIsHardware { def apply(node: Data, msg: String = "") = { node._parent match { // Compatibility layer hack - case Some(x: BaseModule) => x._autoWrapPorts + case Some(x: BaseModule) => x._compatAutoWrapPorts case _ => } if (!node.hasBinding) { diff --git a/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala b/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala index aa0f8064..3c99886e 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala @@ -126,12 +126,12 @@ abstract class ExtModule(val params: Map[String, Param] = Map.empty[String, Para */ abstract class BlackBox(val params: Map[String, Param] = Map.empty[String, Param])(implicit compileOptions: CompileOptions) extends BaseBlackBox { def io: Record - + // Allow access to bindings from the compatibility package - protected def _ioPortBound() = portsContains(io) - + protected def _compatIoPortBound() = portsContains(io) + private[core] override def generateComponent(): Component = { - _autoWrapPorts() // pre-IO(...) compatibility hack + _compatAutoWrapPorts() // pre-IO(...) compatibility hack // Restrict IO to just io, clock, and reset require(io != null, "BlackBox must have io") diff --git a/chiselFrontend/src/main/scala/chisel3/core/Module.scala b/chiselFrontend/src/main/scala/chisel3/core/Module.scala index fa9ab082..0c69010b 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Module.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Module.scala @@ -191,7 +191,7 @@ abstract class BaseModule extends HasId { * * TODO: remove this, perhaps by removing Bindings checks in compatibility mode. */ - def _autoWrapPorts() {} + def _compatAutoWrapPorts() {} // // BaseModule User API functions @@ -200,29 +200,10 @@ abstract class BaseModule extends HasId { Builder.annotations += annotation } - /** - * This must wrap the datatype used to set the io field of any Module. - * i.e. All concrete modules must have defined io in this form: - * [lazy] val io[: io type] = IO(...[: io type]) - * - * Items in [] are optional. - * - * The granted iodef WILL NOT be cloned (to allow for more seamless use of - * anonymous Bundles in the IO) and thus CANNOT have been bound to any logic. - * This will error if any node is bound (e.g. due to logic in a Bundle - * constructor, which is considered improper). - * - * Also registers a Data as a port, also performing bindings. Cannot be called once ports are - * requested (so that all calls to ports will return the same information). - * Internal API. - * - * TODO(twigg): Specifically walk the Data definition to call out which nodes - * are problematic. - */ - protected def IO[T<:Data](iodef: T): iodef.type = { - require(!_closed, "Can't add more ports after module close") - requireIsChiselType(iodef, "io type") - + /** Chisel2 code didn't require the IO(...) wrapper and would assign a Chisel type directly to + * io, then do operations on it. This binds a Chisel type in-place (mutably) as an IO. + */ + protected def _bindIoInPlace(iodef: Data) { // Compatibility code: Chisel2 did not require explicit direction on nodes // (unspecified treated as output, and flip on nothing was input). // This sets assigns the explicit directions required by newer semantics on @@ -248,11 +229,38 @@ abstract class BaseModule extends HasId { } } assignCompatDir(iodef, false) - - // Bind each element of the iodef to being a Port + iodef.bind(PortBinding(this)) _ports += iodef - iodef + } + + /** + * This must wrap the datatype used to set the io field of any Module. + * i.e. All concrete modules must have defined io in this form: + * [lazy] val io[: io type] = IO(...[: io type]) + * + * Items in [] are optional. + * + * The granted iodef WILL NOT be cloned (to allow for more seamless use of + * anonymous Bundles in the IO) and thus CANNOT have been bound to any logic. + * This will error if any node is bound (e.g. due to logic in a Bundle + * constructor, which is considered improper). + * + * Also registers a Data as a port, also performing bindings. Cannot be called once ports are + * requested (so that all calls to ports will return the same information). + * Internal API. + * + * TODO(twigg): Specifically walk the Data definition to call out which nodes + * are problematic. + */ + protected def IO[T<:Data](iodef: T): iodef.type = { + require(!_closed, "Can't add more ports after module close") + requireIsChiselType(iodef, "io type") + + // Clone the IO so we preserve immutability of data types + val iodefClone = iodef.cloneTypeFull + _bindIoInPlace(iodefClone) + iodefClone.asInstanceOf[iodef.type] } // diff --git a/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala b/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala index 9c923037..ac87eda5 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala @@ -151,8 +151,8 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions) def io: Record // Allow access to bindings from the compatibility package - protected def _ioPortBound() = portsContains(io) - + protected def _compatIoPortBound() = portsContains(io) + protected override def nameIds(rootClass: Class[_]): HashMap[HasId, String] = { val names = super.nameIds(rootClass) @@ -165,7 +165,7 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions) } private[core] override def generateComponent(): Component = { - _autoWrapPorts() // pre-IO(...) compatibility hack + _compatAutoWrapPorts() // pre-IO(...) compatibility hack // Restrict IO to just io, clock, and reset require(io != null, "Module must have io") -- cgit v1.2.3 From 11c1112661e04094bccfd805e737e0318eb91ebc Mon Sep 17 00:00:00 2001 From: Albert Magyar Date: Wed, 22 Nov 2017 17:51:51 -0800 Subject: Add auto clone implementation for inner Bundles (#722) --- .../src/main/scala/chisel3/core/Aggregate.scala | 25 ++++++++++++++++++++++ .../src/main/scala/chisel3/core/Data.scala | 7 ++++++ 2 files changed, 32 insertions(+) (limited to 'chiselFrontend') diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index d4d67018..da14cefd 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -5,6 +5,7 @@ package chisel3.core import scala.collection.immutable.ListMap import scala.collection.mutable.{ArrayBuffer, HashSet, LinkedHashMap} import scala.language.experimental.macros +import scala.util.Try import chisel3.internal._ import chisel3.internal.Builder.pushCommand @@ -574,6 +575,30 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { Builder.exception(s"Unable to automatically infer cloneType on $this: $desc") } + // Check if the bundle is an instance of an inner class by examining + // whether it has one one-argument constructor taking a type matching the enclosing class + if (this.getClass.getConstructors.size == 1) { + val aggClass = this.getClass + val constr = aggClass.getConstructors.head + val argTypes = constr.getParameterTypes + val outerClass = aggClass.getEnclosingClass + if (argTypes.size == 1 && outerClass != null) { + // attempt to clone using "$outer" + var clone: Option[this.type] = + Try[this.type](constr.newInstance(aggClass.getDeclaredField("$outer").get(this)).asInstanceOf[this.type]).toOption + if (clone.isEmpty) { + // fall back to outerModule field + clone = Try[this.type](constr.newInstance(outerModule.get).asInstanceOf[this.type]).toOption + } + clone.foreach(_.outerModule = this.outerModule) + if (clone.isDefined) { + return clone.get + } else { + reflectError("non-trivial inner Bundle class") + } + } + } + // Try Scala reflection import scala.reflect.runtime.universe._ diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala index 19adf01b..d84a86e9 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala @@ -208,6 +208,13 @@ abstract class Data extends HasId { } } + // If this Data is an instance of an inner class, record enclosing class + // This is only used for cloneType! + private[core] var outerModule: Option[BaseModule] = + (Option(this.getClass.getEnclosingClass) zip Builder.currentModule) + .find({ case (c, m) => c.isAssignableFrom(m.getClass) }) + .map({ case (_, m) => m }) + // User-specified direction, local at this node only. // Note that the actual direction of this node can differ from child and parent specifiedDirection. private var _specifiedDirection: SpecifiedDirection = SpecifiedDirection.Unspecified -- cgit v1.2.3 From 48e30fab101c5552c73fc5a76cad3ccc6b38946f Mon Sep 17 00:00:00 2001 From: ducky64 Date: Wed, 22 Nov 2017 22:26:09 -0800 Subject: Support for inner classes, implicit parameter lists, supertypess --- .../src/main/scala/chisel3/core/Aggregate.scala | 224 ++++++++++++--------- .../src/main/scala/chisel3/core/Binding.scala | 2 +- .../src/main/scala/chisel3/core/BlackBox.scala | 8 +- .../src/main/scala/chisel3/core/Module.scala | 62 +++--- .../src/main/scala/chisel3/core/UserModule.scala | 6 +- 5 files changed, 165 insertions(+), 137 deletions(-) (limited to 'chiselFrontend') diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index da14cefd..ed3e1fee 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -5,7 +5,6 @@ package chisel3.core import scala.collection.immutable.ListMap import scala.collection.mutable.{ArrayBuffer, HashSet, LinkedHashMap} import scala.language.experimental.macros -import scala.util.Try import chisel3.internal._ import chisel3.internal.Builder.pushCommand @@ -558,131 +557,168 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { } override def cloneType : this.type = { - // Automatic Bundle cloning is inferred as follows: - // - User-overloaded cloneType is called instead of this (if supplied, so this method is never called) - // - Attempt to automatically fill in constructor parameters by using Scala reflection: - // - If multiple constructors, error out - // - If the constructor argument list can be matched to param accessors, call the constructor with - // the result of those accessors - // - Otherwise, error out - // - If Scala reflection crashes, fall back to Java reflection and inspect the constructor argument list: - // - If no parameters, invoke constructor directly. That was easy! - // - If the first parameter's type is equal to this Module, assume the Bundle is an inner class - // and invoke it with the current containing Module - // - Error out to the user - - def reflectError(desc: String) { - Builder.exception(s"Unable to automatically infer cloneType on $this: $desc") + // This attempts to infer constructor and arguments to clone this Bundle subtype without + // requiring the user explicitly overriding cloneType. + + def reflectError(desc: String): Nothing = { + Builder.exception(s"Unable to automatically infer cloneType on $this: $desc").asInstanceOf[Nothing] } - // Check if the bundle is an instance of an inner class by examining - // whether it has one one-argument constructor taking a type matching the enclosing class - if (this.getClass.getConstructors.size == 1) { - val aggClass = this.getClass - val constr = aggClass.getConstructors.head - val argTypes = constr.getParameterTypes - val outerClass = aggClass.getEnclosingClass - if (argTypes.size == 1 && outerClass != null) { - // attempt to clone using "$outer" - var clone: Option[this.type] = - Try[this.type](constr.newInstance(aggClass.getDeclaredField("$outer").get(this)).asInstanceOf[this.type]).toOption - if (clone.isEmpty) { - // fall back to outerModule field - clone = Try[this.type](constr.newInstance(outerModule.get).asInstanceOf[this.type]).toOption + 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) match { + case Some(outerClass) => + val outerInstance = try { + clazz.getDeclaredField("$outer").get(this) // doesn't work in all cases, namely anonymous inner Bundles + } catch { + case _: NoSuchFieldException => this.outerModule match { + case Some(outerModule) => outerModule + case None => reflectError(s"Unable to determine instance of outer class $outerClass") + } } - clone.foreach(_.outerModule = this.outerModule) - if (clone.isDefined) { - return clone.get - } else { - reflectError("non-trivial inner Bundle class") + if (!outerClass.isAssignableFrom(outerInstance.getClass)) { + reflectError(s"Automatically determined outer class instance $outerInstance not assignable to outer class $outerClass") } - } + Some(outerClass, outerInstance) + case None => None } - // Try Scala reflection - import scala.reflect.runtime.universe._ + // 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.outerModule = this.outerModule + 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 => + } + } - val mirror = runtimeMirror(this.getClass.getClassLoader) - val decls = mirror.classSymbol(this.getClass).typeSignature.decls - println(s"$this decls: $decls") + // Get constructor parameters and accessible fields + val mirror = runtimeMirror(clazz.getClassLoader) + val classSymbol = try { + mirror.reflect(this).symbol + } catch { + 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.") + reflectError(s"found multiple constructors ($ctors)." + + " Either remove all but the default constructor, or define a custom cloneType method.") } - - val accessors = decls.collect { case meth: MethodSymbol if meth.isParamAccessor => meth } - val ctorParamss = ctors.head.paramLists + 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") - List() // make the type system happy } + 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.outerModule = this.outerModule + 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 ctorParamsName = ctorParams.map(_.name.toString) val accessorsName = accessors.filter(_.isStable).map(_.name.toString) - val paramsDiff = ctorParamsName.toSet -- accessorsName.toSet + val paramsDiff = ctorParamsNames.toSet -- accessorsName.toSet if (!paramsDiff.isEmpty) { - reflectError(s"constructor has parameters ($paramsDiff) that are not both immutable and accessible." + + 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 ctorParamsVals = ctorParamsName.map { - paramName => instanceReflect.reflectMethod(accessorsMap(paramName)).apply() + val ctorParamsNameVals = ctorParamsNames.map { + paramName => paramName -> instanceReflect.reflectMethod(accessorsMap(paramName)).apply() + } + + // Oppurtunistic 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.map{ case (paramName, paramVal) => paramVal match { + case paramVal: Data => paramVal.hasBinding match { + case true => Some(paramName) + case false => None + } + case _ => None + } + }.filter(_.isDefined).map(_.get) + if (!boundDataParamNames.isEmpty) { + 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 classReflect = mirror.reflectClass(mirror.classSymbol(this.getClass)) - classReflect.reflectConstructor(ctors.head).apply(ctorParamsVals:_*).asInstanceOf[this.type] - - - -// val constructor = this.getClass.getConstructors.head -// val args = Seq.fill(constructor.getParameterTypes.size)(null) -// constructor.newInstance(args:_*).asInstanceOf[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 classMirror = runtimeMirror.reflectClass(classSymbol) -//// val constructors = classSymbol.typeSignature.members.filter(_.isConstructor).toList -//// println(constructors) -//// -// val constructor = this.getClass.getConstructors.head -// println(constructor) -// println(constructor.getParameterTypes) -// -// try { -// val args = Seq.fill(constructor.getParameterTypes.size)(null) -// constructor.newInstance(args:_*).asInstanceOf[this.type] -// } 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 -// } + 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.outerModule = this.outerModule + + 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/Binding.scala b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala index a6cc8bac..3fdc383c 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala @@ -23,7 +23,7 @@ object Binding { object requireIsHardware { def apply(node: Data, msg: String = "") = { node._parent match { // Compatibility layer hack - case Some(x: BaseModule) => x._compatAutoWrapPorts + case Some(x: BaseModule) => x._autoWrapPorts case _ => } if (!node.hasBinding) { diff --git a/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala b/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala index 3c99886e..aa0f8064 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala @@ -126,12 +126,12 @@ abstract class ExtModule(val params: Map[String, Param] = Map.empty[String, Para */ abstract class BlackBox(val params: Map[String, Param] = Map.empty[String, Param])(implicit compileOptions: CompileOptions) extends BaseBlackBox { def io: Record - + // Allow access to bindings from the compatibility package - protected def _compatIoPortBound() = portsContains(io) - + protected def _ioPortBound() = portsContains(io) + private[core] override def generateComponent(): Component = { - _compatAutoWrapPorts() // pre-IO(...) compatibility hack + _autoWrapPorts() // pre-IO(...) compatibility hack // Restrict IO to just io, clock, and reset require(io != null, "BlackBox must have io") diff --git a/chiselFrontend/src/main/scala/chisel3/core/Module.scala b/chiselFrontend/src/main/scala/chisel3/core/Module.scala index 0c69010b..fa9ab082 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Module.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Module.scala @@ -191,7 +191,7 @@ abstract class BaseModule extends HasId { * * TODO: remove this, perhaps by removing Bindings checks in compatibility mode. */ - def _compatAutoWrapPorts() {} + def _autoWrapPorts() {} // // BaseModule User API functions @@ -200,10 +200,29 @@ abstract class BaseModule extends HasId { Builder.annotations += annotation } - /** Chisel2 code didn't require the IO(...) wrapper and would assign a Chisel type directly to - * io, then do operations on it. This binds a Chisel type in-place (mutably) as an IO. - */ - protected def _bindIoInPlace(iodef: Data) { + /** + * This must wrap the datatype used to set the io field of any Module. + * i.e. All concrete modules must have defined io in this form: + * [lazy] val io[: io type] = IO(...[: io type]) + * + * Items in [] are optional. + * + * The granted iodef WILL NOT be cloned (to allow for more seamless use of + * anonymous Bundles in the IO) and thus CANNOT have been bound to any logic. + * This will error if any node is bound (e.g. due to logic in a Bundle + * constructor, which is considered improper). + * + * Also registers a Data as a port, also performing bindings. Cannot be called once ports are + * requested (so that all calls to ports will return the same information). + * Internal API. + * + * TODO(twigg): Specifically walk the Data definition to call out which nodes + * are problematic. + */ + protected def IO[T<:Data](iodef: T): iodef.type = { + require(!_closed, "Can't add more ports after module close") + requireIsChiselType(iodef, "io type") + // Compatibility code: Chisel2 did not require explicit direction on nodes // (unspecified treated as output, and flip on nothing was input). // This sets assigns the explicit directions required by newer semantics on @@ -229,38 +248,11 @@ abstract class BaseModule extends HasId { } } assignCompatDir(iodef, false) - + + // Bind each element of the iodef to being a Port iodef.bind(PortBinding(this)) _ports += iodef - } - - /** - * This must wrap the datatype used to set the io field of any Module. - * i.e. All concrete modules must have defined io in this form: - * [lazy] val io[: io type] = IO(...[: io type]) - * - * Items in [] are optional. - * - * The granted iodef WILL NOT be cloned (to allow for more seamless use of - * anonymous Bundles in the IO) and thus CANNOT have been bound to any logic. - * This will error if any node is bound (e.g. due to logic in a Bundle - * constructor, which is considered improper). - * - * Also registers a Data as a port, also performing bindings. Cannot be called once ports are - * requested (so that all calls to ports will return the same information). - * Internal API. - * - * TODO(twigg): Specifically walk the Data definition to call out which nodes - * are problematic. - */ - protected def IO[T<:Data](iodef: T): iodef.type = { - require(!_closed, "Can't add more ports after module close") - requireIsChiselType(iodef, "io type") - - // Clone the IO so we preserve immutability of data types - val iodefClone = iodef.cloneTypeFull - _bindIoInPlace(iodefClone) - iodefClone.asInstanceOf[iodef.type] + iodef } // diff --git a/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala b/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala index ac87eda5..9c923037 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala @@ -151,8 +151,8 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions) def io: Record // Allow access to bindings from the compatibility package - protected def _compatIoPortBound() = portsContains(io) - + protected def _ioPortBound() = portsContains(io) + protected override def nameIds(rootClass: Class[_]): HashMap[HasId, String] = { val names = super.nameIds(rootClass) @@ -165,7 +165,7 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions) } private[core] override def generateComponent(): Component = { - _compatAutoWrapPorts() // pre-IO(...) compatibility hack + _autoWrapPorts() // pre-IO(...) compatibility hack // Restrict IO to just io, clock, and reset require(io != null, "Module must have io") -- cgit v1.2.3 From 43e4d6dbd0f131c7484dc24cb09ab3a25614cb62 Mon Sep 17 00:00:00 2001 From: Richard Lin Date: Fri, 22 Dec 2017 01:21:41 -0800 Subject: address review style comments --- .../src/main/scala/chisel3/core/Aggregate.scala | 43 +++++++++------------- .../src/main/scala/chisel3/core/Data.scala | 8 ++-- 2 files changed, 21 insertions(+), 30 deletions(-) (limited to 'chiselFrontend') diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index ed3e1fee..43e430cd 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -559,6 +559,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { override def cloneType : 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] @@ -568,21 +569,18 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { // 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) match { - case Some(outerClass) => - val outerInstance = try { - clazz.getDeclaredField("$outer").get(this) // doesn't work in all cases, namely anonymous inner Bundles - } catch { - case _: NoSuchFieldException => this.outerModule match { - case Some(outerModule) => outerModule - case None => reflectError(s"Unable to determine instance of outer class $outerClass") - } - } - if (!outerClass.isAssignableFrom(outerInstance.getClass)) { - reflectError(s"Automatically determined outer class instance $outerInstance not assignable to outer class $outerClass") - } - Some(outerClass, outerInstance) - case None => None + val outerClassInstance = Option(clazz.getEnclosingClass).map { outerClass => + val outerInstance = try { + clazz.getDeclaredField("$outer").get(this) // doesn't work in all cases, namely anonymous inner Bundles + } catch { + case _: NoSuchFieldException => + this.outerModule.getOrElse(reflectError(s"Unable to determine instance of outer class $outerClass")) + } + if (!outerClass.isAssignableFrom(outerInstance.getClass)) { + reflectError(s"Unable to determine instance of outer class $outerClass," + + s" guessed $outerInstance, but is not assignable to outer class $outerClass") + } + (outerClass, outerInstance) } // If possible (constructor with no arguments), try Java reflection first @@ -686,18 +684,13 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { paramName => paramName -> instanceReflect.reflectMethod(accessorsMap(paramName)).apply() } - // Oppurtunistic sanity check: ensure any arguments of type Data is not bound + // 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.map{ case (paramName, paramVal) => paramVal match { - case paramVal: Data => paramVal.hasBinding match { - case true => Some(paramName) - case false => None - } - case _ => None - } - }.filter(_.isDefined).map(_.get) - if (!boundDataParamNames.isEmpty) { + 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.") } diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala index d84a86e9..35401fa1 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala @@ -209,11 +209,9 @@ abstract class Data extends HasId { } // If this Data is an instance of an inner class, record enclosing class - // This is only used for cloneType! - private[core] var outerModule: Option[BaseModule] = - (Option(this.getClass.getEnclosingClass) zip Builder.currentModule) - .find({ case (c, m) => c.isAssignableFrom(m.getClass) }) - .map({ case (_, m) => m }) + // This is only used for cloneType, and is mutable to allow autoCloneType to preserve the + // original outerModule. + private[core] var outerModule: Option[BaseModule] = Builder.currentModule // User-specified direction, local at this node only. // Note that the actual direction of this node can differ from child and parent specifiedDirection. -- cgit v1.2.3 From ade792ee7c5bb718f738f5e4c3886b2e87c68756 Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Thu, 28 Dec 2017 16:56:05 -0800 Subject: Add support for autoclonetype of bound, anonymous inner Bundles Also change Data.outerModule to Bundle._outerInst since it is only used in autoclonetype. _outerInst is also Option[Object] instead of Option[BaseModule] because the outer object could also be a Bundle. --- .../src/main/scala/chisel3/core/Aggregate.scala | 21 ++++++++++++++++----- .../src/main/scala/chisel3/core/Data.scala | 6 +----- 2 files changed, 17 insertions(+), 10 deletions(-) (limited to 'chiselFrontend') diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index 43e430cd..14011cd9 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -556,6 +556,11 @@ 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 = { // This attempts to infer constructor and arguments to clone this Bundle subtype without // requiring the user explicitly overriding cloneType. @@ -570,16 +575,22 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { // 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 => - this.outerModule.getOrElse(reflectError(s"Unable to determine instance of outer class $outerClass")) + // 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 (!outerClass.isAssignableFrom(outerInstance.getClass)) { + 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) } @@ -605,7 +616,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { } clone match { case Some(clone) => - clone.outerModule = this.outerModule + 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.") @@ -650,7 +661,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { require(ctors.size == 1) // should be consistent with Scala constructors try { val clone = ctors.head.newInstance(outerClassInstance.get._2).asInstanceOf[this.type] - clone.outerModule = this.outerModule + clone._outerInst = this._outerInst return clone } catch { case e @ (_: java.lang.reflect.InvocationTargetException | _: IllegalArgumentException) => @@ -703,7 +714,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record { case None => mirror.reflectClass(classSymbol) } val clone = classMirror.reflectConstructor(ctor).apply(ctorParamsVals:_*).asInstanceOf[this.type] - clone.outerModule = this.outerModule + clone._outerInst = this._outerInst if (!clone.typeEquivalent(this)) { reflectError(s"Automatically cloned $clone not type-equivalent to base $this." + diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala index 35401fa1..1cf50e9f 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala @@ -208,11 +208,6 @@ abstract class Data extends HasId { } } - // If this Data is an instance of an inner class, record enclosing class - // This is only used for cloneType, and is mutable to allow autoCloneType to preserve the - // original outerModule. - private[core] var outerModule: Option[BaseModule] = Builder.currentModule - // User-specified direction, local at this node only. // Note that the actual direction of this node can differ from child and parent specifiedDirection. private var _specifiedDirection: SpecifiedDirection = SpecifiedDirection.Unspecified @@ -245,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 -- cgit v1.2.3