VecLiterals First Pass #1793

chick · 2021-02-26T19:03:42Z

Introduces a requested feature Vec literals. Much like Bundle literals, this allows
developers to specify a Vec aggregate literal in a convenient (relatively) compact form
e.g.

  val vecLiteral = Vec(2, UInt(4.W)).Lit(0 -> 0xA.U, 1 -> 0xB.U)

Features

Can be defined outside of module (enables use with chiseltest library)
Can be sparsely specified (with the effect that top level litOption returns None)
litOption behaves like to UInt in bit ordering
works as Reg initializer
Decent error exceptions for
- duplicate keys are found
- wrong data types employed
- width of individual element exceeds the basic Vec width

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

New feature

API Impact

This is an API addition. the Vec type now has a Lit method creates a Vec literal.

Backend Code Generation Impact

This should not impact generated verilog

Desired Merge Strategy

Squash: The PR will be squashed and merged

Release Notes

Introduces Vec literals which are similar to Bundle Literals. Simple example:

  val vecLiteral = Vec(2, UInt(8.W)).Lit(0 -> 0xA.U, 1 -> 0xB.U)

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

Making progress Lits can now be specified and pack Tests are being built - restored some BundleLit tests - working through a list of vec literal tests - adds a cloneWithWidth method to LitArg subclasses - Fixed problem with nested vec lits Vec literals tests pass A bit of cleanup Made bundle-literals support vec literals underneath

Remove println and add some scaladoc

# Conflicts: # core/src/main/scala/chisel3/Aggregate.scala # src/test/scala/chiselTests/ChiselSpec.scala

One test that broke is fixed

ducky64

At a high level this seems sane, but a few comments.

Overall themes:

I think there's code paths that can be shared and DRYed out with the bundle lits code
It might make sense to keep the strictness / checks consistent with normal elements, instead of special casing strict checks in vec literal construction? Good alternatives might to be apply them everywhere universally (and perhaps the shared infrastructure can lead to cleaner code), or just remove them (especially if they're caught in FIRRTL). In general, I think consistency in behavior is best.
Some style / cleanliness issues

ducky64 · 2021-03-02T03:51:26Z

core/src/main/scala/chisel3/Aggregate.scala

      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
    }
+
+    // Shift the accumulated value by our width and add in our component, masked by width of Vec element.
+    def shiftAddVec(accumulator: Option[BigInt], data: Data, elt: LitArg): Option[BigInt] = {


This duplicates a lot of the logic above. I don't fully understand it (yet?), but it seems like perhaps with a bit more thought we might unify the two code paths?

Reduced this to a single ShiftAdd for both Vec and Bundle

ducky64 · 2021-03-02T03:54:01Z

core/src/main/scala/chisel3/Aggregate.scala

+    val bindingString = topBindingOpt match {
+      case Some(VecLitBinding(vecLitBinding)) =>
+        val contents = vecLitBinding.map { case (data, lit) =>
+          s"$data=$lit"


Are you sure this does what we want it to do? Presumably, you want index=lit, but as is this seems like the index is going to be a ugly Data dump. Note that in Record.toString, it uses name from elements, then mixes it with the binding contents.

Good catch, now it shows indices, e.g. SInt<4>[2](0=SLit(1,<4>), 1=SLit(-2,<4>)

ducky64 · 2021-03-02T03:57:15Z

core/src/main/scala/chisel3/Aggregate.scala

+  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 {


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?

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

ducky64 · 2021-03-02T03:59:26Z

core/src/main/scala/chisel3/Aggregate.scala

+      case (x: Element, y: Element) =>
+        require(x typeEquivalent y)
+        Seq(x -> y)
+      case (x: Record, y: Record) =>


In both the Record and Vec case we might want to check for typeEquivalent and element length equality, since zip will silently truncate

ducky64 · 2021-03-02T04:05:59Z

core/src/main/scala/chisel3/Aggregate.scala

+    }
+
+    def checkLiteralConstruction(): Unit = {
+      val dupKeys = elementInitializers.map { x => x._1 }.groupBy(x => x).flatMap { case (k, v) =>


There are shorter ways to detect duplicates, comparing size against .distinct or .toSet
https://stackoverflow.com/questions/7691971/easiest-way-to-decide-if-list-contains-duplicates

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.

ducky64 · 2021-03-02T04:27:45Z

core/src/main/scala/chisel3/internal/firrtl/IR.scala

+    * @param newWidth the new width for this
+    * @return
+    */
+  def cloneWithWidth(newWidth: Width): this.type


Is this needed? Is the root cause here that it's possible to specify a width in both the Data and the LitArg and those two can be out of sync? Do we need to do something special for VecLiteral that needs this new code?

ducky64 · 2021-03-02T04:28:33Z

src/test/scala/chiselTests/BundleLiteralSpec.scala

@@ -47,6 +46,24 @@ class BundleLiteralSpec extends ChiselFlatSpec with Utils {
    } }
  }

+  "bundle literals" should "not pack when sparsely specified" in {


Should this have gone in its own PR? Doesn't look related to Vec literals?

ducky64 · 2021-03-02T04:30:48Z

core/src/main/scala/chisel3/Aggregate.scala

+          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"


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

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

ducky64 · 2021-03-02T04:31:53Z

core/src/main/scala/chisel3/internal/Binding.scala

@@ -123,3 +125,5 @@ sealed trait LitBinding extends UnconstrainedBinding with ReadOnlyBinding
 case class ElementLitBinding(litArg: LitArg) extends LitBinding
 // Literal binding attached to the root of a Bundle, containing literal values of its children.
 case class BundleLitBinding(litMap: Map[Data, LitArg]) extends LitBinding
+// Literal binding attached to the root of a Vec, containing literal values of its children.
+case class VecLitBinding(litMap: mutable.LinkedHashMap[Data, LitArg]) extends LitBinding


I don't think this should be mutable. It might make sense to use mutable to build the final data structure, but the stored data structure should be immutable.

Switched after generation to immutable.ListMap, important to maintain order

ducky64 · 2021-03-02T04:36:43Z

core/src/main/scala/chisel3/Aggregate.scala

+      }
+
+      value match { // Get the litArg(s) for this field
+        case vecField: Vec[_] =>


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?

convert from mutable.LinkedHashMap to immutable.ListMap after generation to preserve key ordering

But is this true: `defaulting any unconnected fields to 0 (regardless of type)` for either Bundles or Vec literals.

chick · 2021-03-02T18:57:15Z

Added some mdoc for vec lits, plagiarized from bundle lits

mwachs5 · 2021-03-02T19:55:38Z

docs/src/appendix/experimental-features.md

+chisel3.stage.ChiselStage.emitVerilog(new Example3)
+```
+
+Registers can be partially initialized from Vec literals. Only specified elements will be assigned.


does that mean that some registers will have reset values and some will not? Can you make that more clear in this sentence, whatever the answer is?

chick · 2021-03-24T01:48:56Z

Closing this in favor of PR #1834

Add a Treadle build to the portion of CI that runs Chisel3 tests. Chisel3 needs to have a master copy of Treadle in order for it to work and cannot rely on resolution from Maven. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>

chick added 2 commits February 26, 2021 10:23

Some final cleanup

38392ed

Remove println and add some scaladoc

chick added release issue Feature New feature, will be included in release notes labels Feb 26, 2021

chick added this to the 3.5.0 milestone Feb 26, 2021

chick requested review from ducky64 and jackkoenig February 26, 2021 19:03

chick self-assigned this Feb 26, 2021

chick added 3 commits February 26, 2021 11:28

Merge branch 'master' into vec-literals

44378d2

# Conflicts: # core/src/main/scala/chisel3/Aggregate.scala # src/test/scala/chiselTests/ChiselSpec.scala

Fix conflicts after merging master.

ea53417

One test that broke is fixed

Merge branch 'master' into vec-literals

2bf83ee

jackkoenig mentioned this pull request Feb 26, 2021

Vec literals #849

Closed

ducky64 requested changes Mar 2, 2021

View reviewed changes

chick added 5 commits March 2, 2021 09:21

DRY out the litOption code in Aggregate

f5799a2

Make toString on VecLit should indices

5ae98a4

Remove duplicated sparse bundle lit test

1af275d

Make vecLitMap immutable.

b114fd8

convert from mutable.LinkedHashMap to immutable.ListMap after generation to preserve key ordering

Add mdoc for bundle literals

109ad9b

But is this true: `defaulting any unconnected fields to 0 (regardless of type)` for either Bundles or Vec literals.

chick added 3 commits March 2, 2021 10:57

Merge branch 'master' into vec-literals

3fa0465

Add a couple more examples of Vec lit

8b7cb0f

Fix typo in mdoc

b9e862a

mwachs5 reviewed Mar 2, 2021

View reviewed changes

chick added 2 commits March 2, 2021 13:04

Some more mdoc fixes

fcab26d

Some more mdoc fixes

e9dec0d

chick closed this Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VecLiterals First Pass #1793

VecLiterals First Pass #1793

chick commented Feb 26, 2021

ducky64 left a comment

ducky64 Mar 2, 2021

chick Mar 2, 2021

ducky64 Mar 2, 2021

chick Mar 2, 2021

ducky64 Mar 2, 2021

chick Mar 2, 2021

ducky64 Mar 2, 2021

ducky64 Mar 2, 2021

ducky64 Mar 2, 2021

ducky64 Mar 2, 2021

chick Mar 2, 2021

ducky64 Mar 2, 2021

chick Mar 2, 2021

ducky64 Mar 2, 2021

chick Mar 2, 2021

ducky64 Mar 2, 2021

chick commented Mar 2, 2021

mwachs5 Mar 2, 2021

chick commented Mar 24, 2021

VecLiterals First Pass #1793

VecLiterals First Pass #1793

Conversation

chick commented Feb 26, 2021

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick commented Mar 2, 2021

Choose a reason for hiding this comment

chick commented Mar 24, 2021