Heterogeneous Vec (MixedVec) #850

edwardcwang · 2018-07-08T19:37:47Z

As discussed with @jackkoenig, this implements the action items which fell out of our discussion in #844.

This PR creates a new HeterogeneousVec type inspired by rocket-chip's HeterogeneousBag or HetVec from dsptools. This is a dynamically indexable type like Vec but supports different underlying types - e.g. myVec(0) could return a UInt(8.W) while myVec(1) could return UInt(16.W), which is not supported by Vec, and as shown in #844 we cannot use the Seq alternative in Bundle construction as we can in Module construction.

Additionally, as discussed in #844, this PR also adds an (optionally-overridable) warning when public Seq[Data] members are present so that users can get a better error message than the current one. The ability to have non-Chisel Data Seqs are unaffected. (along with unit test demonstrating this)

As for the name, HVec is ambiguous (can be confused for Haskell HList or in VLSI can be confused for hierarchical) as noted in #844. HetVec is another option as noted in #844 as well but seems strange as a user (what is "Het"?). Another option that seems reasonable and short is MixedVec, which seems preferable.

For now the compromise is to make the class name HeterogeneousVec and add shorter aliases for HetVec and MixedVec. In addition, rocket-chip has been using the longer name HeterogeneousBag without much issue.

Type of change: bug report | feature request

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes

Add a new dynamically indexable sequence like Vec that supports different underlying widths/types.

ucbjrl

Looks pretty good if the possibly stray comment is addressed.

ucbjrl · 2018-07-11T15:12:08Z

src/main/scala/chisel3/util/HeterogeneousVec.scala

+  * arb.io.in(1) <> producer1.io.out
+  * consumer.io.in <> arb.io.out
+  * }}}
+  */


I'm having trouble reconciling the previous comment with the following code.

My bad - just wrote the actual scaladoc for the code.

ucbjrl

lgtm

jackkoenig · 2018-07-10T21:05:48Z

src/test/scala/chiselTests/HeterogeneousVecSpec.scala

+  }
+
+  property("Connecting a HeterogeneousVec and something of different size should report a ChiselException") {
+    a[IllegalArgumentException] should be thrownBy {


This is a tiny nit but you can put a space between the a and the [, you can also use an.

jackkoenig · 2018-07-10T23:38:46Z

src/main/scala/chisel3/util/HeterogeneousVec.scala

+    require(this.length == that.length)
+    for ((a, b) <- this zip that)
+      a := b
+  }


Is this necessary? I think it should already be implemented by Data.:=

Good catch; will fix.

jackkoenig · 2018-07-12T22:16:07Z

src/main/scala/chisel3/util/util.scala

@@ -11,5 +11,6 @@ package object util {
  type ValidIO[+T <: Data] = chisel3.util.Valid[T]
  val ValidIO = chisel3.util.Valid
  val DecoupledIO = chisel3.util.Decoupled
-
+  val MixedVec = chisel3.util.HeterogeneousVec
+  val HetVec = chisel3.util.HeterogeneousVec


I think we should settle on shorter name rather than just providing multiple

Sure. I'm personally partial to MixedVec. I agree with you that HVec is overloaded and can be confusing, and HetVec still requires me as a user to stop and think about what is a "Het".

MixedVec sounds good to me, should we discuss in the developers meeting tomorrow?

Sure, how about we propose MixedVec for now and see if there are any thoughts about it tomorrow, and if not, let's go ahead with MixedVec? It'll just involve me removing the aliases and renaming the class.

Works for me, I think I would just replace all HeterogeneousVec with MixedVec

shunshou · 2018-07-12T22:21:23Z

I think MixedVec is pretty clear :o

…

On Thu, Jul 12, 2018 at 3:19 PM, edwardcwang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/scala/chisel3/util/util.scala <#850 (comment)> : > @@ -11,5 +11,6 @@ package object util { type ValidIO[+T <: Data] = chisel3.util.Valid[T] val ValidIO = chisel3.util.Valid val DecoupledIO = chisel3.util.Decoupled - + val MixedVec = chisel3.util.HeterogeneousVec + val HetVec = chisel3.util.HeterogeneousVec Sure. I'm personally partial to MixedVec. I agree with you that HVec is overloaded and can be confusing, and HetVec still requires me as a user to stop and think about what is a "Het". — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#850 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGTTFonLXf9Q3XgT0BwKF7DLM47K6T-Sks5uF8t1gaJpZM4VG2CS> .

ducky64

I think we should be careful about adding too many base constructs or multiple ways of achieving the same goal (for example, this provides somewhat similar capability to some of the Mux utils). What use cases does this serve, how are the existing solutions deficient, and how is this positioned with regards to the rest of the base Chisel data types?

ducky64 · 2018-07-13T06:48:05Z

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

+            seen += d
+          }
+        case None =>
+          if (!ignoreSeq) {


I understand why this check is here, but is there a cleaner way to implement the bypass than setting a global flag (which seems a bit unclean).

I'm happy to take suggestions, if you have any. This is actually a method that a user can override in their bundle and is read-only, so I don't think this can cause any concurrency issues?

It’s not a global flag right? It’s a per-Bundle flag which is fairly contained.

Sorry, global was bad terminology, I meant throwing flags in the body of the instance as override vals. I don't think this is something we've done elsewhere, and while it's mitigated by clear time-of-use documentation (which is great!), it still feels odd. I don't have any solutions right now, but it would be worth brainstorming alternatives at a future meeting.

Yeah I agree that overriding a function isn't ideal. What if we made it a trait you could mixin instead?

jackkoenig · 2018-07-13T19:32:25Z

src/main/scala/chisel3/util/HeterogeneousVec.scala

+  }
+
+  // Clone the inputs so that we have our own references.
+  val elts: Seq[T] = eltsIn.map(_.cloneTypeFull)


We need to make elts an IndexedSeq

We also might consider making elts private? Since MixedVec extends IndexedSeq it can be used like one so I"m not sure there's any reason to expose the underlying representation as a public API

jackkoenig · 2018-07-13T20:21:07Z

src/main/scala/chisel3/util/HeterogeneousVec.scala

+  * }}}
+  */
+@chiselName
+case class HeterogeneousVec[T <: Data](private val eltsIn: Seq[T]) extends Record with collection.IndexedSeq[T] {


This should not be a case class but probably be final.

case class has lots of implications about equality and immutability that really don't fit with Chisel types

So does that mean we want to disallow the possibility for users to extend from MixedVec?

Generally allowing a class to be extended has a cost if we want to make any changes in the future. I think start with it as final and then if people ask for it to be make extendible we can evaluate it at a later time.

edwardcwang · 2018-07-13T21:00:40Z

@jackkoenig Renaming, removal of aliases, and comments all addressed.

jackkoenig · 2018-07-13T21:03:43Z

src/main/scala/chisel3/util/MixedVec.scala

+      throw new IndexOutOfBoundsException("Collection is empty")
+    }
+
+    MuxLookup(index, elts.head, elts.zipWithIndex.map { case (el, el_index) => el_index.U -> el })


This actually won't work, you can't create a mux with heterogeneous types. This is a big problem but it's not really clear how you can possibly handle dynamically indexing heterogeneous types...

Hmm, would it be possible for you to elaborate on that? I wrote a test for how I would use it from a user's perspective (MixedVecDynamicIndexTester) and it seems to behave/work as expected.

The issue with the test is that it only checks dynamic indexing when all of the elements are UInts, if you try dynamically indexing a MixedVec composed of a UInt and some other Data like SInt or Bundle, you will get an elaboration time exception

Thanks for the catch Jack. I thought about it some more, and came up with a solution that seems to work for heterogeneous elements as long as the user is careful about it, and added a unit test to demonstrate/test it.

Since Chisel's mux already creates a result of the maximum width (e.g. muxing UInt(3.W) and UInt(8.W) -> UInt(8.W)), so we just return a UInt of the maximum width and let the designer handle packing/unpacking/etc. I think this interpretation makes the most sense in light of dynamic indexing as a hardware construction operation.

jackkoenig · 2018-07-13T21:23:36Z

src/main/scala/chisel3/util/MixedVec.scala

+  * }}}
+  */
+@chiselName
+final case class MixedVec[T <: Data](private val eltsIn: Seq[T]) extends Record with collection.IndexedSeq[T] {


I still think it should be a just a class, not a case class. case class implies things about equality that are not true about this type

I see. Would you know if there's a particular reason the rocket-chip implementation uses case class? https://github.com/freechipsproject/rocket-chip/blob/8c6e7456531c4d7b846d0532b2b94805b26c9793/src/main/scala/util/HeterogeneousBag.scala#L9

No idea but it probably shouldn't be

Okay, will do.

jackkoenig · 2018-07-13T21:28:03Z

src/main/scala/chisel3/util/MixedVec.scala

+  * @param eltsIn Element types. Must be Chisel types.
+  *
+  * @example {{{
+  * val v = Wire(HeterogeneousVec(Seq(UInt(8.W), UInt(16.W), UInt(32.W))))


Comments need to be updated

jackkoenig · 2018-07-27T20:16:01Z

src/main/scala/chisel3/util/MixedVec.scala

+    }
+
+    MuxLookup(index, elts.head.asUInt, elts.zipWithIndex.map { case (el, el_index) => el_index.U -> el.asUInt })
+  }


We discussed this in the meeting, but let's see if we can make this return T and dynamically pick the right concrete type with UInt being the default for full Heterogeneous types. We should then have a test for a MixedVec of all UInts, all SInts, all (the same) Bundle, and then a mix. That or we can remove this, merge the PR, and do that later

Hmm, would you have any suggestions on how to do that? I thought about pattern matching to handle the special cases but we can't match on type parameters (i.e. T match { ... } isn't valid).

Originally I tried returning T at one point and the problem occurs, as you mentioned in a previous comment, when heterogeneous types get mixed. It would be okay if I could detect if T isn't all UInt/SInt/same bundle, but I don't see an obvious way to do that.

You can do it with ClassTags: https://www.scala-lang.org/api/2.12.3/scala/reflect/ClassTag.html

I'm not 100% on how to do it but that's the basic approach.

I think it might be worth removing the dynamic index for now and merging MixedVec without it since it's useful for IOs and dynamic indexing can be shimmed with VecInit(myMixedVec)(idx) (I think).

One concern here is that if the MixedVec is composed of 2 different bundles, then their common supertype would be Bundle, and I think we get a ClassCastException if we try to cast a generic a generic UInt to it.

Perhaps we could keep both a UInt-returning version as well as a T-returning version (possibly Option[T]-returning) with different names and documentation? It seems unlikely that we can cover all cases by returning just T, and in those cases it might still be useful to have a mux function, I think.

I'm also struggling to come up with a good and clean solution - perhaps that indicates a deeper problem? Returning a concrete supertype (like Bundle) that carries data / bits would require a major overhaul of behavior, and returning UInt seems special-case and unclean. It might could also be unintuitive ("wait, how am I getting a UInt when this vec of full of Bundle types?!")

I'm trying to think of what the user wants when dynamically indexing such a vec, would using a regular vec with tagged unions (which don't exist yet but are a requested feature) be a cleaner solution?

Anyways, I'm also leaning towards removing this until we can figure out a clean API. It's easier to add features than to remove them, especially if there are alternative (though potentially clunkier) pathways available currently.

Alright, done.

jackkoenig

I'm happy with this. @ducky64 did you have further thoughts about the ignoreSeq stuff?

ducky64

I'm happy with the mixin, though I have some consistency comments on the rest of the API.

ducky64 · 2018-07-31T00:06:26Z

src/main/scala/chisel3/util/MixedVec.scala

+
+import scala.collection.immutable.ListMap
+
+object MixedVecWireInit {


This seems like a lot of words. Why not MixedVecInit? Note that VecInit also internally creates a wire.

Also, thoughts on a varargs version vs. the Seq version? In the comments for VecInit, it appears that we would have preferred the varargs version for consistency with Scala.

Mainly to make room for the introduction of Vec (and associated MixedVec) literals (Vec literals #849) if it ever happens. VecInit can give a bit of an illusion of a Vec literal when it just creates a wire. I'm happy to rename this if you feel strongly about it.

I wouldn't be opposed to a varargs version in addition to a Seq version, though I would prefer it if the Seq version were not completely removed, since sometimes we do want to programmatically create a Seq to use as the type and not have to worry about :_* etc.

I think that whatever we do, we at least need to keep Vec and MixedVec consistent, and ideally would also keep them consistent with their closest Scala analogues. I'm not sure how we want to do Vec literals in the future, though we might want to keep that consistent with Bundle literals (so Vec(...).Lit, or even Vec.Lit). Or maybe VecLit. But the alternative solution here would be to deprecate VecInit and have it be VecWireInit (or just VecWire?), but we'd end up waiting at least one deprecation cycle (if ever) before we could introduce a different meaning for VecInit. So I highly doubt that's a practical pathway.

The argument for a varargs version is that it's more consistent with the Scala collections library, where you do Seq(elems*), or elems.toSeq. So if we didn't already have VecInit(Seq), a consistent solution might be mySeq.toVec (or mySeq.toVecWire). The idea behind consistency is so that programmers can move between Scala and Chisel without having to think about a different set of interfaces.

I'm happy to rename to MixedVecInit and will do so tomorrow.

So in conclusion can we keep the Seq version as well, since we don't have .toVec or things like that?

I think it's worth a discussion about whether we want .toVec (or similar), since I think there can be issues with a constructor that takes both a varargs and a Seq.

I agree - since (I think) .toVec would be a further API addition, perhaps we can merge this PR first (after MixedVecInit of course) and propose/discuss .toVec in a future issue/PR?

ducky64 · 2018-07-31T00:11:14Z

src/main/scala/chisel3/util/MixedVec.scala

+    * @param eltsIn Element types. Must be Chisel types.
+    * @return MixedVec with the given types.
+    */
+  def apply[T <: Data](eltsIn: Seq[T]): MixedVec[T] = new MixedVec(eltsIn)


ditto as above, should this be varargs?

ducky64 · 2018-07-31T00:12:47Z

src/main/scala/chisel3/util/MixedVec.scala

+@chiselName
+final class MixedVec[T <: Data](private val eltsIn: Seq[T]) extends Record with collection.IndexedSeq[T] {
+  // We want to create MixedVec only with Chisel types.
+  if (compileOptions.declaredTypeMustBeUnbound) {


Doesn't need to happen for this PR, but I'm in favor of getting rid of compile options and just having a catchall compatibility flag. At one point we might have thought about letting people pick and choose individual options, but I don't think that's the philosophy currently, and I'd have all new features be strict, since there's no old code to support.

ducky64 · 2018-07-31T00:16:34Z

src/main/scala/chisel3/util/MixedVec.scala

+    *
+    * @note the lengths of this and that must match
+    */
+  def :=(that: Seq[T]): Unit = {


This also seems a bit special case-y, though I think the right thing to do is to match behavior with Vec for consistency. I don't think this is supported in Vecs yet (though there have been requests?), and how to do it right and generally is a discussion we should have.

edwardcwang · 2018-08-03T19:40:31Z

TODO from meeting: Edward to add varargs API as well

edwardcwang · 2018-08-22T01:38:02Z

Varargs API added and tested; will merge this PR as soon as the status checks pass.

…upported

We can sometimes shim with other workarounds like VecInit or manually creating a mux

edwardcwang added feature request Feature New feature, will be included in release notes Request For Comment labels Jul 8, 2018

edwardcwang requested review from ucbjrl and jackkoenig July 8, 2018 19:37

edwardcwang mentioned this pull request Jul 8, 2018

Cannot use Seq in Bundle construction? #844

Closed

ucbjrl reviewed Jul 11, 2018

View reviewed changes

edwardcwang force-pushed the hvec branch from 4b9ca6b to 08d864b Compare July 11, 2018 17:34

ucbjrl approved these changes Jul 12, 2018

View reviewed changes

edwardcwang force-pushed the hvec branch from 08d864b to 6c9e124 Compare July 12, 2018 22:11

jackkoenig requested changes Jul 12, 2018

View reviewed changes

edwardcwang changed the title ~~Heterogeneous Vec~~ Heterogeneous Vec (MixedVec) Jul 12, 2018

ducky64 reviewed Jul 13, 2018

View reviewed changes

jackkoenig reviewed Jul 13, 2018

View reviewed changes

edwardcwang force-pushed the hvec branch from 4e084aa to 27a4682 Compare July 13, 2018 20:59

jackkoenig reviewed Jul 13, 2018

View reviewed changes

edwardcwang force-pushed the hvec branch from 27a4682 to 6de6733 Compare July 13, 2018 23:04

edwardcwang force-pushed the hvec branch from 299577d to 010a49b Compare July 27, 2018 15:06

jackkoenig requested changes Jul 27, 2018

View reviewed changes

jackkoenig approved these changes Jul 30, 2018

View reviewed changes

ducky64 reviewed Jul 31, 2018

View reviewed changes

edwardcwang force-pushed the hvec branch from e3c2f42 to f827b9a Compare August 22, 2018 01:36

edwardcwang force-pushed the hvec branch from f827b9a to 429ccf0 Compare August 22, 2018 01:56

edwardcwang force-pushed the hvec branch 3 times, most recently from 43dd03d to bedf5eb Compare August 22, 2018 07:01

edwardcwang added 9 commits August 22, 2018 11:29

MixedVec implementation

d6088d7

Remove redundant := method

5d90029

Warn user that using Seq for hardware construction in Bundle is not s…

2c423a6

…upported

MixedVec: clarify dynamic indexing of heterogeneous elements

16d37cb

Use a mix-in to override Seq error

7fd591e

Remove dynamic indexing for now

c7ace09

We can sometimes shim with other workarounds like VecInit or manually creating a mux

Make MixedVec wire init consistent with VecInit

eba33a6

Implement varargs MixedVec API

cb92772

Update class name in error message

57774f0

edwardcwang force-pushed the hvec branch from 48b12f0 to 57774f0 Compare August 22, 2018 18:29

edwardcwang merged commit b72f5b8 into master Aug 22, 2018

edwardcwang deleted the hvec branch August 22, 2018 18:55

ucbjrl added this to the 3.1.3 milestone Sep 7, 2018


		import scala.collection.immutable.ListMap

		object MixedVecWireInit {

Heterogeneous Vec (MixedVec) #850

Heterogeneous Vec (MixedVec) #850

Conversation

edwardcwang commented Jul 8, 2018

ucbjrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ucbjrl 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

edwardcwang Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shunshou commented Jul 12, 2018 via email

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

edwardcwang commented Jul 13, 2018

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

jackkoenig left a comment

Choose a reason for hiding this comment

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardcwang Jul 31, 2018 • edited Loading

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

edwardcwang Jul 31, 2018 • edited Loading

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

edwardcwang commented Aug 3, 2018

edwardcwang commented Aug 22, 2018

edwardcwang Jul 12, 2018 •

edited

Loading

edwardcwang Jul 31, 2018 •

edited

Loading

edwardcwang Jul 31, 2018 •

edited

Loading