Implement DataViews for Seq and Tuple

[jackkoenig]

Fixes #864 (at long last!)

Implemented in new chisel3.experimental.conversions package. Includes:

• Implementations of DataProduct
• Implementations of DataView from Seq to Vec and Tuple to HWTuple
• Implicit conversions from Seq to Vec and Tuple to HWTuple

Examples are buried in the tests, but this makes it possible to write stuff like:

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val sel = IO(Input(Bool()))
  val y, z = IO(Output(UInt(8.W)))
  (y, z) := Mux(sel, (a, b), (c, d))
}

and

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val out = IO(Output(Vec(4, UInt(8.W)))
  out := Seq(a, b, c, d)
}

I still haven't implemented more than just Tuple2, I intend to do several, maybe up to like Tuple7 or something.

Opening this before finishing it because I'd like feedback on the approach.

Questions

Do we like the implicit conversions?

They feel natural but I know folks are generally skeptical of implicit conversions.
An alternative is to provide .asData as an extension method, something like:

implicit class CanConvertToData[A : DataProduct](a: A) {
  def asData[B <: Data](implicit dataview: DataView[A, B]): B = a.viewAs[B]
}

This would change the above examples slightly:

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val sel = IO(Input(Bool()))
  val y, z = IO(Output(UInt(8.W)))
  (y, z).asData := Mux(sel, (a, b).asData, (c, d).asData)
}

and

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val out = IO(Output(Vec(4, UInt(8.W)))
  out := Seq(a, b, c, d).asData
}

You can see the total impact of this change on existing tests in this diff: https://gist.github.com/jackkoenig/ae5d20b18b4df2871aa174f0a38551a0

There is at least one other example where the implicit conversion doesn't work great but .asData does:

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val w, x, y, z = IO(Output(UInt(8.W)))

  // A little annoying that we need the type annotation on VecInit to get the implicit conversion to work
  // Note that one can just use the Seq on the RHS so there is an alternative (may lack discoverability)
  // We could also overload `VecInit` instead of relying on the implicit conversion
  Seq((w, x), (y, z)) := VecInit[HWTuple2[UInt, UInt]]((a, b), (c, d))
}

vs.

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val w, x, y, z = IO(Output(UInt(8.W)))

  Seq((w, x), (y, z)).asData := VecInit((a, b).asData, (c, d).asData)
}

As alluded to in the comment though, VecInit actually becomes obsolete if we have the implicit conversion, so perhaps this example isn't particularly useful.

Do we like the package?

import chisel3.experimental.conversions._

They could live directly chisel3.experimental.dataview, or it could be a subpackage of dataview. I think the DataProducts and the DataViews should live together but it's not clear if the implicit conversions (assuming we keep them) should live in the same package.

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

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (Bug fix: 3.3.x, [small] API extension: 3.4.x, API modification or big change: 3.5.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[jackkoenig]

I'm curious what users think @mwachs5, @aswaterman, @carlosedp, @ingallsj, @schoeberl, @sequencer, @hcook

[mwachs5]

Do we like the implicit conversions?

One of my biggest user issues with the implicit conversions is its hard to understand why my code doesn't work when your code does. If there is a function it's easy to figure out in IDE how to include the function, but if an implicit conversion is missing, it's hard to discover.

What is the advantage of the example:

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val out = IO(Output(Vec(4, UInt(8.W)))
  out := Seq(a, b, c, d).asData
}

Is it just saving the few characters of writing VecInit, or does this change the output in some way? If the former this seems like a pattern that would again just confuse people.

The Mux example is much more compelling IMHO, can you shoudl what that woudl look like if we forced the .toData thing?

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val sel = IO(Input(Bool()))
  val y, z = IO(Output(UInt(8.W)))
  (y, z) := Mux(sel, (a, b), (c, d))
}

[jackkoenig]

Do we like the implicit conversions?

One of my biggest user issues with the implicit conversions is its hard to understand why my code doesn't work when your code does. If there is a function it's easy to figure out in IDE how to include the function, but if an implicit conversion is missing, it's hard to discover.

Yeah it's a fair concern.

I should revise my proposal a bit, I think we should definitely have .asData whether or not we have the implicit conversions [1]. Then, if something isn't working for a user, they can always use the explicit .asData. That doesn't resolve your concern, but maybe it helps.

What is the advantage of the example:

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val out = IO(Output(Vec(4, UInt(8.W)))
  out := Seq(a, b, c, d).asData
}

Is it just saving the few characters of writing VecInit, or does this change the output in some way? If the former this seems like a pattern that would again just confuse people.

In this case, it's true that VecInit(a, b, c, d) would accomplish the same thing. But if we flipped the directions of everything, DataView allows connecting to the underlying elements:

// This would work
Seq(a, b, c, d).asData := out
// This would just connect to an intermediate Wire and thus be dropped
VecInit(a, b, c, d) := out // bad! don't do this!

The Mux example is much more compelling IMHO, can you shoudl what that woudl look like if we forced the .toData thing?

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val sel = IO(Input(Bool()))
  val y, z = IO(Output(UInt(8.W)))
  (y, z) := Mux(sel, (a, b), (c, d))
}

I had it above but it would look like this:

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val sel = IO(Input(Bool()))
  val y, z = IO(Output(UInt(8.W)))
  (y, z).asData := Mux(sel, (a, b).asData, (c, d).asData)
}

This is actually the case that made me add the implicit conversion in the first place. The nice thing about the implicit conversion is that any place a user writes code that accepts Data or T <: Data, it will just work if passed Seqs or Tuples of Data. Without the implicit conversion, people can of course manually cast each thing, and method writers could of course add their own version of the method that will do the conversion. For example, in the case of Mux:

object Mux {
  // Simplified since we also have SourceInfo and CompileOptions implicits here, and a do_ macro
  def apply[T <: Data](cond: Bool, con: T, alt: T): T = ???
}

One could add the following additional methods so that it will do the conversion rather than requiring an implicit:

  def apply[A : DataProduct, T <: Data](cond: Bool, con: T, alt: T)(implicit ev: DataView[A, T]) : T =
    apply(cond, con.viewAs[T], alt.viewAs[T])

I think this is fine to do inside of Chisel itself, but I'm not super keen to suggest users do the same. Thus in practice, most user-defined APIs would require explicit .asData for non-Data-but-viewable-as-Data arguments.

[jackkoenig]

It's occurred to me that .asData is identical to just .viewAs except I'm stylistically leaving off the explicit [T] which works so long as T is unambiguous (as it normally is). Makes me question whether .viewAs is the right method name because the following also works (but feels very weird compared to using .asData):

class MyModule extends Module {
  val a, b, c, d = IO(Input(UInt(8.W)))
  val sel = IO(Input(Bool()))
  val y, z = IO(Output(UInt(8.W)))
  (y, z).viewAs := Mux(sel, (a, b).viewAs, (c, d).viewAs)
}

[mwachs5]

This is actually the case that made me add the implicit conversion in the first place.

I agree I really like the implicit conversion in the Mux case

I think you should include your Seq/Vec example from below it is also nice and compelling.

// This would work
Seq(a, b, c, d).asData := out
// This would just connect to an intermediate Wire and thus be dropped
VecInit(a, b, c, d) := out // bad! don't do this!

[jackkoenig]

Alright, after further thought, I'm going to move all of the DataProduct and DataView implementations into the companion objects of DataProduct and DataView so that they are available in the implicit scope (ie. without a specific import). The implicit conversions will remain in package chisel3.experimental.conversions (requiring the specific import) so that we can evaluate how well they work. My intuition is that they are a good thing (for these particular cases, definitely not for all cases), but we should see how it works in practice.

[sequencer]

I checkout and tried this feature, I really like this conversation! I think implicitly conversion will be more friendly to most of newbies to Chisel.
Maybe a documentation(just copy&paste from PR description) for end user will be much better.

One of my biggest user issues with the implicit conversions is its hard to understand why my code doesn't work when your code does. If there is a function it's easy to figure out in IDE how to include the function, but if an implicit conversion is missing, it's hard to discover.

By default, I forcing users from our lab using IntelliJ IDEA, and think they are supported by Jetbrains by default(I think it really can teach them Scala, haha).
Without implicit conversion, users must understand why they should asData, which is a tough job for me to explain the magic of dataview APIs.
So as a trade-off, I think a implicit conversion should be documented and optional import by user(I only need to ask them: just import this for better user experience).

[carlosedp]

I'm all for the implicit conversions from a user perspective.
Having the casts all over will make things more complex where one will ask things like "should .asData go here... or there" making debug more confusing.

Other than that, I like the implementation making very straightforward to assign tuples/hwtuples too.

[jackkoenig]

Okay I think this is ready. I've made a few minor changes but I think there's wide agreement on the approach here.
DataProducts and DataViews for Seq and Tuple2-Tuple10 are now defined in the companion objects for DataProduct and DataView. This means that users who import chisel3.experimental.dataview._ will have access to .viewAs for Seqs and Tuples with no additional import. The implicit conversions are in a new package, chisel3.experiemental.conversions such that if a user imports from that package, they will get the nice syntax for tuples and the like.

Interestingly, while the implicit conversions seem to work fantastically for Tuples, they seem to work less well for Seq, as evidenced by this test:

class MyModule extends Module {
   val a, b, c, d = IO(Input(UInt(8.W)))
   val sel = IO(Input(Bool()))
   val y, z = IO(Output(UInt(8.W)))
   // Unclear why the implicit conversion does not work in this case for Seq
   // That being said, it's easy enough to cast via `.viewAs` with or without
   Seq(y, z) := Mux(sel, Seq(a, b).viewAs, Seq(c, d).viewAs[Vec[UInt]])
 }

I do not know why this is, but I wrote this test to capture something interesting. The implicit conversion does work for the LHS, ie. := is only working here because of the implicit conversion, yet for some reason it does not work for the arguments to Mux. I then show that one can use .viewAs without explicit type parameters if there's only 1 DataView in scope (which is the normal case). You can of course, do the normally recommended thing and give type parameters.

I'm wondering if we should add .asData that is basically an alias for .viewAs but would return self if called on a Data and otherwise would do the same thing as .viewAs. Any thoughts? I do see that as a separate PR.

[carlosedp]

@jackkoenig it would be great to grab those nice examples and put them into the cookbook

[jackkoenig]

@jackkoenig it would be great to grab those nice examples and put them into the cookbook

Agreed, will do in follow on PR.