migen icon indicating copy to clipboard operation
migen copied to clipboard

assignment of combinatorial values with delayed assignment gives incorrect results when referencing same signal

Open jwise opened this issue 9 months ago • 3 comments

I designed a small priority encoder in a LiteX framework, and found that the post-synthesis results were not as I had specified in my Migen input. I specified the following:

class Fx3Mux(LiteXModule):
    def __init__(self, platform, inputs = [ 2048, 512, 2048, 512 ], pktdepth = 4):
        # ...
        inp_available = Signal(len(inputs))
        inp_priority = Signal(len(inputs))
        # ...
        for input,depth in enumerate(inputs):
            # ... set up pktlenfifo ...
            self.comb += inp_available[input].eq(pktlenfifo.source.valid)
            if input == 0:
                self.comb += inp_priority[input].eq(pktlenfifo.source.valid)
            else:
                self.comb += inp_priority[input].eq(pktlenfifo.source.valid & ~inp_priority[input-1])
        # ...

This synthesized down to:

always @(*) begin
    mux_inp_available <= 4'd0;
    mux_inp_available[0] <= mux_syncfifo0_source_valid1;
    mux_inp_available[1] <= mux_syncfifo1_source_valid1;
    mux_inp_available[2] <= mux_syncfifo2_source_valid1;
    mux_inp_available[3] <= mux_syncfifo3_source_valid1;
end
always @(*) begin
    mux_inp_priority <= 4'd0;
    mux_inp_priority[0] <= mux_syncfifo0_source_valid1;
    mux_inp_priority[1] <= (mux_syncfifo1_source_valid1 & (~mux_inp_priority[0]));
    mux_inp_priority[2] <= (mux_syncfifo2_source_valid1 & (~mux_inp_priority[1]));
    mux_inp_priority[3] <= (mux_syncfifo3_source_valid1 & (~mux_inp_priority[2]));
end

This is not correct, and indeed, in hardware, post-synthesis on Efinix, my gateware periodically had a 5 show up in inp_priority if two sources were ready at the same moment.

This is clearly illegal because the output of that always block would cause that block to retrigger; in an event-based model, this would loop forever, so it does not have a meaningful synthesis equivalent (even though it looks like it should!). This would be fixed either by continuous assignment, or by splitting each into its own always block (note that leaving the default would infer a latch, though!).

jwise avatar Mar 11 '25 03:03 jwise

I should say also that fixing it in this way seems to work:

        # migen's obsession with delayed-assigns gets this priority encoder wrong!
        # inp_priority = Signal(len(inputs))
        inp_priority = [Signal(name=f"inp_priority_{n}") for n,_ in enumerate(inputs)]

jwise avatar Mar 11 '25 03:03 jwise

No, this should be fixed by outputting VHDL. This sort of issue is basically intractable in Verilog and your suggestions are naive.

But in practice, most synthesizers do not have an issue with the Verilog code as generated above, and Migen is designed for these. See if you can get the Efinix compiler to behave itself.

sbourdeauducq avatar Mar 11 '25 03:03 sbourdeauducq

Migen's "obsession" is there for good reason, see https://www.sigasi.com/opinion/jan/vhdls-crown-jewel/ - and this isn't the only problem of this kind that Verilog has.

sbourdeauducq avatar Mar 11 '25 03:03 sbourdeauducq