RHS Vector of Bundles UInt Index Broken

[seldridge]

I'm seeing some strange behavior that seems to relate to using a variable indexed Vector of Bundles in the RHS of an assignment.

The example testbench highlights the problem:

package variableBundleRHS

import Chisel._

class StorageBundle extends Bundle {
  val field = Reg(UInt(width = 16))
}

class VariableBundleRHS extends Module {
  val io = new Bundle {
    val valid   = Bool(INPUT)
    val address = UInt(INPUT, width = log2Up(2))
  }
  val vecOfReg = Vec.fill(2){Reg(UInt(width = 16))}
  val vecOfBun = Vec.fill(2){new StorageBundle}

  when (io.valid) {
    vecOfReg(io.address)       := vecOfReg(io.address)       + UInt(1)
    vecOfBun(io.address).field := vecOfBun(io.address).field + UInt(1)
  }

  (0 until 2).map(i => debug(vecOfReg(i)))
  (0 until 2).map(i => debug(vecOfBun(i).field))
}

class VariableBundleRHSTests (uut: VariableBundleRHS, isTrace: Boolean = false)
    extends Tester(uut, isTrace) {
  def info(isIncrement: Boolean = true) {
    if (isIncrement)
      printf("Increment Index %d -> VecOfReg: ", peek(uut.io.address))
    else
      printf("Initial State     -> VecOfReg: ")
    (0 until 2).map(i => printf("|%d", peek(uut.vecOfReg(i))))
    printf("| VecOfBun:")
    (0 until 2).map(i => printf("|%d", peek(uut.vecOfBun(i).field)))
    printf("|\n")
  }

  info(false)
  for (i <- 0 until 10) {
    poke(uut.io.valid, 1)
    poke(uut.io.address, rnd.nextInt(2))
    step(1)
    info()
  }

  (0 until 2).map(i => expect(peek(uut.vecOfReg(i)) == peek(uut.vecOfBun(i).field),
    "Vector and Bundle increment do not match"))
}

object Main {
  def main(args: Array[String]): Unit = {
    val tutArgs = args.slice(1, args.length)
    val res = args(0) match {
      case "VariableBundleRHS" =>
        chiselMainTest(tutArgs, () => Module(new VariableBundleRHS())){
          c => new VariableBundleRHSTests(c)}
    }
  }
}

I create two equivalent table-like structures. One is a vector of registers. The other is a vector of a bundles. I then try to increment both of these structures using a random index. The vector of registers works while the vector of bundles doesn't. Running the above testbench gives you (for some random seed):

SEED 1431553259900
STARTING build/VariableBundleRHS
Initial State     -> VecOfReg: |0|0| VecOfBun:|0|0|
Increment Index 1 -> VecOfReg: |0|1| VecOfBun:|0|1|
Increment Index 0 -> VecOfReg: |1|1| VecOfBun:|1|1|
Increment Index 0 -> VecOfReg: |2|1| VecOfBun:|1|1|
Increment Index 1 -> VecOfReg: |2|2| VecOfBun:|1|1|
Increment Index 1 -> VecOfReg: |2|3| VecOfBun:|1|1|
Increment Index 0 -> VecOfReg: |3|3| VecOfBun:|1|1|
Increment Index 0 -> VecOfReg: |4|3| VecOfBun:|1|1|
Increment Index 0 -> VecOfReg: |5|3| VecOfBun:|1|1|
Increment Index 0 -> VecOfReg: |6|3| VecOfBun:|1|1|
Increment Index 1 -> VecOfReg: |6|4| VecOfBun:|1|1|
RAN 10 CYCLES FAILED FIRST AT CYCLE 10
*** FAILED ***

Note, that it appears to be a problem with the variable index RHS of the bundle as using a fixed RHS, but variable LHS produces sane output. That is, changing the when (io.valid) block to the following has the bundle and vector outputs matching.

  when (io.valid) {
    vecOfReg(io.address)       := vecOfReg(0)       + UInt(1)
    vecOfBun(io.address).field := vecOfBun(0).field + UInt(1)
  }

SEED 1431553419138
STARTING build/VariableBundleRHS
Initial State     -> VecOfReg: |0|0| VecOfBun:|0|0|
Increment Index 1 -> VecOfReg: |0|1| VecOfBun:|0|1|
Increment Index 1 -> VecOfReg: |0|1| VecOfBun:|0|1|
Increment Index 1 -> VecOfReg: |0|1| VecOfBun:|0|1|
Increment Index 1 -> VecOfReg: |0|1| VecOfBun:|0|1|
Increment Index 1 -> VecOfReg: |0|1| VecOfBun:|0|1|
Increment Index 0 -> VecOfReg: |1|1| VecOfBun:|1|1|
Increment Index 0 -> VecOfReg: |2|1| VecOfBun:|2|1|
Increment Index 0 -> VecOfReg: |3|1| VecOfBun:|3|1|
Increment Index 1 -> VecOfReg: |3|4| VecOfBun:|3|4|
Increment Index 0 -> VecOfReg: |4|4| VecOfBun:|4|4|
RAN 10 CYCLES PASSED
PASSED

Note: I've edited the original post to simplify the still-failing module/testbench

[sdtwigg]

I need to properly work through this later; however, it is generally considered bad form/style (and is liable to be banned in future versions of chisel due to major abuse) to instantiate logic or do assignments in the constructor of a bundle.

Such actions make the bundle ineligible as a descriptor for io, memory, or reg, and tends to break certain internal assumptions as to what the user did to their bundle.

Note, it is perfectly acceptable to have functions in bundles that return logic (like fire in DecoupledIO) On May 12, 2015 9:28 PM, "Schuyler Eldridge" notifications@github.com wrote:

I'm seeing some strange behavior that seems to relate to using a variable indexed Bundle in the RHS of an assignment.

The example testbench highlights the problem:

package variableBundleRHS import Chisel._ class VariableBundleRHSInterface( val len: Int, val w: Int ) extends Bundle { val valid = Bool(INPUT) val address = UInt(INPUT, width = log2Up(len)) val vector = Vec.fill(len){UInt(OUTPUT, width = w)} val bundle = Vec.fill(len){UInt(OUTPUT, width = w)} } class StorageBundle( val w: Int ) extends Bundle { val field = Reg(init = UInt(0, width = w)) override def clone: this.type = new StorageBundle(w).asInstanceOf[this.type] } class VariableBundleRHS( val len: Int = 2, val w: Int = 16 ) extends Module { val io = new VariableBundleRHSInterface(len, w)

val vector = Vec.fill(len){Reg(init = UInt(0, width = w))} val bundle = Vec.fill(len){new StorageBundle(w)}

when (io.valid) { vector(io.address) := vector(io.address) + UInt(1) bundle(io.address).field := bundle(io.address).field + UInt(1) }

(0 until len).map(i => io.vector(i) := vector(i)) (0 until len).map(i => io.bundle(i) := bundle(i).field) } class VariableBundleRHSTests (uut: VariableBundleRHS, isTrace: Boolean = false) extends Tester(uut, isTrace) { def info { printf("Increment %d -> Vector: ", peek(uut.io.address)) (0 until uut.len).map(i => printf("|%d", peek(uut.vector(i)))) printf("| Bundle:") (0 until uut.len).map(i => printf("|%d", peek(uut.bundle(i).field))) printf("|\n") }

for (i <- 0 until 10) { poke(uut.io.valid, 1) poke(uut.io.address, rnd.nextInt(uut.len)) step(1) info } info

(0 until uut.len).map(i => expect(peek(uut.vector(i)) == peek(uut.bundle(i).field), "Vector and Bundle increment do not match")) } object Main { def main(args: Array[String]): Unit = { val tutArgs = args.slice(1, args.length) val res = args(0) match { case "VariableBundleRHS" => chiselMainTest(tutArgs, () => Module(new VariableBundleRHS())){ c => new VariableBundleRHSTests(c)} } } }

