m-labs / migen

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

`else` branch elided in generated Verilog #274

Open McSherry opened 1 year ago

McSherry commented 1 year ago

I have the following in a Migen module:

self.comb += [
    # Always valid
    self.tx.tvalid_i.eq(1),

    # Throw away receiver data
    self.rx.tready_i.eq(1),

    # Clear an error if one occurs
    self.rx.error_clear_i.eq(self.rx.error_o),

    # If we get input from our RTIO interface and our receiver is locked, write
    # it to the transmitter. Otherwise, just continually write /IDLE/.
    If(self.rtlink.o.stb & self.rx.lock_o,
        # D-code if address is 0xDC, else K-code
        If(self.rtlink.o.address == 0xDC, self.tx.tid_i.eq(0)
        ).Else(self.tx.tid_i.eq(1)),

        # Connect RTIO data directly to transmitter
        self.tx.tdata_i.eq(self.rtlink.o.data),
    ).Else(
        self.tx.tid_i.eq(1),
        self.tx.tdata_i.eq(0b0010_0010)
    ),
]

The important part is the If at the bottom. All the inputs to self.rx and self.tx are registered within those modules.

When translated to Verilog, this module produces the following output:

always @(*) begin
    ustransmitter_tid_i <= 1'd0;
    ustransmitter_tdata_i <= 8'd0;
    if ((ointerface_stb & usreceiver_lock_o)) begin
        if ((ointerface_address == 8'd220)) begin
            ustransmitter_tid_i <= 1'd0;
        end else begin
            ustransmitter_tid_i <= 1'd1;
        end
        ustransmitter_tdata_i <= ointerface_data;
    end else begin
        ustransmitter_tdata_i <= 6'd34;
    end
// synthesis translate_off
    dummy_d_84 <= dummy_s;
// synthesis translate_on
end

This is wrong—the assignment self.tx.tid_i.eq(1) in the .Else() has been elided, which produces different behaviour (self.tx.tid_i now has the value '0 when there is no RTIO input instead of 1'b1). I'm also suspicious of the generated Verilog using the nonblocking assignment when it should be combinatorial.

sbourdeauducq commented 1 year ago

I'm also suspicious of the generated Verilog using the nonblocking assignment when it should be combinatorial.

Blocking vs. nonblocking has nothing to do with sequential vs. combinatorial. Verilog is a simulation-first language and the simulator has no notion of sequential/combinatorial; only events, delays, and delta cycles. Non-blocking just assigns at the next delta cycle unlike blocking which assigns immediately. Both blocking and nonblocking can be used to model combinatorial logic; you can have several delta cycles in a combinatorial path.

sbourdeauducq commented 1 year ago

This is wrong—the assignment self.tx.tid_i.eq(1) in the .Else() has been elided,

Please provide a complete self-contained minimal piece of code that demonstrates the bug. I suspect the problem is somewhere else in your codebase.

McSherry commented 1 year ago

I can't readily produce it out of context, although this module on its own is simple—it contains only instantiations of an ARTIQ rtlink and two Modules that wrap Instances, e.g.:

class USTransmitter(Module):
    def __init__(
        self,
        refclk: Tuple[Signal, Signal],
        txd0: Tuple[Signal, Signal],
        txd1: Tuple[Signal, Signal],
        txp: Tuple[Signal, Signal],
    ):
        self.tready_o = Signal(reset_less=True)
        self.tvalid_i = Signal.like(self.tready_o)
        self.tid_i = Signal.like(self.tready_o)
        self.tdata_i = Signal(8, reset_less=True)
        self.error_o = Signal(3, reset_less=True)

        (refclk_p, refclk_n) = refclk
        (txd0_p, txd0_n) = txd0
        (txd1_p, txd1_n) = txd1
        (txp_p, txp_n) = txp
        self.specials += [
            Instance(
                "phy_tx_upstream",
                i_clk_i=ClockSignal(),
                i_rst_ni=~ResetSignal(),
                o_axis_tready_o=self.tready_o,
                i_axis_tvalid_i=self.tvalid_i,
                i_axis_tid_i=self.tid_i,
                i_axis_tdata_i=self.tdata_i,
                o_error_o=self.error_o,
                i_clk_ui_i=ClockSignal("ui"),
                o_refclk_po=refclk_p,
                o_refclk_no=refclk_n,
                o_txd0_po=txd0_p,
                o_txd0_no=txd0_n,
                o_txd1_po=txd1_p,
                o_txd1_no=txd1_n,
                o_txp_po=txp_p,
                o_txp_no=txp_n,
            ),
        ]

The generated Verilog becomes correct if the If in the original post is replaced with:

self.tx.tid_i.eq(1),
self.tx.tdata_i.eq(0b0010_0010),

If(self.rtlink.o.stb & self.rx.lock_o,
    self.tx.tid_i.eq(self.rtlink.o.address != 0xDC),

    self.tx.tdata_i.eq(self.rtlink.o.data),
),