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 size inference #654

Open
schoeberl opened this issue Feb 10, 2016 · 12 comments
Open

Register size inference #654

schoeberl opened this issue Feb 10, 2016 · 12 comments

Comments

@schoeberl
Copy link
Member

The following code should size a register for 4 bits by using an UInt constant:

val reg = Reg(UInt(15))
reg := reg + UInt(1)
io.result := reg

However, the resulting register is a single bit. Both in simulation and in the Verilog code.

Find the test case at: https://github.com/schoeberl/chisel-examples/blob/master/examples/src/main/scala/questions/RegSize.scala

run the simulation with:
sbt "run-main questions.RegSizeTester"
gtkwave RegSize.vcd
or the Verilog generation with
sbt "run-main questions.RegSize"

@aswaterman
Copy link
Member

Reg(t) creates a Reg with the same type as t. For these purposes, UInt(15)
and UInt() are the same. Whether or not this is desirable behavior is
debatable, but as of this moment it's technically correct.

On Tue, Feb 9, 2016 at 10:22 PM, Martin Schoeberl notifications@github.com
wrote:

The following code should size a register for 4 bits by using an UInt
constant:

val reg = Reg(UInt(15))
reg := reg + UInt(1)
io.result := reg

However, the resulting register is a single bit. Both in simulation and in
the Verilog code.

Find the test case at:
https://github.com/schoeberl/chisel-examples/blob/master/examples/src/main/scala/questions/RegSize.scala

run the simulation with:
sbt "run-main questions.RegSizeTester"
gtkwave RegSize.vcd
or the Verilog generation with
sbt "run-main questions.RegSize"


Reply to this email directly or view it on GitHub
#654.

@schoeberl
Copy link
Member Author

If the bit width of an UInt is not part of the type (has no meaning in Reg(UInt(n)))
why is the register width than 1? Could be 0, or anything?
Has Reg(T) without a init or next value any meaning then?
If not I would drop that constructor.

@aswaterman
Copy link
Member

The width of reg can be thought of as the minimal value that solves the
constraint problem

width(reg) >= width(reg + UInt(1))

where

width(reg + UInt(1)) = max(width(reg), width(UInt(1)))

so width(reg) = width(UInt(1)) = 1. Perhaps not intuitive, but definitely
not arbitrary.

On Tue, Feb 9, 2016 at 11:20 PM, Martin Schoeberl notifications@github.com
wrote:

If the bit width of an UInt is not part of the type (has no meaning in
Reg(UInt(n)))
why is the register width than 1? Could be 0, or anything?
Has Reg(T) without a init or next value any meaning then?
If not I would drop that constructor.


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

@schoeberl
Copy link
Member Author

Is there a use case for creating a register with Reg(T) when the size is reduced
to a single bit?

The current behavior is at least surprising, in the worst it can lead to hard to detect
bugs in user code. I would call this than as "code to avoid". Something we need to teach
when using VHDL due to VHDL language issues. Something I would love to see
minimized with Chisel.

I would argue to get the width right from the UInt constant or drop this single parameter
constructor (factory). I remember at some version of Chisel Reg(v) was a shorthand
for Reg(next = v). Why was this dropped?

@shunshou
Copy link
Member

shunshou commented Mar 5, 2016

Just noticed this, but I (and at least one other person) ran into similar issues a couple of months ago. I think you should just not make Reg accessible to Chisel users, because I was using Reg(x), assuming it'd act like Reg(next=x) and nothing behaved the way "I expected." RegNext and RegInit are sufficient...

Takeaway: don't support constructs that might be misleading (unless people have a thorough understanding of the Chisel internals -- but requiring that would be bad for language adoption IMO), because debugging is frustrating. :(

@aswaterman
Copy link
Member

On Sat, Mar 5, 2016 at 12:54 PM, Angie Wang notifications@github.com
wrote:

Just noticed this, but I (and at least one other person) ran into similar
issues a couple of months ago. I think you should just not make Reg
accessible to Chisel users, because I was using Reg(x), assuming it'd act
like Reg(next=x) and nothing behaved the way "I expected." RegNext and
RegInit are sufficient...

In this proposal, how do you create a register with no reset value that is
only conditionally updated?

Takeaway: don't support constructs that might be misleading (unless people
have a thorough understanding of the Chisel internals -- but requiring that
would be bad for language adoption IMO), because debugging is frustrating.
:(


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

@shunshou
Copy link
Member

shunshou commented Mar 5, 2016

Conditional update is essentially just a DFF reg with input mux. Either you conditionally update to a new value or you pass the output back to the input. (I'm not really sure how tools are able to infer clock gates from enables, so whether this works for that, I don't know...)

As long as the value passed into the register doesn't depend on the register's previous value (i.e. not a counter), you just assume that the register will have !@#$ until the enable is set at least once. Then the state is always defined after that... I think that's a fair assumption. If you wanted a counter, then you'd have to specify the initial value with RegInit. Something like:

val out = UInt(OUTPUT,width=5) [assuming an IO]
val next = Mux(io.cond,io.in,io.out)
io.out := RegNext(next)

  • I'm using a custom RegNext that behaves as follows:

val temp = Reg{T}(Some(next), Some(next), None, Some(clock)).toBits (square brackets are giving me problems in markdown...)
val out = chiselCast(temp){next.cloneType}

Basically, think of the logic as a signal flow graph, with operations and nodes. Fundamentally, a Reg is just converted to a DFF (operation) in hardware... At the most basic level, you just need to spec the data in, data out, and clk (nodes).

How it works is exactly spelled out, and it'll error out if you try something that shouldn't be supported. I'd rather get a ton of compile-time errors than think something worked, only to find out that the output was mathematically incorrect because of some intermediate width inference (or other Chisel assumption) I wasn't familiar with. And I've been running into the latter problem frequently enough with various aspects of Chisel...

@shunshou
Copy link
Member

shunshou commented Mar 5, 2016

Also, I haven't tried using when instead of Mux, without defining a default case i.e. the feedback. I assume it will derp just like it derps when you don't specify default values for when's w/ combinational logic.

Maybe defining a default condition for registers is too much additional extra coding for people, b/c Verilog + VHDL properly infer it, but I'd sacrifice verbosity for always getting the obvious result.

@ccelio
Copy link
Member

ccelio commented Mar 5, 2016

On Sat, Mar 5, 2016 at 12:54 PM, Angie Wang notifications@github.com wrote:

Just noticed this, but I (and at least one other person) ran into similar
issues a couple of months ago. I think you should just not make Reg
accessible to Chisel users, because I was using Reg(x), assuming it'd act
like Reg(next=x) and nothing behaved the way "I expected." RegNext and
RegInit are sufficient...

In this proposal, how do you create a register with no reset value that is
only conditionally updated?

I'd really love for a compiler warning (error?) that catches use of Reg(x), where x is not a type, but a literal.

Reg(UInt(0))

Should be invalid, when the user really wants is:

Reg(UInt())

I'd also prefer that if users want to get the type of a signal, they explicitly ask for it:

Reg(io.a) // bad, easily confused with RegNext(io.a)

Reg(io.a.clone()) // intention is clearer (not sure .clone() does what I want but you get the idea).

I admit I don't know if my proposal is doable, or even desirable. But I've been looking through some code, and I've found a number of Reg(io.something.bits), and I find that construct very misleading.

@shunshou
Copy link
Member

shunshou commented Mar 5, 2016

At least for me, I definitely typed something like Reg(io.a) expecting it to behave like RegNext(io.a)... I assume that's actually a fairly high frequency mistake. If I did something like Reg(io.a.type) <-- or in my case, I'd use something like Reg(io.a.cloneType), I'd really hope that any width information associated with io.a is also passed along with the type.

For RegNext, it makes sense to do that, b/c the width of the input/output of a reg shouldn't be different (regardless of how many bits of the output you actually use down the chain).

@shunshou
Copy link
Member

shunshou commented Mar 5, 2016

As a side note, again should probably go in a new issue, but I think the default + might be better off as the current +&. If you wanted to use functional programming i.e. fold to add a chain of #'s together, if you don't properly spec out the max bitwidth of the output and make sure that at least one of the inputs has that bitwidth (padding or similar), you'll likely get an incorrect value. And it's also one of those hard to debug problems... But really, you want + to just work and get the right sum...

There is still some value to keeping the + functionality (I keep it as +% to remind the user that it'll wrap on overflow).

Obviously, +& is sub-optimal as far as resource utilization is concerned, and that's when range inference becomes important... But first and foremost, something should be functionally correct.

@JackDavidson
Copy link
Contributor

So it looks like this is essentially the same issue as #668

I posed a proposed solution that will change what is currently done by Reg(UInt(##)) into Reg(UInt), and make Reg(UInt(##)) do what most users are expecting

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

5 participants