chisel icon indicating copy to clipboard operation
chisel copied to clipboard

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

Open jackkoenig opened this issue 2 years ago • 3 comments

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

jackkoenig avatar Mar 07 '22 21:03 jackkoenig

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

ekiwi avatar Mar 07 '22 21:03 ekiwi

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.

jackkoenig avatar Mar 07 '22 21:03 jackkoenig

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.

ekiwi avatar Mar 07 '22 21:03 ekiwi

@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.

poddar92 avatar Feb 07 '23 19:02 poddar92

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.

jackkoenig avatar Feb 08 '23 00:02 jackkoenig

@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?

poddar92 avatar Feb 15 '23 20:02 poddar92