chipsalliance / chisel

Chisel: A Modern Hardware Design Language
https://www.chisel-lang.org/
Apache License 2.0
3.92k stars 588 forks source link

Add Bundle.Init (analogous to VecInit / Bundle.Lit) #2435

Open jackkoenig opened 2 years ago

jackkoenig commented 2 years ago

I sketched out a version of this here: https://scastie.scala-lang.org/D0948BD1SgeZEqLGkVBoJg

Ideally this (and VecInit for that matter) would: 1) compose 2) utilize DataView -- (we probably cannot change VecInit but views here so that you can connect to the underlying fields would be nice)

Regardless, this sketch is useful on its own even without the more robust solution

Type of issue: bug report | feature request | documentation | other enhancement

Impact: API addition (no impact on existing code)

Development Phase: proposal

What is the use case for changing the behavior?

Useful API for users

ekiwi commented 2 years ago

Bundle expressions is a feature that I recently would have liked to use. Do you know how this is different from Bundle literals though?

jackkoenig commented 2 years ago

Do you know how this is different from Bundle literals though?

Bundle literals require that everything field is initialized to a literal whereas Bundle.Init would allow anything. Do the names of the APIs need to be different? Unclear but they are very similar.

ekiwi commented 2 years ago

Do the names of the APIs need to be different? Unclear but they are very similar.

I think Bundle.Init seems like a more consistent API and it would be cool if it could replace Bundle literal as long as you make sure to assign all elements.

poddar92 commented 1 year ago

@jackkoenig anyone working on this issue? is it okay if I pick it up? I would need some info on where we want to add this API and how to integrate it with the whole system.

jackkoenig commented 1 year ago

Hi @poddar92 and welcome! Feel free to pick this one up, the sketch in the Scastie should serve as a good starting point. You should also feel welcome to join Chisel dev meetings which are on Mondays at 11 am Pacific Time (https://github.com/chipsalliance/chisel3#chisel-dev-meeting).

The basic idea is to take that sketch and add the implicit class inside of package object chisel3: https://github.com/chipsalliance/chisel3/blob/master/core/src/main/scala/chisel3/package.scala

You should then add some tests that show that it works similar to the Vec literal tests, a good test to start with is one that does something like this test: https://github.com/chipsalliance/chisel3/blob/bc55ed16bdcc7fcc08d29f8bcbdf89cdefced96a/src/test/scala/chiselTests/VecLiteralSpec.scala#L79

Please add it to a new file though called BundleInitSpec in the same directory as that VecLiteralSpec.

That would be a good start for a draft PR and we can discuss more on the PR.

poddar92 commented 1 year ago

@jackkoenig do we want to recursively go though the bundle and then initialize them? what if one of the elements is a Vec? Should we follow how Bundle.Lit does it? Also; i see the below error with above sketch:

[error] /Users/radhikapoddar/chisel3/core/src/main/scala/chisel3/package.scala:395:24: macro implementation not found: materialize
[error] (the most common reason for that is that you cannot use macro implementations in the same compilation run that defines them)
[error]         val init = Wire(bun)

is it because we are trying to assign the bundle directly without defining its type in Wire first? is this related to sourceInfo?