I create two equivalent table-like structures. One is a vector of registers. The other is a vector of a bundles. I then try to increment both of these structures using a random index. The vector of registers works while the vector of bundles doesn't. Running the above testbench gives you (for some random seed):

SEED 1431490135223 STARTING build/VariableBundleRHS Increment 1 -> Vector: |0|1| Bundle:|0|1| Increment 1 -> Vector: |0|2| Bundle:|0|1| Increment 1 -> Vector: |0|3| Bundle:|0|1| Increment 0 -> Vector: |1|3| Bundle:|1|1| Increment 1 -> Vector: |1|4| Bundle:|1|1| Increment 1 -> Vector: |1|5| Bundle:|1|1| Increment 1 -> Vector: |1|6| Bundle:|1|1| Increment 1 -> Vector: |1|7| Bundle:|1|1| Increment 0 -> Vector: |2|7| Bundle:|1|1| Increment 0 -> Vector: |3|7| Bundle:|1|1| Increment 0 -> Vector: |3|7| Bundle:|1|1| RAN 10 CYCLES FAILED FIRST AT CYCLE 10

Note, that it appears to be a problem with the variable index RHS of the bundle as using a fixed RHS, but variable LHS produces sane output. That is, changing the when (io.valid) block to the following has the bundle and vector outputs matching.

when (io.valid) { vector(io.address) := vector(0) + UInt(1) bundle(io.address).field := bundle(0).field + UInt(1) }

SEED 1431490345158 STARTING build/VariableBundleRHS Increment 0 -> Vector: |1|0| Bundle:|1|0| Increment 0 -> Vector: |2|0| Bundle:|2|0| Increment 1 -> Vector: |2|3| Bundle:|2|3| Increment 1 -> Vector: |2|3| Bundle:|2|3| Increment 1 -> Vector: |2|3| Bundle:|2|3| Increment 0 -> Vector: |3|3| Bundle:|3|3| Increment 0 -> Vector: |4|3| Bundle:|4|3| Increment 1 -> Vector: |4|5| Bundle:|4|5| Increment 1 -> Vector: |4|5| Bundle:|4|5| Increment 0 -> Vector: |5|5| Bundle:|5|5| Increment 0 -> Vector: |5|5| Bundle:|5|5| RAN 10 CYCLES PASSED PASSED

— Reply to this email directly or view it on GitHub https://github.com/ucb-bar/chisel/issues/419.

[seldridge]

I've edited the above code to reduce the complexity. I also tested this with a vector of size one (code at the end of this). With a vector of size one, the resulting graph looks like: Some R2 register is being inferred which should be equivalent to vecOfBun(0).field, but never gets reduced as such.

I'm trying to dig through the Vector, Bundle, and Register code to see how all this works, but it's slow going.

Tangentially, is what I'm doing here a legitimate use of Chisel? This test fragment comes from a design consisting of a bunch of tables (Vectors of Bundles of Registers) which need to be conditionally updated based on asynchronous requests and responses.

Vector size one code:

package variableBundleRHS

import Chisel._

class StorageBundle extends Bundle {
  val field = Reg(UInt(width = 16))
}

class VariableBundleRHS extends Module {
  val io = new Bundle {
    val valid   = Bool(INPUT)
    val address = UInt(INPUT, width = log2Up(1))
  }
  val vecOfReg = Vec.fill(1){Reg(UInt(width = 16))}
  val vecOfBun = Vec.fill(1){new StorageBundle}

  when (io.valid) {
    vecOfReg(0)       := vecOfReg(io.address)       + UInt(1)
    vecOfBun(0).field := vecOfBun(io.address).field + UInt(1)
  }

  debug(vecOfReg(0))
  debug(vecOfBun(0).field)
}

