wave form for registers is (still) off by one clock tick

[schoeberl]

Dear all,

I am a little bit confused about the wave forms generated for registers. I thought this off by one issue has been solved some time ago. However, as I see it today the wave form still is off by one for registers - or in other words displays the input signal of a register instead of the output value. In standard VHDL/Verilog simulation the register signal is the output signal (i.e., one cycle later). Please, please: this needs to be fixed! Otherwise the acceptance of Chisel by EE engineers, who like to look at wave forms, will drop very, very fast.

For a small example you can look at:

https://github.com/schoeberl/chisel-examples/blob/master/examples/src/BubbleFifo.scala

You can run this by checking out the repository and execute within the folder examples:

make fifo-test gtkwave generated/BubbleFifo.vcd

You will see that io_dout, which is connected to the output of dataReg, is one cycle later than dataReg.

Cheers, Martin

[ghost]

Can you verify that without #614 you are getting right results?

[schoeberl]

On 18 Dec, 2015, at 5:03, rndfax notifications@github.com wrote:

Can you verify that without #614 you are getting right results?

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

I checked with the latest version of Chisel cloned from GitHub and there it is ok. So this can be closed. Sorry for the noise generated.

I did see the issue when I build my project against “latest.release”. Will there be a release soon that includes the fix?

Cheers, Martin

[ucbjrl]

On Dec 18, 2015, at 11:22 AM, Martin Schoeberl notifications@github.com wrote:

On 18 Dec, 2015, at 5:03, rndfax notifications@github.com wrote:

Can you verify that without #614 you are getting right results?

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

I checked with the latest version of Chisel cloned from GitHub and there it is ok. So this can be closed. Sorry for the noise generated.

I did see the issue when I build my project against �latest.release�. Will there be a release soon that includes the fix?

Cheers, Martin

I�m planning on generating a release next Wednesday (which will include the fix).

[schoeberl]

Sorry, but I have to reopen this issue. The observed behavior (in the wave form) is different now, but not correct. The register and and output wire connected to it change at the same clock cycle. However, they change both one clock cycle too early. They change in the very same cycle as the input to the register changes, but should be one cycle later. Maybe I should come over and show you what I mean.

Martin

[ucbjrl]

On Dec 18, 2015, at 12:09 PM, Martin Schoeberl notifications@github.com wrote:

Sorry, but I have to reopen this issue. The observed behavior (in the wave form) is different now, but not correct. The register and and output wire connected to it change at the same clock cycle. However, they change both one clock cycle too early. They change in the very same cycle as the input to the register changes, but should be one cycle later. Maybe I should come over and show you what I mean.

Martin

Could you work with rndfax to resolve this?

[schoeberl]

Sure I can, but I don't know who rndfax is and where he/she is. Maybe he can send me an email, so we get direct contact.

BTW, I did some binary search through the releases and the issue was introduced with release 2.31. Release 2.30 and some more earlier ones display the register values correctly.

[ucbjrl]

On Dec 18, 2015, at 12:48 PM, Martin Schoeberl notifications@github.com wrote:

Sure I can, but I don't know who rndfax is and where he/she is. Maybe he can send me an email, so we get direct contact.

BTW, I did some binary search through the releases and the issue was introduced with release 2.31. Release 2.30 and some more earlier ones display the register values correctly.

Thanks Martin.

It looks like this may be the change:

diff --git a/src/main/resources/emul_api.h b/src/main/resources/emul_api.h
index 7cd6f3b..5a4cbd7 100644
--- a/src/main/resources/emul_api.h
+++ b/src/main/resources/emul_api.h
...
@@ -129,15 +130,15 @@ private:
   }

   virtual inline void step() {
-    module->dump();
 module->print(std::cerr);
 module->clock(LIT<1>(0));
+    module->dump();
 // FIXME: should call twice to get the output for now
-    module->clock_lo(LIT<1>(0));
+    module->clock_lo(LIT<1>(0), false);
   }

Donggyu should be involved as well.

[schoeberl]

Did a git bisect and ended up with this commit as the (first) issue: f40c9e02592591c78c2f25f1a4c8d9de65014b1e is the first bad commit commit f40c9e02592591c78c2f25f1a4c8d9de65014b1e Author: Donggyu Kim dgkim@eecs.berkeley.edu Date: Mon Nov 2 14:24:43 2015 -0800

