Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DataView #1955

Merged
merged 3 commits into from Aug 13, 2021
Merged

Add DataView #1955

merged 3 commits into from Aug 13, 2021

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Jun 8, 2021

Introducing DataView 🙂

The documentation is complete. Here are the outputs of mdoc:

This is a large addition, but keep in mind that 1590 lines of change are documentation and tests (~70%)

TODO

  • MDoc
  • Finish ScalaDoc
  • Fill in link to docs for error messages
  • Implement nested views (see the "ignore" test)
  • Provide DataProduct.contains
  • Implement PartialDataView
  • Implement .toTarget on views
  • Change to .viewAs[T] instead of .viewAs(new T)
  • Support naming, eg. val foo = (new MyHW).viewAs(new OtherType), currently the hardware gets a temporary name
  • Handle differing widths

Follow on PR work

  • Handle dynamic indexing of Vec views (currently left as an error)
  • Built-in DataViews like HWTuple -- done in Implement DataViews for Seq and Tuple #2277
  • Implement FlatIO with DataView -- sketch: https://scastie.scala-lang.org/DOVFDr0XQNi4pWre8X7IzQ
  • Automatically derive andThen DataViews. This works in Scala 2.13 but leads to diverging implicit expansion in 2.12 (because of identityView)
  • Implement .readOnlyView using views
  • Maybe change to DataProduct.lookup (to get a String name for fields that may have a bug)
  • Subword stuff (eg. being able to view an Aggregate as a UInt or vice versa)
  • Subword assignment (make .apply on Bits return views, then we could assign to them)
  • Support for literals (should go along with support in .asTypeOf)
  • Handle default values, what if a field of a Target is a literal for a particular instance?
  • Refactor DataView Binding to hold original mapping (the tuples, not the function) would enable cloneViewToContext to share implementation with .viewAs
  • Refactor CloneModuleAsRecord to return a View instead of a special private Record

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

  • documentation
  • code refactoring
  • new feature/API

API Impact

Important API Expansion (not necessarily "large" since it is focused, but it is a pretty big feature)

Backend Code Generation Impact

No impact

Desired Merge Strategy

  • Squash

Release Notes

DataView is a mechanism for "viewing" Scala objects as a subtype of
Data. Often, this is useful for viewing one subtype of Data, as
another. One can think about a DataView as a cross between a
customizable cast and an untagged union.

A DataView has a Target type T, and a View type V. DataView requires
that an implementation of DataProduct is available for Target types.
DataProduct is a type class that provides a way to iterate on Data
children of objects of implementing types.

If a DataView is provided for a type T to a type V, then the function
.viewAs[V] (of type T => V) is available. The object (of type T) returned
by .viewAs is called a "View" and can be used as both an rvalue and an
lvalue. Unlike when using an .asTypeOf cast, connecting to a "View" will
connect to the associated field or fields of the underlying Target.

DataView also enables .viewAsSupertype which is available for viewing
Bundles as a parent Bundle type. It is similar to .viewAs but requires
a prototype object of the Target type which will be cloned in order to
create the returned View. .viewAsSupertype maps between the
corresponding fields of the parent and child Bundle types.

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?

@edwardcwang
Copy link
Contributor

No PR description or Scaladocs?

@jackkoenig
Copy link
Contributor Author

No PR description or Scaladocs?

The majority of public APIs (but not all) have ScalaDoc, and description is coming. This is a draft PR for a reason 🙂

@jackkoenig jackkoenig added this to the 3.5.0 milestone Jun 8, 2021
@edwardcwang
Copy link
Contributor

Whoops right this is a draft PR.

@jackkoenig
Copy link
Contributor Author

Whoops right this is a draft PR.