class VariableBundleRHSTests (uut: VariableBundleRHS, isTrace: Boolean = false)
    extends Tester(uut, isTrace) {
  def info(isIncrement: Boolean = true) {
    if (isIncrement)
      printf("Increment Index %d -> VecOfReg: ", peek(uut.io.address))
    else
      printf("Initial State     -> VecOfReg: ")
    printf("|%2d| VecOfBun: |%2d|\n",
      peek(uut.vecOfReg(0)), peek(uut.vecOfBun(0).field))
  }

  info(false)
  for (i <- 0 until 10) {
    poke(uut.io.valid, 1)
    poke(uut.io.address, 0)
    step(1)
    info()
  }

  expect(peek(uut.vecOfReg(0)) == peek(uut.vecOfBun(0).field),
    "Vector and Bundle increment do not match")
}

object Main {
  def main(args: Array[String]): Unit = {
    val tutArgs = args.slice(1, args.length)
    val res = args(0) match {
      case "VariableBundleRHS" =>
        chiselMainTest(tutArgs, () => Module(new VariableBundleRHS())){
          c => new VariableBundleRHSTests(c)}
    }
  }
}

[seldridge]

So, the whole vecOfBun(0).field example was probably erroneous, as this will use Vec's apply(idx: Int) and not apply(ind: UInt). This is evidenced by the fact that a vecOfBun(UInt(0)).field RHS has the exact same problem. So, this has nothing to do with a variable index, but a UInt index. Consequently, I would expect that this has something to do with Vec's read.

@sdtwigg, thanks for the advice. You are saying that instantiating a Reg in a Bundle is bad, right (not giving a register an initial value)? This does seem to be the root of the problem... However, lack of support for this is counterintuitive coming from a SystemVerilog background where I'm used to having registered updates to structs. There may exist a more Chisel/Scala-like way of describing what I'm trying to do here. Anyway, the ways that I've tried to work around this are below with two of them working.

tl;dr Using an additional "next" bundle seems to be the best workaround

Going with a slightly different approach of a strict wire Bundle and doing a registered update is closer, but still doesn't really grasp the underlying design for most cases, e.g.:

class StorageBundle extends Bundle {
  val field = UInt(width = 16)
}
// ...
when (io.valid) {
  vecOfBun(0).field := Reg(next = vecOfBun(io.address).field + UInt(1))
}

This doesn't build as there's do default value for the wire vecOfBun(0).field. I can define a default, fall-through case like the following:

vecOfBun(0).field := Reg(next = vecOfBun(io.address).field)
when (io.valid) {
  vecOfBun(0).field := Reg(next = vecOfBun(io.address).field + UInt(1))
}

However, this causes two registers to be inferred (which I don't really want) and vecOfBun(0).field is then tied to a wire with combinational logic before it.

But, throwing a mux into the mix does cause the intended design to be inferred:

vecOfBun(0).field := Reg(next = Mux(io.valid, vecOfBun(io.address).field + UInt(1),
    vecOfBun(io.address).field))

This can work for a small case, but if I have some large combinational structure which governs the register update this becomes much harder to describe. So, I suppose I can have an explicit bundle just for the next state:

val vecOfBun = Vec.fill(1){new StorageBundle}
val vecOfBun_next = Vec.fill(1){new StorageBundle}

vecOfBun(0) := Reg(next = vecOfBun_next(0))
vecOfBun_next(0).field := vecOfBun(io.address).field
when (io.valid) {
  vecOfBun_next(0).field := vecOfBun(io.address).field + UInt(1)
}

This works and gives me the most freedom to describe the next value of the register outside of somewhat restrictive ternary primitives. However, it's a non-intuitive solution as I'd expect this to work just like a vector of registers.

Apologies for the all the novel-length updates---just trying to document this as best as I can.

[sdtwigg]

A good way to think of how Bundles work currently is that they are analogous to C structs (or C++ classes without constructors). They just describe the structural layout of a bunch of data. Then, a port declaration (putting them in IO), wrapping them in a Reg call, Mem call, etc. recreates that structure as ports, registers, memories, etc. (If they are not wrapped, then they default to acting as wires; however, a Chisel 3.0 syntax will require wrapping them in a Wire call before use. This should make their role as a structural description vs. being actual hardware more straightforward.)

The general abstraction that results from this is that created Bundles should make sense in all context: as ports (where asInput, asOutput, or flipped was called before this), as wires, as a memory description, and as registers. In your case, the bundle only makes sense if used in wire context (as using it in a reg or port call would cause undefined behavior to happen and not produce helpful error messages or warnings).

Now, I actually don't quite understand precisely what hardware you want (can you write a really quick example in SystemVerilog?) but here is a possibly implementation strategy:

Don't actually use a Bundle. Just use a 'naked' scala class. Then, you can declare field as a Reg(UInt) as desired. This has the advantages that using this directly in a Reg, Mem, port call will yield a type error as desired (since those contexts don't make sense here). The only issue is if you then place this class inside a Bundle. The Bundle introspection/reflection will, correctly, ignore that class and the logic used by that class will just get dumped (correctly) into whatever module is being elaborated when that class is constructed. Bulk assignment on the enclosing Bundle will not touch that class (unless you override := yourself to force it to) and, if used in a Bunlde describing part of a port, then you are likely to get a cross-module reference exception. Also, you can't make a Vec of this class. Instead, you'd have to use scala containers. Then, if you want to do a UInt lookup, you do something like: Vec(myScalaContainer.map(_.field))(address)

