Registers Update on the Wrong Cycle

[ben-k]

Whether using the Tester class or a vcd, Chisel displays the input value of registers, rather than the output, which I don't think is correct behavior. Here's a very simple example:

class Cordic extends Module {
val io = new CordicIo()
val delay = Reg(next=io.in,init=Bool(false))
io.out := delay
}

And here's the Tester result:

STEP 2 -> 2
POKE Cordic.io_in <- 1
STEP 1 -> 3
PEEK Cordic.io_in -> 0x1
PEEK Cordic.delay -> 0x1
PEEK Cordic.io_out -> 0x0
POKE Cordic.io_in <- 0
STEP 1 -> 4
PEEK Cordic.io_in -> 0x0
PEEK Cordic.delay -> 0x0
PEEK Cordic.io_out -> 0x1

Delay is always displayed according to its input value, rather than its output. In particular, io.out is combinationally wired directly to the output of delay, so out and delay should never be different.

I don't think this necessarily impacts functionality, but it makes debugging using the Tester class or Chisel-generated waveforms very confusing.

[albert-magyar]

Here, it's not actually showing the input value of the registers. The behavior is that stepping the circuit does a clock_lo followed by a clock_hi, which results in updates to register values from the clock_hi not propagating through any combinational nodes (including wires) to affect values of downstream nodes until another step is performed. This is also mirrored in VCDs from testers. There is ongoing discussion about whether to change this often-confusing behavior.

[albert-magyar]

Here, it's not actually showing the input value of the registers. The behavior is that stepping the circuit does a clock_lo followed by a clock_hi, which results in updates to register values from the clock_hi not propagating through any combinational nodes (including wires) to affect values of downstream nodes until another step is performed. This is also mirrored in VCDs from testers. There is ongoing discussion about whether to change this often-confusing behavior.

[aswaterman]

It's not just confusing; it's wrong, and we should fix it.

On Fri, May 16, 2014 at 3:02 PM, Albert Magyar notifications@github.comwrote:

Here, it's not actually showing the input value of the registers. The
behavior is that stepping the circuit does a clock_lo followed by a
clock_hi, which results in updates to register values from the clock_hinot propagating through any combinational nodes (including wires) to affect
values of downstream nodes until another step is performed. This is also
mirrored in VCDs from testers. There is ongoing discussion about whether to
change this often-confusing behavior.

—
Reply to this email directly or view it on GitHubhttps://github.com/ucb-bar/chisel/issues/216#issuecomment-43384247
.

[aswaterman]

It's not just confusing; it's wrong, and we should fix it.

On Fri, May 16, 2014 at 3:02 PM, Albert Magyar notifications@github.comwrote:

Here, it's not actually showing the input value of the registers. The
behavior is that stepping the circuit does a clock_lo followed by a
clock_hi, which results in updates to register values from the clock_hinot propagating through any combinational nodes (including wires) to affect
values of downstream nodes until another step is performed. This is also
mirrored in VCDs from testers. There is ongoing discussion about whether to
change this often-confusing behavior.

—
Reply to this email directly or view it on GitHubhttps://github.com/ucb-bar/chisel/issues/216#issuecomment-43384247
.

[albert-magyar]

I personally agree that it was wrong, but there was some debate when I was presenting a possible change for this at the Chisel meeting. Jonathan and I came up with some optimizations for minimizing the performance costs of the extra computation when propagating signals properly for use with the tester, and the plan should be to settle all the tester-behavior questions as soon as possible.

[albert-magyar]

I personally agree that it was wrong, but there was some debate when I was presenting a possible change for this at the Chisel meeting. Jonathan and I came up with some optimizations for minimizing the performance costs of the extra computation when propagating signals properly for use with the tester, and the plan should be to settle all the tester-behavior questions as soon as possible.

[ben-k]

To the extent that there is debate about this, I would add that everyone in 290C this semester (BWRC folks, mostly) thought that this behavior didn't make any sense.

[ben-k]

To the extent that there is debate about this, I would add that everyone in 290C this semester (BWRC folks, mostly) thought that this behavior didn't make any sense.

[sdtwigg]

I just checked and this seems to also be a new issue for vcd dumping. I had fixed it before so that the dump function was being called in-between clock_lo and clock_hi (this would then at least dump waveforms that could be somewhat reasoned over); however, with the tester rewrite the same trick cannot be immediately employed.

[sdtwigg]

I just checked and this seems to also be a new issue for vcd dumping. I had fixed it before so that the dump function was being called in-between clock_lo and clock_hi (this would then at least dump waveforms that could be somewhat reasoned over); however, with the tester rewrite the same trick cannot be immediately employed.

[schoeberl]

If this is still the display of the 'input' value of a register at the waveform, then I also strongly think this is wrong. You see the value 'shifted' by a clock cycle. This confused quite some of my students.

Another argument: compare it with the waveform when simulating the Verilog in ModelSim where you will see the 'output' of the register.

Cheers,
Martin

On 17 May 2014, at 01:05, Albert Magyar notifications@github.com wrote:

I personally agree that it was wrong, but there was some debate when I was presenting a possible change for this at the Chisel meeting. Jonathan and I came up with some optimizations for minimizing the performance costs of the extra computation when propagating signals properly for use with the tester, and the plan should be to settle all the tester-behavior questions as soon as possible.

—
Reply to this email directly or view it on GitHub.

[schoeberl]

If this is still the display of the 'input' value of a register at the waveform, then I also strongly think this is wrong. You see the value 'shifted' by a clock cycle. This confused quite some of my students.

Another argument: compare it with the waveform when simulating the Verilog in ModelSim where you will see the 'output' of the register.

Cheers,
Martin

On 17 May 2014, at 01:05, Albert Magyar notifications@github.com wrote:

I personally agree that it was wrong, but there was some debate when I was presenting a possible change for this at the Chisel meeting. Jonathan and I came up with some optimizations for minimizing the performance costs of the extra computation when propagating signals properly for use with the tester, and the plan should be to settle all the tester-behavior questions as soon as possible.

—
Reply to this email directly or view it on GitHub.

[stevobailey]

I'd like to refresh this discussion and make sure someone is working on fixing it. It feels like a rather problematic bug.

[stevobailey]

I'd like to refresh this discussion and make sure someone is working on fixing it. It feels like a rather problematic bug.

[albert-magyar]

There is a fix for this that will be going in ASAP, likely in the next day when I return to Berkeley.

[albert-magyar]

There is a fix for this that will be going in ASAP, likely in the next day when I return to Berkeley.

[schoeberl]

That's very good news ;-)

Cheers,
Martin

On 24 Jun 2014, at 02:36, Albert Magyar notifications@github.com wrote:

There is a fix for this that will be going in ASAP, likely in the next day when I return to Berkeley.

—
Reply to this email directly or view it on GitHub.

[schoeberl]

That's very good news ;-)

Cheers,
Martin

On 24 Jun 2014, at 02:36, Albert Magyar notifications@github.com wrote:

There is a fix for this that will be going in ASAP, likely in the next day when I return to Berkeley.

—
Reply to this email directly or view it on GitHub.