fix vcd dump bugs

where module-dump was moved, but not (yet) the change to clock_lo. Shell we change back?

[ghost]

Okay, the only thing to do is to find the right place for module->dump() :)

It seems that module->dump() should never be moved around at the first place (my test compared to VCD from iverilog is passed with this patch):

diff --git a/src/main/resources/emul_api.h b/src/main/resources/emul_api.h
index 7316945..e7aa893 100644
--- a/src/main/resources/emul_api.h
+++ b/src/main/resources/emul_api.h
@@ -130,11 +130,11 @@ private:
   }

   virtual inline void step() {
+    module->dump();
     module->print(std::cerr);
     module->clock(LIT<1>(0));
     // FIXME: should call twice to get the output for now
     module->clock_lo(LIT<1>(0), false);
-    module->dump();
   }

   virtual inline void update() {

PS. I don't know if this patch is correct, I just guessing.

[schoeberl]

Just did the same and can say that this looks correct with my example.

[ucbjrl]

On Dec 18, 2015, at 1:42 PM, Martin Schoeberl notifications@github.com wrote:

Just did the same and can say that this looks correct with my example.

Let's wait for Donggyu's input before committing rndfax's patch.

[ghost]

This is great! Now someone who understands all this should come here and say "Who moved module->dump()?! Confess!" :)

[ucbjrl]

On Dec 18, 2015, at 1:48 PM, rndfax notifications@github.com wrote:

This is great! Now someone who understands all this should come here and say "Who moved module->dump()?! Confess!" :)

How about a test we can use to make sure this doesn�t just track the bottom bit of the release number.

[ghost]

@ucbjrl Well, BubbleFifo (which was pointed out by @schoeberl) is fine as test - it's quite sensitive for module->dump() placement.

[ghost]

Ok, or I'm just got tired or something, but even rolling back module->dump() to its first position some mine tests show strange results which are inconsistent with iverilog VCD. Most probably I made somewhere mistakes, I just don't see them, no time to debug. I'm leaving this issue to not generate the noise.

[donggyukim]

Unfortunately, It seems like the right place to place module->dump() is somewhere inside clock(). The first place was generally ok, but totally broken with my examples which are quite complex. Right now, I think there's no clear solution to this, so let me figure it out.

[donggyukim]

@schoeberl @rndfax Can you guys check waveforms with #635? I'll also write vcd dump checks from this commit if it's confirmed.

[ghost]

@donggyukim at first glance for my little test from #628, and also test BubbleFifo from @schoeberl looks fine to me. Due to this fact I can continue my work and if I found something suspicious I'll let you know :)

[schoeberl]

How can I merge your pull request at ucb-bar/chisel into my local clone of chisel?

[ucbjrl]

Depending on your git configuration, you may be able to:

% git fetch origin
% git checkout origin/pr/635/merge

This may fail if your git configuration is set up to �squash� pull requests (which unfortunately, seems to be the common setup).

I believe the branch �fix_tester_outputs� is pretty close to the result of the pr #635 merge, so if the above fails, you should be able to:

% git checkout fix_tester_outputs

On Jan 21, 2016, at 3:52 PM, Martin Schoeberl notifications@github.com wrote:

How can I merge your pull request at ucb-bar/chisel into my local clone of chisel?

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

[schoeberl]

On 21 Jan, 2016, at 16:32, Jim Lawson notifications@github.com wrote:

Depending on your git configuration, you may be able to:

% git fetch origin % git checkout origin/pr/635/merge

This may fail if your git configuration is set up to �squash� pull requests (which unfortunately, seems to be the common setup).

Sadly this did not work. I also tried it on a fresh clone from ucb-bar/chisel, but I only got:

error: pathspec 'origin/pr/635/merge' did not match any file(s) known to git.

I believe the branch �fix_tester_outputs� is pretty close to the result of the pr #635 merge, so if the above fails, you should be able to:

% git checkout fix_tester_outputs

That one gives the correct waveform for my bubble example. So this should fix the issue. Very good!

Cheers, Martin

On Jan 21, 2016, at 3:52 PM, Martin Schoeberl notifications@github.com wrote:

How can I merge your pull request at ucb-bar/chisel into my local clone of chisel?

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

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