ucb-bar / chisel2-deprecated

chisel.eecs.berkeley.edu
387 stars 89 forks source link

Changed Vec cloneType #669

Open shunshou opened 8 years ago

shunshou commented 8 years ago

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

shunshou commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 notifications@github.com 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 https://github.com/ucb-bar/chisel/pull/669#issuecomment-193091134.

sdtwigg commented 8 years ago

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 commented 8 years ago

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

ucbjrl commented 8 years ago

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

sdtwigg commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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