VecLiterals First Pass #1793
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,15 +5,14 @@ package chisel3 | |
| import scala.collection.immutable.ListMap | ||
| import scala.collection.mutable.{HashSet, LinkedHashMap} | ||
| import scala.language.experimental.macros | ||
| import chisel3.experimental.{BaseModule, BundleLiteralException, ChiselEnum, EnumType, VecLiteralException} | ||
| import chisel3.internal._ | ||
| import chisel3.internal.Builder.pushCommand | ||
| import chisel3.internal.firrtl._ | ||
| import chisel3.internal.sourceinfo._ | ||
|
|
||
| import scala.collection.mutable | ||
|
|
||
| class AliasedAggregateFieldException(message: String) extends ChiselException(message) | ||
|
|
||
| /** An abstract class for data types that solely consist of (are an aggregate | ||
|
|
@@ -51,19 +50,32 @@ sealed abstract class Aggregate extends Data { | |
| */ | ||
| override def litOption: Option[BigInt] = { | ||
| // Shift the accumulated value by our width and add in our component, masked by our width. | ||
| def shiftAdd(accumulator: Option[BigInt], elt: Data): Option[BigInt] = { | ||
| (accumulator, elt.litOption()) match { | ||
| case (Some(accumulator), Some(eltLit)) => | ||
| val width = elt.width.get | ||
| val masked = ((BigInt(1) << width) - 1) & eltLit // also handles the negative case with two's complement | ||
| Some((accumulator << width) + masked) | ||
| case (None, _) => None | ||
| case (_, None) => None | ||
| } | ||
| } | ||
|
|
||
| topBindingOpt match { | ||
| case Some(BundleLitBinding(_)) => | ||
| getElements | ||
| .reverse | ||
| .foldLeft[Option[BigInt]](Some(BigInt(0)))(shiftAdd) | ||
| case Some(VecLitBinding(elementsToValue)) => | ||
| if (elementsToValue.size != this.asInstanceOf[Vec[_]].length) { | ||
| None | ||
| } else { | ||
| elementsToValue | ||
| .toList | ||
| .map(_._1) | ||
| .reverse | ||
| .foldLeft[Option[BigInt]](Some(BigInt(0)))(shiftAdd) | ||
| } | ||
| case _ => None | ||
| } | ||
| } | ||
|
|
@@ -155,9 +167,18 @@ trait VecFactory extends SourceInfoDoc { | |
| */ | ||
| sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int) | ||
| extends Aggregate with VecLike[T] { | ||
|
|
||
| override def toString: String = { | ||
| val bindingString = topBindingOpt match { | ||
| case Some(VecLitBinding(vecLitBinding)) => | ||
| val contents = vecLitBinding.zipWithIndex.map { case ((data, lit), index) => | ||
| s"$index=$lit" | ||
| }.mkString(", ") | ||
| s"($contents" | ||
| case _ => bindingToString | ||
| } | ||
| val elementType = sample_element.cloneType | ||
| s"$elementType[$length]$bindingString" | ||
| } | ||
|
|
||
| private[chisel3] override def typeEquivalent(that: Data): Boolean = that match { | ||
|
|
@@ -318,6 +339,190 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int) | |
| } | ||
| curLayer(0) | ||
| } | ||
|
|
||
| /** Creates a Vec literal of this type with specified values. this must be a chisel type. | ||
| * | ||
| * @param elementInitializers literal values, specified as a pair of the Vec field to the literal value. | ||
| * The Vec field is specified as a function from an object of this type to the field. | ||
| * Fields that aren't initialized to DontCare, and assignment to a wire will overwrite any | ||
| * existing value with DontCare. | ||
| * @return a Vec literal of this type with subelement values specified | ||
| * | ||
| * @example {{{ | ||
| * class MyVec extends Vec { | ||
| * val a = UInt(8.W) | ||
| * val b = Bool() | ||
| * } | ||
| * | ||
| * (mew MyVec).Lit( | ||
| * _.a -> 42.U, | ||
| * _.b -> true.B | ||
| * ) | ||
| * }}} | ||
| */ | ||
| private[chisel3] def _makeLit(elementInitializers: (Int, Data)*): this.type = { | ||
|
|
||
| // Returns pairs of all fields, element-level and containers, in a Record and their path names | ||
| def getRecursiveFields(data: Data, path: String): Seq[(Data, String)] = data match { | ||
| case data: Record => | ||
| data.elements.map { case (fieldName, fieldData) => | ||
| getRecursiveFields(fieldData, s"$path.$fieldName") | ||
| }.fold(Seq(data -> path)) { | ||
| _ ++ _ | ||
| } | ||
| case data: Vec[_] => | ||
| data.getElements.zipWithIndex.map { case (fieldData, fieldIndex) => | ||
| getRecursiveFields(fieldData, path = s"$path($fieldIndex)") | ||
| }.fold(Seq(data -> path)) { | ||
| _ ++ _ | ||
| } | ||
| case data => Seq(data -> path) // we don't support or recurse into other Aggregate types here | ||
| } | ||
|
|
||
| // Returns pairs of corresponding fields between two Vecs of the same type and size | ||
| def getMatchedFields(x: Data, y: Data): Seq[(Data, Data)] = (x, y) match { | ||
| case (x: Element, y: Element) => | ||
| require(x typeEquivalent y) | ||
| Seq(x -> y) | ||
| case (x: Record, y: Record) => | ||
|
There was a problem hiding this comment. In both the Record and Vec case we might want to check for typeEquivalent and element length equality, since zip will silently truncate |
||
| (x.elements zip y.elements).map { case ((xName, xElt), (yName, yElt)) => | ||
| require(xName == yName) // assume fields returned in same, deterministic order | ||
| getMatchedFields(xElt, yElt) | ||
| }.fold(Seq(x -> y)) { | ||
| _ ++ _ | ||
| } | ||
| case (x: Vec[_], y: Vec[_]) => | ||
| (x.getElements zip y.getElements).map { case (xElt, yElt) => | ||
| getMatchedFields(xElt, yElt) | ||
| }.fold(Seq(x -> y)) { | ||
| _ ++ _ | ||
| } | ||
| } | ||
|
|
||
| def checkLiteralConstruction(): Unit = { | ||
| val dupKeys = elementInitializers.map { x => x._1 }.groupBy(x => x).flatMap { case (k, v) => | ||
|
There was a problem hiding this comment. There are shorter ways to detect duplicates, comparing size against More set operations can probably get the duplicated keys. You don't get the count, but I'd think the index is the important aspect. |
||
| if (v.length > 1) { | ||
| Some(k, v.length) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| if (dupKeys.nonEmpty) { | ||
| throw new VecLiteralException( | ||
| s"VecLiteral: has duplicated indices ${dupKeys.map { case (k, n) => s"$k($n times)" }.mkString(",")}" | ||
| ) | ||
| } | ||
|
|
||
| val outOfRangeIndices = elementInitializers.map(_._1).flatMap { | ||
|
|
||
| case fieldIndex if fieldIndex < 0 || fieldIndex >= length => Some(fieldIndex) | ||
| case _ => None | ||
| } | ||
| if (outOfRangeIndices.nonEmpty) { | ||
| throw new VecLiteralException( | ||
| s"VecLiteral: The following indices (${outOfRangeIndices.mkString(",")}) " + | ||
| s"are less than zero or greater or equal to than Vec length" | ||
| ) | ||
| } | ||
|
|
||
| val badLits = elementInitializers.flatMap { | ||
|
There was a problem hiding this comment.
Also, I don't think we do width checking elsewhere in chisel frontend? We don't need to special-case Vecs. |
||
| case (index, lit) if (!(gen.getClass == lit.getClass)) => Some(index -> lit) | ||
| (sample_element.width, lit.width) match { | ||
| case (KnownWidth(m), KnownWidth(n)) => | ||
| if (m < n) Some(index -> lit) else None | ||
| case (KnownWidth(_), _) => | ||
| None | ||
| case (UnknownWidth(), _) => | ||
| None | ||
| case _ => | ||
| Some(index -> lit) | ||
| } | ||
| case _ => None | ||
| } | ||
| if (badLits.nonEmpty) { | ||
| throw new VecLiteralException( | ||
| s"VecLiteral: Vec[${gen}] has the following incorrectly typed or sized initializers: " + | ||
| badLits.map { case (a, b) => s"$a -> $b" }.mkString(",") | ||
| ) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| requireIsChiselType(this, "vec literal constructor model") | ||
| checkLiteralConstruction() | ||
|
There was a problem hiding this comment. Does it make sense to just inline the checks here, perhaps with comments for visual delimiting? |
||
|
|
||
| val clone = cloneType | ||
| val cloneFields = getRecursiveFields(clone, "(vec root)").toMap | ||
|
|
||
| // Create the Vec literal binding from litargs of arguments | ||
| val vecLitLinkedMap = new mutable.LinkedHashMap[Data, LitArg]() | ||
| elementInitializers.sortBy { case (a, _) => a }.foreach { case (fieldIndex, value) => | ||
|
|
||
| val field = apply(fieldIndex) | ||
| val clonedField = clone(fieldIndex) | ||
|
|
||
| val valueBinding = value.topBindingOpt match { | ||
| case Some(litBinding: LitBinding) => litBinding | ||
| case _ => throw new VecLiteralException(s"field $fieldIndex specified with non-literal value $value") | ||
| } | ||
|
|
||
| value match { // Get the litArg(s) for this field | ||
| case vecField: Vec[_] => | ||
|
There was a problem hiding this comment. Does it make sense to keep the values consistent (value vs. vecField)? Otherwise it seems to be confusing to refer to either, when they're both the same thing? |
||
| if (!(vecField typeEquivalent value)) { | ||
|
There was a problem hiding this comment. Will this ever happen? vecField is the same object as value, is it not? |
||
| throw new VecLiteralException(s"field $fieldIndex $vecField specified with non-type-equivalent value $value") | ||
| } | ||
| // Copy the source VecLitBinding with vecFields (keys) remapped to the clone | ||
| val remap = getMatchedFields(value, clonedField).toMap | ||
| value.topBinding.asInstanceOf[VecLitBinding].litMap.map { case (valueField, valueValue) => | ||
| vecLitLinkedMap(remap(valueField)) = valueValue | ||
| } | ||
| case bitField: Bits => | ||
| val adjustedBitField = if (bitField.isInstanceOf[Bool]) { | ||
|
There was a problem hiding this comment. Why is this special case needed? |
||
| bitField | ||
| } else { | ||
| bitField.cloneTypeWidth(field.getWidth.W) | ||
| } | ||
|
|
||
| val notSameClass = !field.typeEquivalent(adjustedBitField) | ||
| if (notSameClass) { // TODO typeEquivalent is too strict because it checks width | ||
| throw new VecLiteralException( | ||
| s"VecLit: Literal specified at index $fieldIndex ($value) does not match Vec type $sample_element" | ||
|
There was a problem hiding this comment. Do we do these checks in the frontend elsewhere? If so, the code should be unified with that, and if not, I don't think it's a good idea to have mixed semantics There was a problem hiding this comment. I'm not completely sure. In general I think errors should be reported as early as possible and at compile time over run time (or firrtl time) also when possible. I think the error messages can be more helpful with information that is possessed at earlier stages |
||
| ) | ||
| } else if (bitField.getWidth > field.getWidth) { | ||
| throw new VecLiteralException( | ||
| s"VecLit: Literal specified at index $fieldIndex ($value) is too wide for Vec type $sample_element" | ||
| ) | ||
| } | ||
| val litArg = valueBinding match { | ||
| case ElementLitBinding(litArg) => litArg | ||
| case VecLitBinding(litMap) => litMap.getOrElse(value, | ||
| throw new VecLiteralException(s"Field $fieldIndex specified with unspecified value")) | ||
| } | ||
| val adjustedLitArg = litArg.cloneWithWidth(sample_element.width) | ||
| vecLitLinkedMap(clonedField) = adjustedLitArg | ||
| case recordField: Record => | ||
| if (!(recordField typeEquivalent value)) { | ||
| throw new VecLiteralException(s"field $fieldIndex $recordField specified with non-type-equivalent value $value") | ||
| } | ||
| // Copy the source BundleLitBinding with fields (keys) remapped to the clone | ||
| val remap = getMatchedFields(value, recordField).toMap | ||
| value.topBinding.asInstanceOf[VecLitBinding].litMap.map { case (valueField, valueValue) => | ||
| vecLitLinkedMap(remap(valueField)) = valueValue | ||
| } | ||
| case enumField: EnumType => { | ||
| if (!(enumField typeEquivalent value)) { | ||
| throw new VecLiteralException(s"field $fieldIndex $enumField specified with non-type-equivalent enum value $value") | ||
| } | ||
| val litArg = valueBinding match { | ||
| case ElementLitBinding(litArg) => litArg | ||
| } | ||
| vecLitLinkedMap(clonedField) = litArg | ||
| } | ||
| case _ => throw new VecLiteralException(s"unsupported field $fieldIndex of type $field") | ||
| } | ||
| } // don't convert to a Map yet to preserve duplicate keys | ||
|
|
||
| clone.bind(VecLitBinding(ListMap(vecLitLinkedMap.toSeq:_*))) | ||
| clone | ||
| } | ||
| } | ||
|
|
||
| object VecInit extends SourceInfoDoc { | ||
|
|
@@ -530,9 +735,14 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio | |
| private[chisel3] def _makeLit(elems: (this.type => (Data, Data))*): this.type = { | ||
| // Returns pairs of all fields, element-level and containers, in a Record and their path names | ||
| def getRecursiveFields(data: Data, path: String): Seq[(Data, String)] = data match { | ||
| case data: Record => | ||
| data.elements.map { case (fieldName, fieldData) => | ||
| getRecursiveFields(fieldData, s"$path.$fieldName") | ||
| }.fold(Seq(data -> path)) { _ ++ _ } | ||
| case data: Vec[_] => | ||
| data.getElements.zipWithIndex.map { case (fieldData, fieldIndex) => | ||
| getRecursiveFields(fieldData, path = s"$path($fieldIndex") | ||
| }.fold(Seq(data -> path)) { _ ++ _ } | ||
| case data => Seq(data -> path) // we don't support or recurse into other Aggregate types here | ||
| } | ||
|
|
||
|
|
@@ -546,6 +756,10 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio | |
| require(xName == yName) // assume fields returned in same, deterministic order | ||
| getMatchedFields(xElt, yElt) | ||
| }.fold(Seq(x -> y)) { _ ++ _ } | ||
| case (x: Vec[_], y: Vec[_]) => | ||
| (x.getElements zip y.getElements).map { case (xElt, yElt) => | ||
| getMatchedFields(xElt, yElt) | ||
| }.fold(Seq(x -> y)) { _ ++ _ } | ||
| } | ||
|
|
||
| requireIsChiselType(this, "bundle literal constructor model") | ||
|
|
@@ -572,7 +786,12 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio | |
| val litArg = valueBinding match { | ||
| case ElementLitBinding(litArg) => litArg | ||
| case BundleLitBinding(litMap) => litMap.getOrElse(value, | ||
| throw new BundleLiteralException(s"Field $fieldName specified with unspecified value") | ||
| ) | ||
| case VecLitBinding(litMap) => litMap.getOrElse(value, | ||
| throw new VecLiteralException(s"Vec literal $fieldName specified with out literal values") | ||
| ) | ||
|
|
||
| } | ||
| Seq(field -> litArg) | ||
| case field: Record => | ||
|
|
@@ -584,6 +803,15 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio | |
| value.topBinding.asInstanceOf[BundleLitBinding].litMap.map { case (valueField, valueValue) => | ||
| remap(valueField) -> valueValue | ||
| } | ||
| case vecField: Vec[_] => | ||
| if (!(vecField typeEquivalent value)) { | ||
| throw new BundleLiteralException(s"field $fieldName $vecField specified with non-type-equivalent value $value") | ||
| } | ||
| // Copy the source BundleLitBinding with fields (keys) remapped to the clone | ||
| val remap = getMatchedFields(value, vecField).toMap | ||
| value.topBinding.asInstanceOf[VecLitBinding].litMap.map { case (valueField, valueValue) => | ||
| remap(valueField) -> valueValue | ||
| } | ||
| case field: EnumType => { | ||
| if (!(field typeEquivalent value)) { | ||
| throw new BundleLiteralException(s"field $fieldName $field specified with non-type-equivalent enum value $value") | ||
|
|
@@ -694,6 +922,7 @@ class AutoClonetypeException(message: String) extends ChiselException(message) | |
| package experimental { | ||
|
|
||
| class BundleLiteralException(message: String) extends ChiselException(message) | ||
| class VecLiteralException(message: String) extends ChiselException(message) | ||
|
|
||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be merged with Record._makeLit? It seems like a lot of helper functions share the same structure, and updates to both would be needed to support like Vecs in Bundles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea. But I would not be very comfortable trying this right now. There's pretty much as many differences as there are similarities. Going DRY might break bundle lits in some subtle way, where as keeping it separate should keep any logic flaws limited to Vecs