migen icon indicating copy to clipboard operation
migen copied to clipboard

`else` branch elided in generated Verilog

Open McSherry opened this issue 1 year ago • 3 comments

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.

McSherry avatar Jun 22 '23 14:06 McSherry

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 avatar Jun 23 '23 01:06 sbourdeauducq

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.

sbourdeauducq avatar Jun 23 '23 01:06 sbourdeauducq

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),
),

McSherry avatar Jun 23 '23 09:06 McSherry