CODEWITHSUNDEEP
if-statementsystem-verilog
George Waller
George Waller

Reputation: 105

SystemVerilog 'if' statement inside always_comb 'not purely combinational logic' error

I'm confused! And a bit frustrated. I've spent quite a lot of time working on some SystemVerilog in Modelsim. I got it to a certain stage where I could test it on my hardware however compiling in Quartus was unsuccessful. I did understand this may happen but in this case my errors don't seem to make sense.

The code below is inside an always_comb block. When I compile I get the following error:

Error (10166): SystemVerilog RTL Coding error at fifo_interface.sv(80): always_comb construct does not infer purely combinational logic.

I really don't understand this. This is the code, it's just a mux.

always_comb
    if(fifo_select == 0)
    begin
        fifo0_data_in = data_in;
        fifo0_in_clk = in_clk;
        fifo0_in_dn = in_dn;
        in_rdy = fifo0_in_rdy;

        fifo1_in_clk = 0;   //Prevent 'in_clk' entering fifo1
    end

    else
    begin
        if(fifo_select == 1)
        begin
            fifo1_data_in = data_in;
            fifo1_in_dn = in_dn;
            fifo1_in_clk = in_clk;
            in_rdy = fifo1_in_rdy;

            fifo0_in_clk = 0;   //Prevent 'in_clk' entering fifo0
        end 
    end
end

When I change the block to type 'always' Modelsim will act strangely. It will break in the code or will crash altogether with the exit code 211. Changing the type back to 'always_comb' does not fix the issue and so I have to restart modelsim to successfully simulate the HDL.

I'm interested to know what the source of the error is?

Thanks for any help.

Upvotes: 4

Views: 24482

Answers (2)

Paul S
Paul S

Reputation: 7755

The answer by nguthrie is right, but you've tripped up over something that's quite important to understand, so I wanted to write a more explicit version.

If we take your code, and simplify it so we're just looking at the assignments to fifo0_data_in it'll be easier to talk about.

always_comb
begin
    if(fifo_select == 0)
    begin
        fifo0_data_in = data_in;
    end
end

So, when fifo_select is zero we assign to fifo0_data_in, when fifo_select isn't zero, we.... errr... do nothing, which means that it needs to keep it's previous value.

What we've described is a "latch", and latches aren't combinatorial (they retain information) so they can't exist in an always_comb. They would have to be in an always_latch or always block.

Unintended latches are a really common bug when using always blocks, and in general you never want them. SystemVerilog added always_comb so that errors like this told the designer when they'd forgotten to define a value for a signal in a code path (in your case, when fifo_select is not zero).

An alternative way to fix this, and a style that's worth being familiar with, is:

always_comb
begin
    fifo0_data_in = '0;  // Default value

    if(fifo_select == 0)
    begin
        fifo0_data_in = data_in;
    end
end

Here we assign a default value at the start, so regardless if the if condition is true or not, something has been assigned to the signal. This style is often easier is there are lots of complex conditions which control the signal.

Upvotes: 2

nguthrie
nguthrie

Reputation: 2685

You are not assigning all outputs in all branches of the mux. For example, fifo0_data_in gets assigned if fifo_select == 0. But if fifo_select == 1 then it gets no value. This implies that fifo0_data_in needs to remember what its value was if fifo_select changes. Therefore, synthesis will then infer a latch for this output.

Here is what I think you want:

always_comb
    if(fifo_select == 0)
    begin
        fifo0_data_in = data_in;
        fifo1_data_in = '0;
        fifo0_in_clk = in_clk;
        fifo1_in_clk = '0;
        fifo0_in_dn = in_dn;
        fifo1_in_dn = '0;
        in_rdy = fifo0_in_rdy;
    end

    else
    begin
        fifo1_data_in = data_in;
        fifo0_data_in = '0;
        fifo1_in_clk = in_clk;
        fifo0_in_clk = '0;
        fifo1_in_dn = in_dn;
        fifo0_in_dn = '0;
        in_rdy = fifo1_in_rdy;
    end
end

And, since you appear to be gating the clock to the unused FIFO you could simplify further as shown below:

assign fifo0_data_in = data_in;
assign fifo1_data_in = data_in;
assign fifo0_in_dn = in_dn;
assign fifo1_in_dn = in_dn;

always_comb
    if(fifo_select == 0)
    begin
        fifo0_in_clk = in_clk;
        fifo1_in_clk = '0;
        in_rdy = fifo0_in_rdy;
    end

    else
    begin
        fifo0_in_clk = '0;
        fifo1_in_clk = in_clk;
        in_rdy = fifo1_in_rdy;
    end
end

Upvotes: 7

Related Questions