Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

else branch elided in generated Verilog #274

Open
McSherry opened this issue Jun 22, 2023 · 3 comments
Open

else branch elided in generated Verilog #274

McSherry opened this issue Jun 22, 2023 · 3 comments

Comments

@McSherry
Copy link

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
Copy link
Member

sbourdeauducq commented Jun 23, 2023

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
Copy link
Member

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants