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

Simulation of Verilog output doesn't match nMigen simulation #418

Closed
jeanthom opened this issue Jul 1, 2020 · 17 comments
Closed

Simulation of Verilog output doesn't match nMigen simulation #418

jeanthom opened this issue Jul 1, 2020 · 17 comments

Comments

@jeanthom
Copy link

jeanthom commented Jul 1, 2020

Hi! I was working on a design that used comb statements inside a FSM and noticed that nMigen outputs "always @*" verilog blocks. This causes issues in verilog simulators because the always block is only evaluated when a signal of its sensibility list changes.

Here's some minimal code that showcases this bug:

from nmigen import *
from nmigen.cli import main

class Top(Elaboratable):
	def __init__(self):
		self.a = Signal()

	def elaborate(self, platform):
		m = Module()

		with m.FSM():
			with m.State("aaaaaaaa"):
				m.d.comb += self.a.eq(1)

		# Workaround for #417 
		b = Signal()
		m.d.sync += b.eq(1)

		return m

if __name__ == "__main__":
	top = Top()
	main(top, ports=[top.a])

Its verilog output:

/* Generated by Yosys 0.9+2406 (git sha1 9d658a19, gcc 10.1.1 -fPIC -Os) */

(* \nmigen.hierarchy  = "top" *)
(* top =  1  *)
(* generator = "nMigen" *)
module top(b, a);
  (* src = "test.py:6" *)
  output a;
  reg a;
  (* src = "test.py:7" *)
  input b;
  (* src = "test.py:12" *)
  wire fsm_state;
  always @* begin
    a = 1'h0;
    (* src = "test.py:12" *)
    casez (fsm_state)
      /* \nmigen.decoding  = "aaaaaaaa/0" */
      /* src = "test.py:13" */
      1'h0:
          a = 1'h1;
    endcase
  end
  assign fsm_state = 1'h0;
endmodule

When the Verilog code is simulated with either Icarus Verilog or Modelsim, a is invalid (because its value hasn't been set). When simulated using nMigen built-in simulator, a is 1.

@jeanthom
Copy link
Author

jeanthom commented Jul 1, 2020

Adding proc to the Yosys script seems to fix the issue.

@whitequark
Copy link
Member

whitequark commented Jul 1, 2020

We should already be handling that case (grep for $verilog_initial_trigger...) but we don't because of a bug.

@jeanthom
Copy link
Author

jeanthom commented Jul 1, 2020

Is there a ticket in Yosys for the bug you mention?

@whitequark
Copy link
Member

whitequark commented Jul 1, 2020

It is not technically a Yosys bug; Yosys outputs "structural netlists" so it doesn't care about what iverilog does. The bug lies in the way nMigen combines with Yosys. I'll need to think about a good way to fix this.

@whitequark
Copy link
Member

whitequark commented Jul 1, 2020

This is actually a surprisingly subtle issue. I'll raise it at the next Yosys meeting.

@whitequark
Copy link
Member

whitequark commented Jul 2, 2020

@jeanthom I've discussed this in a Yosys meeting today. The conclusion was to adjust the write_verilog output to include a workaround for this problem, such that instead of:

reg x;
always @* begin
    x <= 1;
end

we get something like:

reg init;
initial init = 0;
reg x;
always @* begin
    if (init) begin end
    x <= 1;
end

@jeanthom
Copy link
Author

jeanthom commented Jul 3, 2020

Thank you for taking the time to discuss this issue with the Yosys team. That solution seems good, may I ask you why an initial statement wasn't considered (so that the register is set to its correct initial value)?

@whitequark
Copy link
Member

whitequark commented Jul 4, 2020

That solution seems good, may I ask you why an initial statement wasn't considered (so that the register is set to its correct initial value)?

It was; reg x = 0; is the same thing as reg x; initial x = 0;, and for the simple cases where the initial value does not depend on any other wires/regs in the design, we already do that. But for complex cases like in this bug report, how would you use initial?

@jeanthom
Copy link
Author

jeanthom commented Jul 6, 2020

I would resolve the wire/regs dependencies, but this might be something to avoid on really large designs. BTW does the init approach induces a performance penalty in some simulators?

@whitequark
Copy link
Member

whitequark commented Jul 6, 2020

I would resolve the wire/regs dependencies

Yeah, so requiring manual changes makes that approach unviable, of course. The goal of a code generator is to make my life simpler, not the other way around.

BTW does the init approach induces a performance penalty in some simulators?

Hard to say without benchmarking. I know it wouldn't affect Verilator.

@jeanthom
Copy link
Author

jeanthom commented Jul 16, 2020

The issue was solved upstream.

@whitequark whitequark reopened this Jul 16, 2020
@whitequark
Copy link
Member

whitequark commented Jul 16, 2020

This is actually not entirely fixed yet--nMigen should detect whether Yosys has the fix, and omit proc_prune if it doesn't.

@whitequark whitequark added this to the 0.3 milestone Jul 22, 2020
@whitequark
Copy link
Member

whitequark commented Aug 26, 2020

It turns out I've also forgot to fix the Verilog-2001 mode I didn't, that was YosysHQ/yosys#2273.

@whitequark
Copy link
Member

whitequark commented Aug 26, 2020

When the Verilog code is simulated with either Icarus Verilog or Modelsim, a is invalid (because its value hasn't been set).

Now that I'm looking at this again, I'm confused. How is the behavior of your snippet not a simualtor bug? The continuous assignment assign fsm_state = 1'h0; should produce an event at time 0 per IEEE 1364-2005 §11.6.1:

Screenshot_20200826_085846

Further, the always @* block that contains casez (fsm_state) should be scheduled in response to that event, which is explicitly said in IEEE 1364-2005 §9.7.5:

Screenshot_20200826_090036
Screenshot_20200826_090040

@whitequark
Copy link
Member

whitequark commented Aug 26, 2020

@jeanthom If I run the following Verilog with Icarus Verilog:

/* Generated by Yosys 0.9+2406 (git sha1 9d658a19, gcc 10.1.1 -fPIC -Os) */

(* \nmigen.hierarchy  = "top" *)
(* top =  1  *)
(* generator = "nMigen" *)
module top(b, a);
  (* src = "test.py:6" *)
  output a;
  reg a;
  (* src = "test.py:7" *)
  input b;
  (* src = "test.py:12" *)
  wire fsm_state;
  always @* begin
    a = 1'h0;
    (* src = "test.py:12" *)
    casez (fsm_state)
      /* \nmigen.decoding  = "aaaaaaaa/0" */
      /* src = "test.py:13" */
      1'h0:
          a = 1'h1;
    endcase
  end
  assign fsm_state = 1'h0;

  initial begin
    $dumpfile("t.vcd");
    $dumpvars;
    #100;
    $finish;
  end
endmodule

I get this VCD file:

$scope module top $end
$var wire 1 ! b $end
$var wire 1 " fsm_state $end
$var reg 1 # a $end
$upscope $end
$enddefinitions $end
#0
$dumpvars
1#
0"
z!
$end
#100

That definitely sets a. Please take another look at this issue.

@jeanthom
Copy link
Author

jeanthom commented Aug 26, 2020

I'm also confused because I spent the whole day trying to reproduce the issue with older versions of Yosys to no avail... Probably the minified test code was wrong?

The issue I faced in gram#18 was that the generated Verilog code looked like this:

  reg fsm_state;
  always @* begin
    a = 1'h0;
    casez (fsm_state)
      1'h0:
          a = 1'h1;
    endcase
  end

fsm_state didn't have a default value, thus not triggering an event at t=0. The issue was indeed fixed with your PR on Yosys.

@whitequark
Copy link
Member

whitequark commented Aug 26, 2020

Thanks. I can work with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants