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

Register Widths #10

Open
seldridge opened this issue Jan 24, 2020 · 6 comments
Open

Register Widths #10

seldridge opened this issue Jan 24, 2020 · 6 comments

Comments

@seldridge
Copy link

The asymmetry of how Reg, RegNext, and RegInit set their widths can lead to unexpected behavior.

h/t @antonblanchard

Type of issue: bug report | feature request | other enhancement

Impact: API modification

Development Phase: request

Other information

There's an old PR referencing and documenting some of this behavior: chipsalliance/chisel#455

If the current behavior is a bug, please provide the steps to reproduce the problem:

Consider the following example. While foo, bar, and baz have almost identical construction, foo has an unset width, bar was a width of 8, and baz is a bundle with an internal signal of width 8.

package examples

import chisel3._
import chisel3.stage.{ChiselStage, ChiselGeneratorAnnotation}
import chisel3.internal.sourceinfo.UnlocatableSourceInfo

object NoInfo {
  implicit val noInfo = UnlocatableSourceInfo
}

import NoInfo._

class DelayedBundle extends Bundle {
  val in = UInt(8.W)
}

class Delayed extends MultiIOModule {
  val in = IO(Input(UInt(8.W)))
  val bundle = IO(Input(new DelayedBundle))

  /** The width of foo is unset */
  val foo = RegNext(in)

  /** The width of bar is set to 8.W */
  val bar = Reg(chiselTypeOf(in))
  bar := in
  
  /** The width of baz is set to 8.W */
  val baz = RegNext(bundle)

  Seq(foo, bar, baz).foreach(a => println(a.widthOption))

}

object Delayed extends App {
  (new ChiselStage).execute(Array.empty, Seq(ChiselGeneratorAnnotation(() => new Delayed)))
}

This results in the following FIRRTL:

circuit Delayed : 
  module Delayed : 
    input clock : Clock
    input reset : UInt<1>
    input in : UInt<8>
    input bundle : {in : UInt<8>}
    
    reg foo : UInt, clock
    foo <= in
    reg bar : UInt<8>, clock
    bar <= in
    reg baz : {in : UInt<8>}, clock
    baz.in <= bundle.in

To be very concrete, I have implicit classes for doing automatic conversion to bits, words, etc. (i.e., (Bits) => Vec[UInt]). These rely on having widths known. Using these methods on foo will cause the implicit class methods to error out complaining that they can't be used on something that has an unknown width. However, they work with bar. This is unintuitive behavior as it would seem that foo and bar are identical constructions.

I do realize that RegNext(0.U) is probably the reason why this exists the way it does.

What is the current behavior?

The width of a RegNext is always unset when given a subtype of Bits.

What is the expected behavior?

The width of a RegNext should be set to the type of whatever is next.

Please tell us about your environment:

What is the use case for changing the behavior?

This clarifies unintuitive behavior and unifies behavior for RegNext when given different types.

@seldridge seldridge changed the title RegNext Widths Register Widths Jan 24, 2020
@kammoh
Copy link

kammoh commented Feb 4, 2020

I'm hit by something which seems related without doing anything fancy and find it seriously annoying. In my case asBools fails because RegNext does not keep the width information, while y and z have know widths at elaboration time.

val s = RegNext(~y & z)
val t = x ^ s
(t).asBools()  // <---

results in chisel3.internal.ChiselException: Width of UInt(OpResult in KeccakCore) is unknown!

While this works:

val s = Reg(UInt(5.W))
s:= ~y & z
val t = x ^ s
(t).asBools() 

and removing the Reg works too:

( x ^ (~y & z)).asBools() 

@aswaterman
Copy link
Member

aswaterman commented Feb 4, 2020

Propagating the width from the RegNext is harder than it seems, because later assignments can increase the width. For example,

val foo = RegNext(bar)
...
foo := baz

is legal, and foo's width will be the larger of bar's and baz's.

@kammoh
Copy link

kammoh commented Feb 4, 2020

This could get into the realm of "Chisel philosophy :)" and beyond my understanding, but as a designer I'd really like the second assignment to error out if the width is larger than what's explicitly set at instantiation.

@seldridge
Copy link
Author

seldridge commented Feb 5, 2020

I think the real issue here is that the RegNext API doesn't give the user a choice of setting the width. Conversely, when declaring a Reg a user has the choice of whether or not you specify a width (Reg(UInt()) vs. Reg(UInt(4.W))).

For val foo = Reg(UInt()) vs val bar = Reg(UInt(4.W)), it's obvious to a user why foo.getWidth fails while bar.getWidth does not. It's not obvious that RegNext doesn't pass along the width.

I think this is more an example of there being two different, desirable behaviors with RegNext providing an API for only one. Users who expect this (or understand Chisel internals) grok what's going on, but a user who just wants to delay a signal by a cycle is rightly confused.

In the short term, RegNext should be documented as "Return a 1-cycle delayed version of a signal with an unspecified width`. In the medium term, two APIs might help here.

@schoeberl
Copy link

schoeberl commented Feb 7, 2020 via email

@seldridge
Copy link
Author

Dev meeting result: this is a complicated issue that touches on identifying the correct behavior of widths. E.g, when are widths set, when are widths inferred. Also, any change to this will result in a lot of code silently breaking.

Therefore, this is better handled via an RFC on the full discussion of widths. I will transfer this to the RFCs repo and close this there once an RFC is written.

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

No branches or pull requests

4 participants