chipsalliance / chisel

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

Vec of size 0 not supported #188

Closed ccelio closed 8 years ago

ccelio commented 8 years ago

I have been told that Vec's of size 0 is not supported. I would like for them to be supported.

I have a pipeline of a parameterized size num_stages:

val r_valids = Reg(init = Vec.fill(num_stages) { Bool(false) }) 

It may even be of size zero, in which the data passes through instantly with no registering.

ducky64 commented 8 years ago

How do you plan on using this? When it's zero size, you would have zero width wire (I think work is in progress to support that), but how would you get the combinational passthrough behavior? Presumably, if you tried to do anything with the Vec that's not a collective operation, it would just give you an error.

At a glance, I don't see anything that would prevent having a size zero Vec; the collective operations all seem to do approximately sane things on an empty Vec, and indexing into it hopefully should just error out.

Since you want to see both combinational and registered behavior, might it make more sense to define your own helper function / class, like makePipelineStage(numStages: Int) which internally can generate either a Wire or Reg(Vec)?

ccelio commented 8 years ago

It looks like (at least in this case) I can code my way around this - I was already guarding accesses to the r_valids if num_stages==0, but it makes code a bit uglier to move the instantiations around.

I'd make this low priority, but still something I'd like to see.

ducky64 commented 8 years ago

I'm still not sure how you plan on making zero width Vecs do what you want to do (combinational passthrough) unless Reg does something super unintuitive on having a zero width element passed in or you add in special case code (which defeats the purpose of this in the first place, and you should probably just write your custom makePipelineStage(numStages: Int) or delay(numCycles: Int) method that internally instantiates either a Reg or Wire).

ucbjrl commented 8 years ago

At least one other design, (LBNL's OpenSoC) makes similar assumptions (constructing a Vec with zero elements is legal).

On May 12, 2016, at 4:16 PM, Christopher Celio notifications@github.com wrote:

I have been told that Vec's of size 0 is not supported. I would like for them to be supported.

I have a pipeline of a parameterized size num_stages:

val r_valids = Reg(init = Vec.fill(num_stages) { Bool(false) }) It may even be of size zero, in which the data passes through instantly with no registering.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub

ducky64 commented 8 years ago

Can you link to their code (if it's open source) or post a snippet showing how it's used?

aswaterman commented 8 years ago

Vec(0) should be supported to avoid needless special casing in generators. It's also presently used in some cases to conditionalize I/O generation. (You might say that would better handled with Option or something, but not supporting Vec(0) is certainly a regression.)

On Thu, May 12, 2016 at 4:52 PM, Richard Lin notifications@github.com wrote:

I'm still not sure how you plan on making zero width Vecs do what you want to do (combinational passthrough) unless Reg does something super unintuitive on having a zero width element passed in or you add in special case code (which defeats the purpose of this in the first place, and you should probably just write your custom makePipelineStage(numStages: Int) or delay(numCycles: Int) method that internally instantiates either a Reg or Wire).

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/ucb-bar/chisel3/issues/188#issuecomment-218917896

ducky64 commented 8 years ago

I think it would be nice to see some example code, but having Vec(0) be part of an interface definition could make sense, essentially equivalent to Option(None) but more uniform.

zhemao commented 8 years ago

Is this still an issue for anyone? Vec of size 0 is supported now and we use it extensively in RocketChip's top-level.

terpstra commented 8 years ago

Ups. I clicked 'close' meaning to close the window. ;-) Probably this issue should be closed anyway.

seldridge commented 8 years ago

FYI, it looks like the (to be deprecated?) Vec.fill approach doesn't like vectors of size zero. The following craps out:

val din = Vec.fill(numReadWritePorts)(UInt(OUTPUT, width = dataWidth))

With the following (truncated) error:

...
Caused by: java.lang.IllegalArgumentException: requirement failed
    at scala.Predef$.require(Predef.scala:207)
    at chisel3.core.Vec$.do_apply(Aggregate.scala:50)
    at chisel3.core.Vec$.do_fill(Aggregate.scala:95)
...

Due to: https://github.com/ucb-bar/chisel3/blob/master/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala#L50

The supported Vec(gen: T, n: Int) works fine, however:

val din = Vec(UInt(OUTPUT, width = dataWidth), numReadWritePorts)
zhemao commented 8 years ago

No, Vec.fill is supported going forward, but Vec(gen: T, n: Int) is deprecated.

I think the issue is that you can declare vectors of size 0, but you can't try to connect anything to them.

On Fri, Jul 22, 2016 at 5:49 PM, Schuyler Eldridge <notifications@github.com

wrote:

FYI, it looks like the (to be deprecated?) Vec.fill approach doesn't like vectors of size zero. The following craps out:

val din = Vec.fill(numReadWritePorts)(UInt(OUTPUT, width = dataWidth))

With the following (truncated) error:

... Caused by: java.lang.IllegalArgumentException: requirement failed at scala.Predef$.require(Predef.scala:207) at chisel3.core.Vec$.do_apply(Aggregate.scala:50) at chisel3.core.Vec$.do_fill(Aggregate.scala:95) ...

Due to: https://github.com/ucb-bar/chisel3/blob/master/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala#L50

The supported Vec(gen: T, n: Int) works fine, however:

val din = Vec(UInt(OUTPUT, width = dataWidth), numReadWritePorts)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ucb-bar/chisel3/issues/188#issuecomment-234688829, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaehwC5YH0fACIQp1fAc9hrCIF3N156ks5qYWU1gaJpZM4IdkOl .