There are possible alternatives; like declaring a Bundle but hiding some of the data-carrying fields via private (although this may require overriding the Bundle reflection since it then may not be able to find them).

Finally, I am exploring syntax and implementations details that would allow you to 'add a constructor' to Bundles (and thus let them be a class that can itself be nested into other Bundles or Vec aggregates). Basically, after Reg, Wire, Mem, Port, etc. is called on the Bundle, the user-supplied constructor code would run. This constructor would be told a brief summary of what its constituent nodes were turned into and could register an initialization error depending on context. Otherwise, it could immediately do some stuff to those nodes. I am still working through some of the edge cases, semantic details, and potential syntax though, so I do not expect this until Chisel 3.1.

[seldridge]

Stephen -- For whatever reason, I never tried just specifying it as you describe, i.e., leave the Bundle alone and wrap it in a Reg call. I thought this was my original tack but it must not have been... because this works! The two descriptions below are equivalent and the latter is the exact hardware that I'm trying to infer:

class StorageBundle extends Bundle {
  val field = UInt(width = 16)
}
// ... 
val vecOfReg = Vec.fill(2){Reg(UInt(width = 16))}
val vecOfBun = Vec.fill(2){Reg(new StorageBundle)}

So, your initial intuition about the use of Reg inside Bundle was the culprit and this is just user error / it would be great if Chisel disallowed this (as you suggest) or at least provided some strongly worded warning that the user should know what they're doing. You can close this unless you want to keep it open for disallowing logic instantiation/constructors in Bundles.

I can provide a SystemVerilog example if needed, but I don't know if this is helpful now. My usage case is simpler than I thought, Chisel supports it, and most of this stems from my confusion about how to properly describe a structure of registers in Chisel. As you know, it's basically the same in SV, where you describe a struct and then either assign values to this or describe edge-triggered updates. The struct in SV isn't a struct of registers, but a generic collection of to-be-typed signals. The way you then use that struct determines reg/wire typing. So, Chisel's Bundle and SV's struct are already logically equivalent, I just didn't realize it.

Thanks for all the help.

[sdtwigg]

Excellent, glad this is working for you.

Also, from our conversation, I will look into just describing bundles as SV structs in the documentation.

Out of curiosity are there any other SV features that seem lacking in Chisel (excluding ones that are clearly only for simulation and completely non-synthesis able)? On May 14, 2015 9:41 AM, "Schuyler Eldridge" notifications@github.com wrote:

Stephen -- For whatever reason, I never tried just specifying it as you describe, i.e., leave the Bundle alone and wrap it in a Reg call. I thought this was my original tack but it must not have been... because this works! The two descriptions below are equivalent and the latter is the exact hardware that I'm trying to infer:

class StorageBundle extends Bundle { val field = UInt(width = 16) }// ... val vecOfReg = Vec.fill(2){Reg(UInt(width = 16))}val vecOfBun = Vec.fill(2){Reg(new StorageBundle)}

So, your initial intuition about the use of Reg inside Bundle was the culprit and this is just user error / it would be great if Chisel disallowed this (as you suggest) or at least provided some strongly worded warning that the user should know what they're doing. You can close this unless you want to keep it open for disallowing logic instantiation/constructors in Bundles.

I can provide a SystemVerilog example if needed, but I don't know if this is helpful now. My usage case is simpler than I thought, Chisel supports it, and most of this stems from my confusion about how to properly describe a structure of registers in Chisel. As you know, it's basically the same in SV, where you describe a struct and then either assign values to this or describe edge-triggered updates. The struct in SV isn't a struct of registers, but a generic collection of to-be-typed signals. The way you then use that struct determines reg/wire typing. So, Chisel's Bundle and SV's struct are already logically equivalent, I just didn't realize it.

Thanks for all the help.

— Reply to this email directly or view it on GitHub https://github.com/ucb-bar/chisel/issues/419#issuecomment-102097544.

[seldridge]

Sound good. You may want to clear it with somebody more knowledgeable regarding the 1:1 correspondence of Chisel Bundle to SV struct. They also have some overlap with SV interfaces, though I never got around to using them.

Re: SV features, I took a look at the SV design I'm currently migrating to Chisel and the things which come to mind are:

• Severity specification for assertion failures. With SV, the action taken when an assertion fires is left to the designer. The standard procedure that I've used here is using one of {$warn, $error, $fatal} all of which will log the assertion, but only $fatal will actually kill the simulation. I think that the current implementation of assert in Chisel forces a hard exit. This all or nothing approach isn't ideal because the designer would like to monitor for "unexpected"/$warn, "bad"/$error, or "he's dead Jim"/$fatal situations.
• Some shorthand for assigning multiple values to a Bundle/Struct. With SV, for some struct you can do a mass assignment like below.

struct {
  logic [7:0] x;
  logic [31:0] y;
  logic z;
} my_struct;
assign my_struct <= '{
  x: 8'h1f,
  y: 32'hdeadbeef,
  z: 1};

This is really just useful from a programmer sanity perspective as it's much easier to visually parse that than below when grouped with other assignments.

assign my_struct.x = 8'h1f;
assign my_struct.y = 32'hdeadbeef;
assign my_struct.z = 1;

I think this can just be done with a Scala function (and I've done it that way), but it might be nice to extend the Bundle apply(name: String) method to support a list of strings. Not sure on this, though this seems like something that I could reasonably implement.

• Union types, but you guys are well aware of this (#127) and unions should be supported soon.

Beyond that, the great thing about Chisel is that a lot of the useful, but non-synthesizable portions of SV (object-oriented programming, polymorphism, inheritance, etc.) are available if you're using Chisel. Normally, these are relegated to SV testbenches which the simulation tools can handle, but none of the synthesis tools understand.

[sdtwigg]

1. Severity specification: Good idea. (Temporary workaround is that a printf in a when could emulate error and warn)
2. Shorthand: The (experimental) fromMap function comes close. my_struct := my_struct.fromMap(Map("x" -> UInt(1), "y" -> UInt(2), "z" -> UInt(3))) would work as expected. Unfortunately, omitting a field in the fromMap mapping leads to undesirable behavior. (Behind the scenes a copy of my_struct is made and assigned to. Omitting the field causes that field in the real my_struct to get assigned to an undriven wire.) This feature is still experimental with the details still subject to change, hence why it hasn't been fully documented yet.
3. unions: Yes, this is on the features list and mostly just a matter of getting the syntax/semantics/implementation correct.

PS: Yes, you have hit upon one of our major goals for Chisel that all described hardware be truly synthesizable. Thank you very much for your feedback!