I feel like the Github UI could make it a little clearer, but oh well. Take a look at the tests if you're interested in what this does and why it's useful (or wait till I've written up some docs hah).

@jackkoenig
Copy link
Contributor Author

jackkoenig commented Jun 15, 2021

After discussion with @azidar, we need to add a DataProduct.contains. The dataIterator API is important for total DataViews, but would be very slow for merely checking things in PartialDataViews where the target type is a module. For most DataProducts (and perhaps by default), .contains is easily implementable from dataIterator. For DataProduct[BaseModule], we can implement .contains cleverly by seeing if the .id for the argument is greater than the id of the Module itself and less than whatever the final id is in the module (this could be set at the end of the Module.apply wrapper).

@jackkoenig
Copy link
Contributor Author

For anyone following along at home, this is nearing completion and I have "published" the initial version of the docs (see the links in the original PR comment).

@kammoh
Copy link
Contributor

kammoh commented Jul 19, 2021

This is very smart solution to what I believe to be a problem during integration Chisel-based components.
Great job @jackkoenig!

@jackkoenig
Copy link
Contributor Author

Note that the API has changed a little bit, I have updated the docs with the changes.

@jackkoenig jackkoenig marked this pull request as ready for review August 3, 2021 22:38
@jackkoenig
Copy link
Contributor Author

This is done and ready for merging. There are some follow on things to enhance this and I'm sure there are bugs but it'd be nice to get this on 3.5-SNAPSHOT so people can try it out!

@jackkoenig jackkoenig requested a review from azidar August 3, 2021 23:04
@jackkoenig jackkoenig force-pushed the dataview branch 2 times, most recently from 68d20ef to 69fcec1 Compare August 5, 2021 20:44
Copy link
Contributor

@azidar azidar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor changes.

A+, definitely going to be an amazing feature, and you knocked it out of the park with the tests and documentation, as well as the quality of the implementation. Excited to see this in the wild!

core/src/main/scala/chisel3/Data.scala Show resolved Hide resolved
core/src/main/scala/chisel3/Module.scala Show resolved Hide resolved
core/src/main/scala/chisel3/Module.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/RawModule.scala Show resolved Hide resolved
core/src/main/scala/chisel3/RawModule.scala Outdated Show resolved Hide resolved
core/src/main/scala/chisel3/internal/SourceInfo.scala Outdated Show resolved Hide resolved

object Foo {
implicit def view[T <: Data]: DataView[Foo[T], Bar[T]] = {
DataView(f => new Bar(f.foo.cloneType), _.foo -> _.bar)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gosh darn it, this is why I want Field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear if Field would actually fix this because Field would require that something is an unbound type and this would fail that without the .cloneType. I admit I don't have a good solution here but I agree this is a wart.

docs/src/cookbooks/dataview.md Show resolved Hide resolved
DataView is a mechanism for "viewing" Scala objects as a subtype of
`Data`. Often, this is useful for viewing one subtype of `Data`, as
another. One can think about a DataView as a cross between a
customizable cast and an untagged union.

A DataView has a Target type `T`, and a View type `V`. DataView requires
that an implementation of `DataProduct` is available for Target types.
DataProduct is a type class that provides a way to iterate on `Data`
children of objects of implementing types.

If a DataView is provided for a type T to a type V, then the function
.viewAs[V] (of type T => V) is available. The object (of type T) returned
by .viewAs is called a "View" and can be used as both an rvalue and an
lvalue. Unlike when using an .asTypeOf cast, connecting to a "View" will
connect to the associated field or fields of the underlying Target.

DataView also enables .viewAsSupertype which is available for viewing
Bundles as a parent Bundle type. It is similar to .viewAs but requires
a prototype object of the Target type which will be cloned in order to
create the returned View. .viewAsSupertype maps between the
corresponding fields of the parent and child Bundle types.
It does not work in Scala 2.12 due to diverging implicit expansion with
identityView. It *does* work in Scala 2.13.
@jackkoenig jackkoenig merged commit 1ceb974 into master Aug 13, 2021
@jackkoenig jackkoenig deleted the dataview branch August 13, 2021 00:04
@aswaterman
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants