Migen outputs incorrect verilog in specific use-case

[rohitk-singh]

I wrote a Migen code which used MultiReg for signal synchronization across clock domains and the relevant section is similar to the test code below:

from migen import *
from migen.fhdl import verilog
from migen.genlib.cdc import MultiReg

class Test(Module):
    def __init__(self, n):
        counter = Signal(8)
        done = Signal(n)
        regs = []

        for i in range(n):
            regs.append(MultiReg(counter[i], done[i]))

        self.specials += regs
        self.sync.rx += counter.eq(counter + 1)

print(verilog.convert(Test(2)))

The verilog output of this code is:

Incorrect Code Section

wire [1:0] done;
always @(*) begin
	done <= 2'd0;
	done[0] <= multiregimpl01;
	done[1] <= multiregimpl11;
end

Complete generated verilog file (cleaned up) :

module top(
	input rx_clk,
	input rx_rst,
	input sys_clk,
	input sys_rst
);


reg [7:0] counter = 8'd0;
wire [1:0] done;
(* no_retiming = "true" *) reg multiregimpl00 = 1'd0;
(* no_retiming = "true" *) reg multiregimpl01 = 1'd0;
(* no_retiming = "true" *) reg multiregimpl10 = 1'd0;
(* no_retiming = "true" *) reg multiregimpl11 = 1'd0;

always @(*) begin
	done <= 2'd0;
	done[0] <= multiregimpl01;
	done[1] <= multiregimpl11;
end

always @(posedge rx_clk) begin
	counter <= (counter + 1'd1);
	if (rx_rst) begin
		counter <= 8'd0;
	end
end

always @(posedge sys_clk) begin
	multiregimpl00 <= counter[0];
	multiregimpl01 <= multiregimpl00;
	multiregimpl10 <= counter[1];
	multiregimpl11 <= multiregimpl10;
end

endmodule

Issue

The problem with above generated verilog is that done[1:0] is declared as wire instead of being declared as reg since it is being assigned inside the always block.

It might be fault of the coding style used in the above test code, but creating object with Test(1) instead of Test(2) gives correct output: https://hastebin.com/jiwomizawe.scala

Also, if I refactor the code to this:

from migen import *
from migen.fhdl import verilog
from migen.genlib.cdc import MultiReg

class Test(Module):
    def __init__(self, n):
        counter = Signal(8)
        done = []
        regs = []

        for i in range(n):
            done.append(Signal(name='done{}'.format(i)))
            regs.append(MultiReg(counter[i], done[i]))

        self.specials += regs
        self.sync.rx += counter.eq(counter + 1)

print(verilog.convert(Test(2)))

then I get correct verilog output -> https://hastebin.com/bujarijeqe.scala

I can definitely use the refactored code to get intended output but just wanted to confirm whether this is a bug or not.

[sbourdeauducq]

Yes it's a bug; can you patch it, test it carefully, and send a PR?

[rohitk-singh]

Sure, let me give it a try.

[rohitk-singh]

@sbourdeauducq, @jordens and @whitequark: Could you please review this commit: rohitk-singh@b3206e2

It seems to solve the above issue.

[sbourdeauducq]

If the way to solve it is to allow special outputs to be reg, then the comb assign in MultiRegImpl and maybe other places (please check) becomes unnecessary.

[gsomlo]

sort-of related question: in general, it is considered "good verilog" to use only blocking assignments in combinational (e.g., "always @(*)" blocks, and I noticed that migen uses nonblocking "<=" throughout all such blocks. I wonder if simply generating "="s instead of "<="s for combinational (note: NOT synchronous) blocks wouldn't be a better fix?
EDIT: nevermind, I notice this is already being discussed in a few other migen PRs; sorry for the noise :)

[whitequark]

Triage: fixed in nMigen.