Add multiple dimensions to VecInit fill and iterate #2065

anniej-sifive · 2021-08-10T23:01:32Z

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

API Impact

This extends the tabulate and fill functions for VecInit to create multi-dimensional Vecs
Ex. Vec[Vec[UInt]]

Backend Code Generation Impact

This should not change the backend code generation as it only provides different ways to fill a multi-dimensional Vec.

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

This PR adds builds upon companion object factory methods for the VecInit class, .fill and .iterate, to include multi-dimensional Vecs.

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.x, 3.5.0) ?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

…to vec-init-branch

…it-branch

mwachs5 · 2021-08-11T16:55:58Z

Can you update the PR title?

mwachs5 · 2021-08-11T16:57:14Z

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

+    myVec
+  }
+
+  /** Creates a new 4D [[Vec]] of length `n by m by p by q` composed of the result of the given


is there any particular reason you went to 4D, not 3D or 5D? Is this paralleling something else in the codebase and/or scala?

Scala seqs can go to 4D, so I think its to follow that pattern (https://www.scala-lang.org/api/2.13.3/scala/collection/SeqFactory.html)

Ah, I See that scala Seq.tabulate and so on have up to 5D versions. But there they don't have different names (tabulate vs tabulate2D), they just have different numbers of parameters to distinguish... any reason to not follow that pattern

https://www.scala-lang.org/api/2.13.6/scala/collection/Seq$.html#tabulate[A](n1:Int,n2:Int,n3:Int,n4:Int,n5:Int)(f:(Int,Int,Int,Int,Int)=%3EA):CC[CC[CC[CC[CC[A]]]]@scala.annotation.unchecked.uncheckedVariance]

I guess I might not encourage people to write 5D versions in hardware, and might stop at 3D for chisel... but willing to be convinced otherwise... what does the code even look like for a 4D or 5D generated vec?

Ah oh I guess I forgot that it went to 5D, @jackkoenig suggested to go to 4D and I assumed thats kinda where it came from... I could add a 5D if you want it?

i guess i'd be more inclined to go lower, not higher, but I'd defer to @jackkoenig on what he thinks is best. I'm not sure how realistic 5-d generated hardware would be / how happy tools like VCS would be about trying to do something with that

I was thinking 4D because it's what Seq did in Scala 2.12. I agree that 5D is a bit much and maybe 4D is too much. Maybe let's go to 3D for the time being?

debs-sifive · 2021-08-11T17:05:36Z

src/test/scala/chiselTests/Vec.scala

+    if (n == 0) assert(arr(n).asUInt() === compVec.asUInt)
+    else  {
+      assert(arr(n).asUInt() === compVec.asUInt())
+      assert2DIsCorrect(n-1, arr, compVec)
+    }


could this and other tests be simplified to:

Suggested change

if (n == 0) assert(arr(n).asUInt() === compVec.asUInt)

else {

assert(arr(n).asUInt() === compVec.asUInt())

assert2DIsCorrect(n-1, arr, compVec)

}

assert(arr(n).asUInt() === compVec.asUInt()

if (n != 0) assert2DIsCorrect(n-1, arr, compVec)

? Not sure if that will work, but seems equivalent and less repeat-y

mwachs5

could you add something to the docs / cookbook about the 2d usage, so that it's more discoverable?

jackkoenig

A bit of a long review because there are some subtleties here. I'd also like to see tests that check for bidirectional support.

jackkoenig · 2021-08-12T18:26:37Z

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

+  def do_tabulate[T <: Data](n: Int, m: Int)(gen: (Int) => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {
+    val myVec = Wire(Vec(n, Vec(m, cloneSupertype((0 until m).map(i => gen(i)), "Vec"))))
+    myVec.foreach {
+      _ := VecInit.tabulate(m)(gen)
+    }
+    myVec
+  }


Using := is actually not correct--VecInit.tabulate needs to support bidirectional aggregates and := is a unidirectional connection. Now unfortunately, just switching this implementation to using <> also won't work due to the fact that <> is commutative and thus it is ambiguous which direction connections should go if both sides of a <> are wires (if one side is a port, that side "anchors" the connection making it unambiguous). For example:

This works (both sides ports)

val enq = IO(Vec(4, Decoupled(UInt(8.W)))) val deq = IO(Flipped(Vec(4, Decoupled(UInt(8.W))))) deq <> enq

This works (one side port, one side wire)

val enq = IO(Vec(4, Decoupled(UInt(8.W)))) val deq = IO(Flipped(Vec(4, Decoupled(UInt(8.W))))) // Effectively what VecInit does val wire = Wire(Vec(4, Decoupled(UInt(8.W)))) wire <> enq deq <> wire

But this doesn't work (both sides wire):

val enq = IO(Vec(4, Decoupled(UInt(8.W)))) val deq = IO(Flipped(Vec(4, Decoupled(UInt(8.W))))) // Effectively what VecInit does val wire = Wire(Vec(4, Decoupled(UInt(8.W)))) wire <> enq val wire2 = Wire(Vec(4, Flipped(Decoupled(UInt(8.W))))) wire2 <> deq wire2 <> wire

In order to make this higher dimensional fills and tabulates work, they cannot create a Wire and delegate to a function that also creates a Wire. I think the only way to do this correctly is to create the wire in each tabulate and then connect every single element (no delegation).

Also, looking at the fact that the 1D tabulate delegates to do_apply which has this weird logic, I think we should preserve that functionality as well. I'm envisioning factoring that out into a private higher-order function like so:

private def getConnectOpFromDirectionality[T <: Data](proto: T): (T, T) => Unit = proto.direction match { case ActualDirection.Input | ActualDirection.Output | ActualDirection.Unspecified => // When internal wires are involved, driver / sink must be specified explicitly, otherwise // the system is unable to infer which is driver / sink (x, y) => x := y case ActualDirection.Bidirectional(_) => // For bidirectional, must issue a bulk connect so subelements are resolved correctly. // Bulk connecting two wires may not succeed because Chisel frontend does not infer // directions. (x, y) => x <> y }

Then you could use that for this function as:

Suggested change

def do_tabulate[T <: Data](n: Int, m: Int)(gen: (Int) => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {

val myVec = Wire(Vec(n, Vec(m, cloneSupertype((0 until m).map(i => gen(i)), "Vec"))))

myVec.foreach {

_ := VecInit.tabulate(m)(gen)

}

myVec

}

// Also gen should take more Ints, 1 per dimensionality

def do_tabulate[T <: Data](n: Int, m: Int)(gen: (Int, Int) => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {

// TODO make this lazy (requires LazyList and cross compilation, beyond the scope of this PR)

val elts = Seq.tabulate(n, m)(gen)

val flatElts = elts.flatten

// These checks are copied from do_apply, perhaps some room for factoring out

require(flatElts.nonEmpty, "Vec hardware values are not allowed to be empty")

flatElts.foreach(requireIsHardware(_, "vec element"))

val tpe = cloneSupertype(flatElts, "Vec.tabulate")

val myVec = Wire(Vec(n, Vec(m, tpe)))

val op = getConnectOpFromDirectionality(tpe)

// We could get fancy here to have reuse across the multiple dimensions, but being more verbose is more clear

for {

(xs1D, ys1D) <- myVec zip elts

(x, y) <- xs1D zip ys1D

} do op(x, y)

myVec

}

I know this is a lot, and this is made complex by the lack of a non-commutative <>, but it is what it is.

Instead of the nested for, we could actually just flatten and iterate:

for ((x, y) <- myVec.flatten zip flatElts) { op(x, y) }

This requires fewer intermediate names and would work basically the same way for the higher dimensional versions (with extra .flattens)

jackkoenig · 2021-08-12T18:28:57Z

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

+  def do_fill[T <: Data](n: Int, m: Int)(gen: => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {
+    val myVec = Wire(Vec(n, Vec(m, cloneSupertype(Seq.fill(m)(gen), "Vec"))))
+    myVec.foreach {
+      _ := VecInit.fill(m)(gen)
+    }
+    myVec
+  }


Note that fill is just a special case of tabulate (where you ignore the indices) so this can be implemented as just:

Suggested change

def do_fill[T <: Data](n: Int, m: Int)(gen: => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {

val myVec = Wire(Vec(n, Vec(m, cloneSupertype(Seq.fill(m)(gen), "Vec"))))

myVec.foreach {

_ := VecInit.fill(m)(gen)

}

myVec

}

def do_fill[T <: Data](n: Int, m: Int)(gen: => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] =

do_tabulate(n, m)((_, _) => gen)

jackkoenig · 2021-08-12T18:30:28Z

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

+    myVec
+  }
+
+  /** Creates a new 4D [[Vec]] of length `n by m by p by q` composed of the result of the given


I was thinking 4D because it's what Seq did in Scala 2.12. I agree that 5D is a bit much and maybe 4D is too much. Maybe let's go to 3D for the time being?

…it-branch

jackkoenig

Some nits and then a couple of suggestions on the tests.

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

docs/src/cookbooks/cookbook.md

jackkoenig · 2021-08-19T00:06:34Z

docs/src/cookbooks/cookbook.md

+
+}
+```
+


It would be good to actually run this test too:

Suggested change

```scala mdoc:invisible

// Hidden but will make sure this actually compiles

ChiselStage.emitVerilog(new Foo)

```

macros/src/main/scala/chisel3/internal/sourceinfo/SourceInfoTransform.scala

src/test/scala/chiselTests/Vec.scala

jackkoenig · 2021-08-19T00:24:32Z

src/test/scala/chiselTests/Vec.scala

+    }
+    stop()
+  }
+}


Also there are no tests for the .tabulate. I'd do something similar but it might be useful to factor in the indices in the tabulate into the test somehow.

The fills call tabulate, bc they are a special case of tabulate, so I figured by calling 2D/3D fill I would be catching tabulate within that.

They do right now, but part of the goal of tests is catching if someone were to refactor things and get it slightly wrong. Perhaps after a refactor fill won't call tabulate anymore and I'll bet the person doing the refactor won't bother to check the tests.

…it-branch

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

jackkoenig

LGTM! Great work @anniej-sifive!

anniej-sifive and others added 6 commits August 2, 2021 10:55

Added VecInit factory methods (fill,iterate)

b00597d

Merge branch 'master' into vec-init-branch

6c81326

Spacing fix

5d96a3a

Merge branch 'vec-init-branch' of github.com:anniej-sifive/chisel3 in…

d2f127c

…to vec-init-branch

Added multi-dimension to fill/iterate

a035c55

Merge branch 'master' of github.com:anniej-sifive/chisel3 into vec-in…

4198cd3

…it-branch

mwachs5 reviewed Aug 11, 2021

View reviewed changes

debs-sifive reviewed Aug 11, 2021

View reviewed changes

anniej-sifive changed the title ~~Vec init branch~~ Add multiple dimensions to VecInit fill and iterate Aug 11, 2021

mwachs5 reviewed Aug 11, 2021

View reviewed changes

jackkoenig requested changes Aug 12, 2021

View reviewed changes

anniej-sifive added 3 commits August 18, 2021 15:28

updated to handle bidirectional connection

5313df3

Merge branch 'master' of github.com:anniej-sifive/chisel3 into vec-in…

03359fe

…it-branch

added scala doc comment to getConnectOpFromDirectionality

ba88046

jackkoenig reviewed Aug 19, 2021

View reviewed changes

anniej-sifive added 4 commits August 18, 2021 17:59

Merge branch 'master' of github.com:anniej-sifive/chisel3 into vec-in…

0a2feeb

…it-branch

syntax updates and spacing fixes

0a88515

spacing

794a289

cookbook addition

c693d7c

jackkoenig reviewed Aug 19, 2021

View reviewed changes

core/src/main/scala/chisel3/Aggregate.scala Outdated Show resolved Hide resolved

added tabulate bidirectional tests

3d17456

jackkoenig approved these changes Aug 23, 2021

View reviewed changes

jackkoenig added this to the 3.5.0 milestone Aug 23, 2021

Merge branch 'master' into vec-init-branch

d5e7853

jackkoenig merged commit f50ce19 into chipsalliance:master Aug 23, 2021

MAFOR-TECH mentioned this pull request Oct 24, 2021

Chisel 3.5 Release Candidate 1 #2202

Closed

-  def do_tabulate[T <: Data](n: Int, m: Int)(gen: (Int) => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {
-    val myVec = Wire(Vec(n, Vec(m, cloneSupertype((0 until m).map(i => gen(i)), "Vec"))))
-    myVec.foreach {
-      _ := VecInit.tabulate(m)(gen)
-    }
-    myVec
-  }
+  // Also gen should take more Ints, 1 per dimensionality
+  def do_tabulate[T <: Data](n: Int, m: Int)(gen: (Int, Int) => T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Vec[Vec[T]] = {
+    // TODO make this lazy (requires LazyList and cross compilation, beyond the scope of this PR)
+    val elts = Seq.tabulate(n, m)(gen)
+    val flatElts = elts.flatten
+    // These checks are copied from do_apply, perhaps some room for factoring out
+    require(flatElts.nonEmpty, "Vec hardware values are not allowed to be empty")
+    flatElts.foreach(requireIsHardware(_, "vec element"))
+    val tpe = cloneSupertype(flatElts, "Vec.tabulate")
+    val myVec = Wire(Vec(n, Vec(m, tpe)))
+    val op = getConnectOpFromDirectionality(tpe)
+    // We could get fancy here to have reuse across the multiple dimensions, but being more verbose is more clear
+    for {
+      (xs1D, ys1D) <- myVec zip elts
+      (x, y) <- xs1D zip ys1D
+    } do op(x, y)
+    myVec
+  }

+```scala mdoc:invisible
+// Hidden but will make sure this actually compiles
+ChiselStage.emitVerilog(new Foo)
+```

Add multiple dimensions to VecInit fill and iterate #2065

Add multiple dimensions to VecInit fill and iterate #2065

Uh oh!

Conversation

anniej-sifive commented Aug 10, 2021

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Uh oh!

mwachs5 commented Aug 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwachs5 Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwachs5 Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwachs5 left a comment

Choose a reason for hiding this comment

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anniej-sifive Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mwachs5 Aug 11, 2021 •

edited

Loading

mwachs5 Aug 11, 2021 •

edited

Loading

anniej-sifive Aug 19, 2021 •

edited

Loading