ucb-bar / chisel2-deprecated

chisel.eecs.berkeley.edu
388 stars 90 forks source link

Wrong VCD for wires [suggested PATCH included] #609

Closed ghost closed 8 years ago

ghost commented 8 years ago

Hello!

With latest git-clone:

scalaVersion := "2.11.7"

libraryDependencies += "edu.berkeley.cs" %% "chisel" % "2.3-SNAPSHOT"

scalacOptions ++= Seq("-deprecation", "-feature", "-unchecked", "-language:reflectiveCalls")

With this simple code:

import Chisel._

class Top extends Module {
        val io = new Bundle {
                val empty = Bool(OUTPUT) 

                val fb = Bool(INPUT) 
        }
        val head = Reg(init = UInt(0, width = 32)) 
        val tail = Reg(init = UInt(0, width = 32)) 
        val diff = tail - head

        io.empty := diff === UInt(0)

        when (io.fb) { 
                head := head + UInt(1) 
        }
}

class TopTests(c: Top) extends Tester(c) {
        step(2) 
        poke(c.io.fb, true) 
        step(2)
}

object Top {
        def main(args: Array[String]): Unit = {
                chiselMain(args.slice(1, args.length), () => Module(new Top()), (c: Top) => new TopTests(c))
        }
}
run Top --backend c --genHarness --compile --test --vcd

Wire 'io_empty' has one tick latency reaction on values - this is not what, for example, iverilog produces. With patch below I've got identical with iverilog results:

diff --git a/src/main/resources/emul_api.h b/src/main/resources/emul_api.h
index 5a4cbd7..7316945 100644
--- a/src/main/resources/emul_api.h
+++ b/src/main/resources/emul_api.h
@@ -132,9 +132,9 @@ private:
   virtual inline void step() {
     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), false);
+    module->dump();
   }

   virtual inline void update() {
ghost commented 8 years ago

Also, this patch confirms identical results with iverilog VCD with more complex example (I can provide it if needed).

albert-magyar commented 8 years ago

This is a regression on a fix to this issue from a year or so ago. Thanks for the report!

donggyukim commented 8 years ago

Yes, I agree that this is a better placement. Thanks a lot!

ghost commented 8 years ago

Since it's merged ....