m-labs / migen

A Python toolbox for building complex digital hardware
https://m-labs.hk/migen
Other
1.19k stars 209 forks source link

Assignment to slices, #56

Closed benreynwar closed 7 years ago

benreynwar commented 7 years ago

I've been mostly using VHDL, so I'm in the habit of assigning to slices which is causing me some issues with migen.

class Minimal(Module):

    def __init__(self):
        # Ports  
        i = Signal()
        o = Signal(2)
        self.ios = {i, o}
        self.comb += (o[0].eq(i))
        self.comb += (o[1].eq(~i))

produces the following verilog

/* Machine-generated using Migen */
module top(
    input i,
    output reg [1:0] o
);

// synthesis translate_off
reg dummy_s;
initial dummy_s <= 1'd0;
// synthesis translate_on

// synthesis translate_off
reg dummy_d;
// synthesis translate_on
always @(*) begin
    o <= 2'd0;
    o[0] <= i;
    o[1] <= (~i);
// synthesis translate_off
    dummy_d <= dummy_s;
// synthesis translate_on
end

endmodule

When simulated in Vivado this gets stuck in a loop. Removing "o <+ 2'd0" fixes the problem.

It feels like there are two issues there: 1) The unnecessary assigning of the reset value in the @(*) block. 2) The incorrect use of "reg" for the output rather than "wire".

sbourdeauducq commented 7 years ago

This is a bug in Vivado, not Migen.

sbourdeauducq commented 7 years ago

You feel that the reset assignment is unnecessary, and Migen should use wire/assign. Determining that in the general case requires examining all code paths and checking that all bits in the slice are assigned at all times when the always block is run. Synthesizers and simulators are better equipped for that than Migen. Further, it's Vivado's problem if it puts o into the sensitivity list or re-runs the block even when o's value hasn't actually changed.

sbourdeauducq commented 7 years ago

Try one of the free alternatives - Icarus, Verilator, etc.

benreynwar commented 7 years ago

Yeah, I agree you're right about the bug being in Vivado, and that the reset assignment is necessary.

But how about the use of "reg" rather than "wire"? The generated verilog would clearly be more human-readable then, and it wouldn't run afoul of the Vivado bug. I'm happy to have a crack at making this change myself, but perhaps there are issues I'm unaware of that make this more complicated than it appears.

I'm really liking what I've seen of migen so far (thanks for creating it!), but I will need to use the generated verilog with Vivado. I think that making the generated verilog as similar as possible to what a human would write has the advantage of making it more readable, and reduces the chances of triggering bugs in other tools.

sbourdeauducq commented 7 years ago

You can't use wire unless you do the code analysis I mentioned. And the Vivado synthesizer should be fine with the code you posted. Why do you want to use the Vivado simulator and add more code and complexity to Migen? There is no advantage to using it.

benreynwar commented 7 years ago

How about explicitly populating the sensitivity list? Would that be more reasonable?

And yeah, I could switch to using another simulator, and I probably should. But I'd still rather I could run it through the Vivado simulator too :), especially if it could be done with a minor change to migen.

sbourdeauducq commented 7 years ago

The most reasonable thing is Xilinx makes a minor change to fix Vivado.

benreynwar commented 7 years ago

OK. I just do it in my own branch, and won't issue a pull request :). Thanks for discussing it with me.