Emit initial block and remove dummy_signal

[dvc94ch]

Currently in fpga mode regs are initialized as following:

reg a = 1'b1;

This isn't recognized by symbiyosys as initializing the regs. Also it does not
initialize output reg when the ClockDomain is reset_less.

New way of initializing regs and output regs:

initial begin
  a = 1'b1;
end

When initialized correctly this dummy_signal should not be required. Also
verilator refuses to compile.

[sbourdeauducq]

It used to be done like that - except with non-blocking assignment - but was changed in c0fb0ef

Is the blocking assignment better?

[dvc94ch]

Yes, non-blocking isn't accepted by verilator - that is why it doesn't compile. Why was it changed?

[dvc94ch]

This [0] is @cliffordwolf 's solution to this problem. I think it does the same as dummy signal, but I'm not a verilog expert...
I think it would be better to wrap the dummy_signal in ifndef SYNTHESIS instead of // synthesis translate_off`. Yosys spews in disgust :)

• [0] chipy-hdl/chipy@628fa71

[dvc94ch]

Added some more commits:

• fsm states have explicit widths using Constant(x, width) - verilator will complain otherwise
• Always use blocking assignment in comb blocks - If someone can explain to me why it's gated on asic_syntax that would be great. I think it's wrong.
• Emit `default_nettype none, without it you get extremely weird behaviour instead of an error message when making stupid mistakes. (Wasted half an hour yesterday)

[sbourdeauducq]

nstates.bit_length()?

[sbourdeauducq]

I think it would be better to wrap the dummy_signal in ifndef SYNTHESIS instead of// synthesis translate_off`. Yosys spews in disgust :)

synthesis translate_off is the de-facto standard, and hardly the most disgusting thing with Verilog. It also doesn't require adding defines that are potentially difficult to set up and easily forgotten to every synthesizer that takes the Migen code.

This [0] is @cliffordwolf 's solution to this problem. I think it does the same as dummy signal, but I'm not a verilog expert...

Looks similar to me: the if (__always_comb_guard) begin adds the signal to the sensitivity list, and the wire assignment generates an event on __always_comb_guard at the beginning.
I prefer the Migen solution since it is better decoupled: the useful code is not wrapped into a dummy if, and it doesn't rely on the synthesizer/simulator to optimize the if away.

fsm states have explicit widths

They always have an explicit width in the Verilog output. The problem is actually that the width could be different for different states.

Always use blocking assignment in comb blocks - If someone can explain to me why it's gated on asic_syntax that would be great. I think it's wrong.

There were some intractable problems (I cannot remember what it was exactly) with every simulator other than Verilator when using blocking assignments. Using non-blocking ones often works, but sometimes some simulators run comb blocks in infinite loops - and I do not know if this is a bug in the simulator, or a problem with the Migen approach. This kind of EDA snafu is the main reason for the integrated Migen Python simulator, where the behavior is better determined and such stupid issues avoided.
It might be okay to switch to blocking assignments and have Verilator as the only supported external simulator, if it works well, including with multiple clock domains. I'm also afraid of subtle synthesis bugs when making such a high-impact change on a large codebase - have you tested e.g. misoc on a board and does everything look OK?
I suspect that the best way out of this mess is to have Migen transform the output code so that every signal takes its final value at the end of the comb block - no iteration on comb blocks like it can often happen now. This transform should make external simulators behave and probably give a nice speed boost to the (very slow) integrated one.
As for the "ASIC syntax" see with @hutch31.

Emit `default_nettype none, without it you get extremely weird behaviour instead of an error message

This is fine (assuming commonly used EDA tools do not choke on it), but it would be easier to merge if you made a separate PR for such changes.

[sbourdeauducq]

IIRC @nakengelhardt tried Verilator and blocking assignments and there were cases where it would refuse to compile due to combinatorial loops (that could be broken but are not).

[dvc94ch]

Can you give an example of where combinatorial loops are generated so that I can take a look?
It is my understanding that stuff like this is never a good idea:

always @ (a or b)
  a = a + b;

works well, including with multiple clock domains

It won't detect domain crossing issues, but if all clock signals are added as ios and they are driven correctly in the verilator bench it should work, although I haven't tried it yet.

[sbourdeauducq]

It is my understanding that stuff like this is never a good idea:

always @ (a or b)
a = a + b;

Obviously not; the combinatorial loop cannot be broken here and this won't even synthesize to something sensible. But there are cases that are useful, where loops are "fake", and which synthesize just fine but break the simulator.

[nakengelhardt]

The fake combinatorial loops come from blocks that assign multiple values. If a first block is e.g.

always @(*) begin
a <= some_reg_val;
b <= c;
end

and the second is

always @(*) begin
if (a) c <= some_fixed_val;
else c <= another_fixed_val;
end

then despite there not being any real combinatorial loop (since a is always assigned a fixed value), executing the first block marks 'a' as changed, so the second block is run, which marks 'c' changed, so the first block is run, etc... For this simple case some optimizing simulators manage to realize the problem and make it go away, but for a big design you're almost guaranteed to run into some problem of this type. It makes migen code impossible to run with any simulator I've tried, including icarus verilog, xsim, modelsim, and verilator. (Although verilator will give compile-time errors rather than infinite loops.)

Changing to blocking assignments doesn't interact well with the way If() is implemented either. When migen exports e.g. If(a, b.eq(c)), it includes first a reset statement, to avoid latches when no else is provided:

always @(*) begin
b <= 1'd0;
if(a) b <= c;
end

There can be whole groups of those in blocks with multiple assignments. Crucially, all the resets happen at the beginning of the block. With blocking assignments, these resets sometimes end up overwriting the values that should be assigned, and spurious reset values get propagated through the design.

[sbourdeauducq]

Right, here is an example:

always @(*) begin
  // migen built-in resets
  c <= 1'd0;
  b <= 1'd0;
  // migen comb block
  if(b) c <= 1'd1;
  if(a) b <= 1'd1; 
end

With non-blocking assignments, this is equivalent to c <= a, which is the expected behavior from the migen description.
With blocking assignments, c is stuck at 0 in simulation and synthesis.

[sbourdeauducq]

Actually, we should probably drop asic_syntax since this behavior is clearly incorrect.

[dvc94ch]

Is there a way to fix this these cases or are we stuck forever with the python simulator?

[sbourdeauducq]

Yes, transform the code as I explained above; but it's not simple. It is definitely doable though, as synthesizers do it internally.

I suspect that the best way out of this mess is to have Migen transform the output code so that every signal takes its final value at the end of the comb block - no iteration on comb blocks like it can often happen now. This transform should make external simulators behave and probably give a nice speed boost to the (very slow) integrated one.

[dvc94ch]

I see:

// blocking
/* Generated by Yosys 0.7+631 (git sha1 93efbd5d, clang 6.0.1 -fPIC -Os) */

(* src = "if_reset.v:3" *)
module top(clk, a, b, c);
  (* src = "if_reset.v:25" *)
  wire _0_;
  (* init = 1'h1 *)
  (* src = "if_reset.v:3" *)
  output a;
  reg a = 1'h1;
  (* src = "if_reset.v:3" *)
  output b;
  (* src = "if_reset.v:3" *)
  output c;
  (* src = "if_reset.v:3" *)
  input clk;
  assign _0_ = ~ (* src = "if_reset.v:26" *) a;
  always @(posedge clk)
      a <= _0_;
  assign b = a ? (* src = "if_reset.v:15" *) 1'h1 : 1'h0;
  assign c = 1'h0;
endmodule

// non-blocking
/* Generated by Yosys 0.7+631 (git sha1 93efbd5d, clang 6.0.1 -fPIC -Os) */

(* src = "if_reset.v:3" *)
module top(clk, a, b, c);
  (* src = "if_reset.v:25" *)
  wire _0_;
  (* init = 1'h1 *)
  (* src = "if_reset.v:3" *)
  output a;
  reg a = 1'h1;
  (* src = "if_reset.v:3" *)
  output b;
  (* src = "if_reset.v:3" *)
  output c;
  (* src = "if_reset.v:3" *)
  input clk;
  assign _0_ = ~ (* src = "if_reset.v:26" *) a;
  always @(posedge clk)
      a <= _0_;
  assign b = a ? (* src = "if_reset.v:15" *) 1'h1 : 1'h0;
  assign c = b ? (* src = "if_reset.v:14" *) 1'h1 : 1'h0;
endmodule

I'll have a look at how yosys does it.

Output regs should be initialized. So what is the status of this PR? Or do you prefer to revert commit c0fb0ef?

[sbourdeauducq]

There are (too) many things in this PR, what are you asking about exactly? Register initialization (sync ones) is independent from blocking/non-blocking assignments in comb blocks.

[dvc94ch]

@sbourdeauducq I dropped the other commits, I forgot to mention.

[enjoy-digital]

I would also like to solve the incompatibilities with generated code/tools (if possible in a generic way).
I'm using hacks on my side to be able to simulate the generated code: https://github.com/enjoy-digital/litex/blob/master/litex/gen/fhdl/verilog.py:

• split async_syntax to have better control on the generated code.
• added a (dirty) patch to prevent the the combinatorial loop (generate the code with regular_comb=False)
  We can try to solves these issues one by one.
  For the initial change, if working on hardware and simulation, i'm ok with it.

[sbourdeauducq]

How does it prevent the combinatorial loops?

[enjoy-digital]

I'm not the author (davidcarne) but it's doing:

• nonblocking -> blocking assigns for combinatorial logic
• splitting combinatorial blocks to prevent cyclic dependencies
  With this, i'm able to simulate big generated designs without trouble with xsim, modelsim, etc...

[sbourdeauducq]

But isn't it generating each statement many times?

[enjoy-digital]

Yes, here is the same design with and without it:
https://gist.github.com/enjoy-digital/ab2fffb01ae838eaa62c60458e43fd72#file-top_regular_comb_false-v
https://gist.github.com/enjoy-digital/ab2fffb01ae838eaa62c60458e43fd72#file-top_regular_comb_true-v