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

Changed Vec cloneType #669

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Changed Vec cloneType #669

wants to merge 1 commit into from

Conversation

shunshou
Copy link
Member

@shunshou shunshou commented Mar 6, 2016

Each element's type is properly cloned. See #392

Each element's type is properly cloned
@shunshou
Copy link
Member Author

shunshou commented Mar 7, 2016

I guess there should really be a distinction between different types of Vecs. You can't dynamically read from a Vec of UInts with different widths (addres using UInt), since it uses head's width. But I think Vec's of UInts with different widths should be supported... Unfortunately, I don't think an IndexedSeq of UInts of different widths is supported by Bundle for IO... (since Bundle needs Data?), so I instead made a parameterizable Vec of UInts with different widths for IO and wanted to clone their type into the top module, but originally, it'd just clone the type with the first element's required # of bits.

@aswaterman
Copy link
Member

Note, in chisel3, Vecs are homogeneous (or at least are supposed to be), so relying on any other Vec behavior is not forward compatible.

@shunshou
Copy link
Member Author

shunshou commented Mar 7, 2016

Will you have an alternative to Vec for non-homogeneous groupings that don't require dynamic addressing? Specifically for IO... Optionable IO is one thing, but I'd also like to have a parameterizable set of IO with varying widths + length, and without Vec, there's no way to do something like that. There should be some kind of ChiselIndexedSeq that can be placed in an IO Bundle.

@aswaterman
Copy link
Member

That doesn't exist, but I can see the use case. They'd basically function as bundles where the fields are numbered rather than named.

@shunshou
Copy link
Member Author

shunshou commented Mar 7, 2016

Sure basically be able to parameterize the bundle length or dynamically add
a bunch of similar elements to a bundle. If Chisel3 could eventually
support something like that, it would be great for churning out generators.

On Sunday, March 6, 2016, Andrew Waterman [email protected] wrote:

That doesn't exist, but I can see the use case. They'd basically function
as bundles where the fields are numbered rather than named.


Reply to this email directly or view it on GitHub
#669 (comment).

@sdtwigg
Copy link
Contributor

sdtwigg commented Mar 7, 2016

A better approach for those desires again seems to hint at allowing you to override Bundle reflection to act in special cases. We already saw one case of this desire, being able to have optional fields (i.e. looking into scala option). Now, this includes variable numbers of non-homogeneous outputs (e.g. looking into scala sequences since scala vec has homogeneity implications).

@shunshou
Copy link
Member Author

shunshou commented Mar 7, 2016

Ok I'd be down for something like that too.

@ucbjrl
Copy link
Contributor

ucbjrl commented Mar 7, 2016

Should we wait for the Chisel3 solution and back port it to Chisel2?

@sdtwigg
Copy link
Contributor

sdtwigg commented Mar 7, 2016

Upon examination, I don't think this change will address scenarios from #392 (which I imagine is similar to Angie's issues?) due to https://github.com/shunshou/chisel/blob/master/src/main/scala/Vec.scala#L134 . Thus, you are likely to still see truncation. Sadly, due to that, I'm not sure if this change has much of an effect on the underlying issue except....

This change isn't strictly backwards compatible because it may require instantiation of clone methods where previously they were not needed. (The old clone method would reuse tabulate to construct members whereas the new one requires members to have clone method.)

@sdtwigg
Copy link
Contributor

sdtwigg commented Mar 7, 2016

The Chisel3 approach will (necessarily, due to FIRRTL incompatibility) removes the following way of updating registers:

val reg0 = Reg(init=UInt(0, width=4))
val wire1 = UInt(width=4)
wire1 := UInt(0)
Vec(reg0, wire1)(io.select) := UInt(1)

(Removal of this ability isn't necessarily a bad thing; however, this still constitutes another perhaps unforeseen backwards incompatibility. What is a bad this is that the chisel3 version won't yield an error or warning: Just silently be wrong as the reg0 update gets lost.)

@shunshou
Copy link
Member Author

shunshou commented Mar 8, 2016

I assume it'll take a long time for a proper Chisel3 solution to be put in place. I don't think changing the cloneType of Vec will harm anything... since if the elements are really meant to be homogeneous in Chisel3, the cloneType that I have will still be correct (all elements will have the width of head). As far as I understand it, I think all subclasses of Chisel Data currently have implemented cloneType. This fix is really only to have cloneType perform the proper clone for non-homogeneous Vecs, which I still would like to index statically via Scala Ints for generator purposes. Ofc the long-term Chisel3 solution would be to support IndexedSeq in bundle or have a VecNonHomogeneous that would allow elements of different types (or rather, I care about same types, different widths -- it might be bad to support, say, Dbl and UInt in the same VecNonHomogeneous) that only need to be indexed via Scala Ints (instead of dynamically via Chisel UInts).

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.

4 participants