Is this "legal" in Verilog?

[betocool]

Hi all, I have this bit of code that runs ok when I simulate it with Verilator. However, I'm having issues compiling it with Symbiflow... so maybe it's a case of "oh... you're not allowed to do that" sort of thing. I'm not very fluent in Verilog, more of a VHDL person, but trying things out at the moment:

Code: [Select]

always @(posedge clk) begin

    if(!resetn) begin
      cfg_reg <= 32'h 0000_0000;
      presc_reg <= 32'h 0000_0000;
      cnt_reg <= 32'h 0000_0000;
      presc_cnt <= 32'h 0000_0000;
    end else begin

      if (reg_we != 4'b 0000) begin

        case(reg_addr)
          4'b 0000: cfg_reg <= reg_di;
          4'b 0001: presc_reg <= reg_di;
          4'b 0010: cnt_reg <= reg_di;
        endcase

        wr_ready <= 1;
       
      end else begin
        wr_ready <= 0;
      end

      if(cfg_reg == 32'h 0000_0001) begin

        if (presc_cnt == presc_reg) begin
          presc_cnt <= 0;
        end else begin
          presc_cnt <= presc_cnt + 1;
        end

        if(presc_reg == 0) begin
          cnt_reg <= cnt_reg + 1;
        end else begin
          if (presc_reg == presc_cnt) begin
            cnt_reg <= cnt_reg + 1;
          end
        end

      end
    end
  end

The question is regarding if I can use a "case" statement within a clocked process... Like I said, Verilator accepts it...

Cheers,

Alberto

[Bassman59]

Of course you can use a case within a clocked block!

Symbiflow's parser may be confused. It wouldn't be the first time that a front-end parser chokes on legal language structures.

[Someone]

Things in that code making it harder for the tools:
if/else reset (avoid resets for FPGA targets)
case statement inside a wrapping if, nothing on the else
partial decode/no default case

Its all "legal" but can lead to sub optimal results.

Given what you are doing in that case statement, it would be cleaner for the tools to have them as 3 discrete if statements.

[SiliconWizard]

Hi all, I have this bit of code that runs ok when I simulate it with Verilator. However, I'm having issues compiling it with Symbiflow...

Maybe you could tell us what the issues are.

[Cerebus]

If we take out all the unecessary begin..end blocks wrapped around single statements the code becomes a lot clearer and we can actually see it.

Code: [Select]

always @(posedge clk) begin

    if(!resetn) begin
      cfg_reg <= 32'h 0000_0000;
      presc_reg <= 32'h 0000_0000;
      cnt_reg <= 32'h 0000_0000;
      presc_cnt <= 32'h 0000_0000;
    end
    else
    begin

      if (reg_we != 4'b 0000)
      begin

        case(reg_addr)
          4'b 0000: cfg_reg <= reg_di;
          4'b 0001: presc_reg <= reg_di;
          4'b 0010: cnt_reg <= reg_di;
        endcase

        wr_ready <= 1;
       
      end
      else
        wr_ready <= 0;

      if(cfg_reg == 32'h 0000_0001)
      begin

        if (presc_cnt == presc_reg)
          presc_cnt <= 0;
        else
          presc_cnt <= presc_cnt + 1;

        if(presc_reg == 0)
          cnt_reg <= cnt_reg + 1;
        else
          if (presc_reg == presc_cnt)
            cnt_reg <= cnt_reg + 1;

      end
    end
end

Then it become obvious where two problems are:

Code: [Select]

        if (presc_cnt == presc_reg)
          presc_cnt <= 0;
        else
          presc_cnt <= presc_cnt + 1;

        if(presc_reg == 0)
          cnt_reg <= cnt_reg + 1;
        else
          if (presc_reg == presc_cnt)
            cnt_reg <= cnt_reg + 1;


You need to factor out the redundant tests on the same condition if (presc_cnt == presc_reg) versus if (presc_reg == presc_cnt) - which is the same condition expressed backwards - and the redundant assignments of cnt_reg <= cnt_reg + 1; on both arms of the final if. So restructuring the final if that becomes:

Code: [Select]

        if (presc_cnt == presc_reg)
          presc_cnt <= 0;
        else
          presc_cnt <= presc_cnt + 1;

        if((presc_reg == 0) | (presc_cnt == presc_reg))
          cnt_reg <= cnt_reg + 1;

Which still leaves that double condition check there. It might be right, I haven't reverse engineered what your logic might be intended to be, but it has the feel of a "bad code smell" to me.

My guess at your intended logic suggests that what you wanted to write was:

Code: [Select]

        if((presc_reg == 0) | (presc_reg == presc_cnt))    // If the prescale factor is set to zero, or the prescale counter has hit the scale count
        begin
          cnt_reg <= cnt_reg + 1;    // Increment the actual counter
          presc_cnt <= 0;                // Clear the prescaler counter
        end
        else
          presc_cnt <= presc_cnt + 1;    // Increment the prescaler counter

You still have problems left, because there are two places, that can execute simultaneously, that both try to write to cnt_reg, the block that executes if (reg_we != 4'b 0000) and the block that executes if (cfg_reg == 32'h 0000_0001). Simulation will let you do that, synthesis won't. You need to make those two blocks mutually exclusive. You will have got an error message about the clash of assignments, it would have made things a lot easier if you'd included it rather than just handwaving 'so maybe it's a case of "oh... you're not allowed to do that" sort of thing'.

You also probably want to move wr_ready into your reset block rather than leaving it undefined until after the first execution of the if(reg_we != 4'b 0000) block.

[asmi]

You still have problems left, because there are two places, that can execute simultaneously, that both try to write to cnt_reg, the block that executes if (reg_we != 4'b 0000) and the block that executes if (cfg_reg == 32'h 0000_0001). Simulation will let you do that, synthesis won't. You need to make those two blocks mutually exclusive. You will have got an error message about the clash of assignments, it would have made things a lot easier if you'd included it rather than just handwaving 'so maybe it's a case of "oh... you're not allowed to do that" sort of thing'.

We've just had a discussion on this in another thread. Multiple assignments within a single always_* block are not only allowed, but such behavior is explicitly defined in the specification, namely, the last assignment in the code flow always wins, and so there is never a clash nor any ambiguity. This is very convenient in many cases for state machines, when you can assign "default" values in the beginning of the block, and later down the flow only reassign those signals which are supposed to have non-default values.

[Cerebus]

You still have problems left, because there are two places, that can execute simultaneously, that both try to write to cnt_reg, the block that executes if (reg_we != 4'b 0000) and the block that executes if (cfg_reg == 32'h 0000_0001). Simulation will let you do that, synthesis won't. You need to make those two blocks mutually exclusive. You will have got an error message about the clash of assignments, it would have made things a lot easier if you'd included it rather than just handwaving 'so maybe it's a case of "oh... you're not allowed to do that" sort of thing'.

We've just had a discussion on this in another thread. Multiple assignments within a single always_* block are not only allowed, but such behavior is explicitly defined in the specification, namely, the last assignment in the code flow always wins, and so there is never a clash nor any ambiguity. This is very convenient in many cases for state machines, when you can assign "default" values in the beginning of the block, and later down the flow only reassign those signals which are supposed to have non-default values.

My own experience has been that if you pile a load of defaults at the top an always there's no problem, the tools can figure out what you mean - in fact I typically do just that for the outputs from state machines. If your conflicting drivers are buried away behind a couple of conditionals that could both be true all hell breaks loose, well OK, you get an error message. Imagine it as "How would I build this physically out of multiplexers without using priority encoders" and you find you can't. If there's anything in there that looks like a priority encoder (e.g. nested ifs) then it sails through. it's only when it can't work out "who wins" on two simultaneous enables that it breaks, and although the standards might say that the lexically latest wins, some of the tools don't seem to have got that message.

Edit: Out of curiosity I went away and tested this out in Yosys on as simple a test case as I could.

Code: [Select]

module  Test (
    input wire clk,
    input wire enableA,
    input wire sourceA,
    input wire enableB,
    input wire sourceB
);
    reg destination;
   
    always @(posedge clk)
    begin
        if (enableA) destination <= sourceA;
        if (enableB) destination <= sourceB;
    end
endmodule

And I find to my surprise that it doesn't barf on it, and it superficially appears acts according to standards. I have definitely had problems with this in the past, but with exactly which synthesis engine I'm not now sure. I thought I had encountered it in Yosys, so either Yosys has changed or it was another tool, but either way my technical objection fails.

Presumably to do this Yosys has to synthesise a priority encoder. Checking exactly what it does synthesise is a job for another time, and when I've got a bit I'll come back to it and take a look.

So, I have to remove my objection to clashing sources on technical grounds, but I'm going to retain my objection on the grounds that code that relies on implicit subtleties is bad compared to code that explicitly states what will happen. That is

Code: [Select]

    always @(posedge clk)
    begin
        if (enableA) destination <= sourceA;
        if (enableB) destination <= sourceB;
    end

and

Code: [Select]

    always @(posedge clk)
    begin
        if (enableB) destination <= sourceB;
        else
            if (enableA) destination <= sourceA;
    end

ought to both do exactly the same thing, but the latter makes the coder's intention entirely clear, the former does not even though it will result in exactly the same behaviour.

2nd edit:

Here's what gets synthesised by Yosys without any optimisation, two cascaded multiplexers:

[betocool]

Hey all, thanks for the answers, it's getting a bit clearer now.

To the background, I've dabbled in FPGA's on and off since 2002, more off than on to be honest, and I'm giving Symbiflow a crack, as it seems like a good open source alternative. I mostly worked in VHDL, but Symbiflow only supports Verilog, so I'm kind of getting back to it. I could not for the life of me remember if the statement I asked about was "legal" or "formal'. Unfortunately I did not have any more information about the error message other than:

Code: [Select]

Error 1:
Type: Blif file
File: top.eblif
Line: 43998
Message: Failed to find matching architecture model for 'LDCE'

from the "symbiflow_pack" tool, that's why I didn't post a message with the question.

And fair enough, I didn't notice the problems with the same condition in two places, thanks for pointing that out.

I will now get my head around the "latches are bad" or "why some latches are bad" issue.

Cheers,

Alberto

[AndyC_772]

My own experience has been that if you pile a load of defaults at the top an always there's no problem, the tools can figure out what you mean

I'm a little surprised there's any ambiguity here at all; if code doesn't have an explicit, well defined meaning (however difficult for a human being to follow), then that's a major problem.

I use VHDL, and often structure code along the following lines. Suppose, for example, that a block of logic is used to perform a sequence of actions that occasionally yield a result that must be kept. That result is pushed into a FIFO, which is triggered by a signal 'fifo_we' having the value '1'.

Code: [Select]

fifo_we <= '0';

case state_variable is
when state_idle =>
<wait>
when state_busy =>
<do stuff>
when state_finished =>
fifo_data <= intermediate_result;
fifo_we <= '1';
end case;

if reset_signal = '1' then
fifo_we <= '0';
state_variable <= state_idle;
end if;

There could be very many states depending on the complexity of the system. Most don't generate a final result, so assigning a value to fifo_we in every state would be pointless clutter and repetition, and a potential source of errors (eg. what if one of the states fails to assign a value at all, then it takes the value it had last time round the loop, which was probably zero but not always).

Finally, we have a reset condition. In the reset condition, any work in progress is aborted and any result generated should be discarded. Putting this assignment at the end of the loop clearly, unambiguously indicates what changes should occur when a reset is asserted, without the need to test for the reset condition in every state individually. Again, by understanding the way the language is parsed, and the order in which assignments take priority over each other, we've achieved clarity, readability, and a reduced chance of error.

[asmi]

My own experience has been that if you pile a load of defaults at the top an always there's no problem, the tools can figure out what you mean - in fact I typically do just that for the outputs from state machines. If your conflicting drivers are buried away behind a couple of conditionals that could both be true all hell breaks loose, well OK, you get an error message. Imagine it as "How would I build this physically out of multiplexers without using priority encoders" and you find you can't. If there's anything in there that looks like a priority encoder (e.g. nested ifs) then it sails through. it's only when it can't work out "who wins" on two simultaneous enables that it breaks, and although the standards might say that the lexically latest wins, some of the tools don't seem to have got that message.

Can you please give us an example of a tool which does that? And what's the error message?
In all the time I used Verilog I have NEVER seen any issues with that, nor did I ever expect to as - like I said - this situation is covered in the standard.

[asmi]

I'm a little surprised there's any ambiguity here at all; if code doesn't have an explicit, well defined meaning (however difficult for a human being to follow), then that's a major problem.

There is no ambiguity at all, it's all defined by the standard.

[Bassman59]

Things in that code making it harder for the tools:
if/else reset (avoid resets for FPGA targets)

Why avoid resets? In some fabrics, an async reset (released synchronously) is required as the configuration mechanism doesn't ensure that registers have a defined initial state.

case statement inside a wrapping if, nothing on the else

If the process is synchronous, an else is not necessary -- the flip-flops will retain their current value if you do not assign to them.

[Cerebus]

My own experience has been that if you pile a load of defaults at the top an always there's no problem, the tools can figure out what you mean - in fact I typically do just that for the outputs from state machines. If your conflicting drivers are buried away behind a couple of conditionals that could both be true all hell breaks loose, well OK, you get an error message. Imagine it as "How would I build this physically out of multiplexers without using priority encoders" and you find you can't. If there's anything in there that looks like a priority encoder (e.g. nested ifs) then it sails through. it's only when it can't work out "who wins" on two simultaneous enables that it breaks, and although the standards might say that the lexically latest wins, some of the tools don't seem to have got that message.

Can you please give us an example of a tool which does that? And what's the error message?
In all the time I used Verilog I have NEVER seen any issues with that, nor did I ever expect to as - like I said - this situation is covered in the standard.

See my edit above.

[free_electron]

I'm a little surprised there's any ambiguity here at all; if code doesn't have an explicit, well defined meaning (however difficult for a human being to follow), then that's a major problem.

There is no ambiguity at all, it's all defined by the standard.

correct. this is called scheduled logic. you can do this in vhdl too.
the root of this lies in the way synthesizers build logic.  any 'IF' clause is nothing but a switch (multiplexer) which is pure combinatorial logic. Combinatorial logic can be minimized.
an example :
build me a counter then can reset, clear , preset to9 , parallel load , count up and down and has an enable.
RESET : set counter to 0
PRESET: set counter to 9
LOAD : load input
ENABLE : enable counting and only counting. reset,load preset are not affected by this
UPDOWN : if high : count up , else count down.

Code: [Select]

always_ff @(posedge clk) begin
cnt <=cnt;  // not strictly necessary ...
if ENABLE begin   // if we are enabled
  cnt <= cnt-1;    // we count down
  if UPDOWN cnt <=cnt+1; //unless UPDOWN is high , in which case we count up.
end
if (LOAD)  cnt <input;  // if load is high we parallel load.
if (PRESET) cnt <=9;  // you get the idea by now.
if (RESET) cnt<0;
end

the attached image shows the decision logic being built. anything before the actual register ( flipflops) is purely combinatorial and can be minimized with very high efficiency. ( in an FPGA this gets converted into a LUT  whereas in true silicon this becomes a logic cloud. )
Note that the adder/subtractor can actually get implemented as a rom ( if this is a 4 bit counter this becomes a simple look back/ahead mux )

[SiliconWizard]

Hey all, thanks for the answers, it's getting a bit clearer now.

To the background, I've dabbled in FPGA's on and off since 2002, more off than on to be honest, and I'm giving Symbiflow a crack, as it seems like a good open source alternative. I mostly worked in VHDL, but Symbiflow only supports Verilog, so I'm kind of getting back to it. I could not for the life of me remember if the statement I asked about was "legal" or "formal'. Unfortunately I did not have any more information about the error message other than:

Code: [Select]

Error 1:
Type: Blif file
File: top.eblif
Line: 43998
Message: Failed to find matching architecture model for 'LDCE'

from the "symbiflow_pack" tool, that's why I didn't post a message with the question.

Maybe you could have also told us what was the target FPGA? (Unless I missed it.)

The 'LDCE' primitive is a "Transparent Data Latch with Asynchronous Clear and Gate Enable" (Xilinx). It can be found in various forms and names on various FPGAs. Possibly some FPGAs do not support that, but that would sound odd.

While latches should be avoided in general in synchronous designs (and yes, that can be fixed on a RTL level), they can usually be implemented in many FPGAs, so the message you get from Symbiflow is a bit odd. So, could you tell us what your target is? And maybe you have incorrectly defined the target when running the toolchain? We need more info. (But sure, getting rid of latches in your code should "fix" this, just saying that maybe this error message is revealing yet another issue in your setup.)

[asmi]

See my edit above.

So there is nothing aside from subjective opinions regarding code style. Which is what I suspected in the first place.

BTW, in SystemVerilog you can enforce checking for uniquiness of if/else and case conditions byt employing "unique" qualifier ("unique if" or "unique case"). This will display a warning if synthesizer/simulator will determine that those conditions are in fact not mutually-exclusive. More importantly, it will remove any priority encoding logic which might normally be there as it makes no sense when all conditions are exclusive and so exactly one of them is true at any point of time, which can lead to lower resource utilization and higher Fmax.

[Cerebus]

See my edit above.

So there is nothing aside from subjective opinions regarding code style. Which is what I suspected in the first place.

Have you ever considered answering in these types of topics by trying to be helpful to the OP rather than going around being adversarial to everybody who is trying to help?

[asmi]

Have you ever considered answering in these types of topics by trying to be helpful to the OP rather than going around being adversarial to everybody who is trying to help?

I'm not adversarial to anybody, I'm simply pointing out factual errors whenever I notice them. That's all there is to it.

It's important to differentiate opinions from facts, and avoid representing the former as the latter. Opinions can be debated and are often subjective and preferential, but facts are just what they are, and the concept of right and wrong applies to them. This is especially so when talking to beginners because they lack knowledge and experience to tell them apart.

[Cerebus]

Have you ever considered answering in these types of topics by trying to be helpful to the OP rather than going around being adversarial to everybody who is trying to help?

I'm not adversarial to anybody, I'm simply pointing out factual errors whenever I notice them. That's all there is to it.

It's important to differentiate opinions from facts, and avoid representing the former as the latter. Opinions can be debated and are often subjective and preferential, but facts are just what they are, and the concept of right and wrong applies to them. This is especially so when talking to beginners because they lack knowledge and experience to tell them apart.

It's a matter of tone. If someone goes and updates a post to correct themselves it is both unnecessary and confrontational to follow that up with the equivalent of "Told you so". That's not about the facts, it's not even debating opinion, it's just chest thumping.

[asmi]

It's a matter of tone. If someone goes and updates a post to correct themselves it is both unnecessary and confrontational to follow that up with the equivalent of "Told you so". That's not about the facts, it's not even debating opinion, it's just chest thumping.

That's not what I meant, but - OK - I will try to do better to avoid misunderstandings in the future. Let's leave it at that.

[betocool]

Maybe you could have also told us what was the target FPGA? (Unless I missed it.)

The 'LDCE' primitive is a "Transparent Data Latch with Asynchronous Clear and Gate Enable" (Xilinx). It can be found in various forms and names on various FPGAs. Possibly some FPGAs do not support that, but that would sound odd.

While latches should be avoided in general in synchronous designs (and yes, that can be fixed on a RTL level), they can usually be implemented in many FPGAs, so the message you get from Symbiflow is a bit odd. So, could you tell us what your target is? And maybe you have incorrectly defined the target when running the toolchain? We need more info. (But sure, getting rid of latches in your code should "fix" this, just saying that maybe this error message is revealing yet another issue in your setup.)

Sorry, yes, it's an Artix 7 board. The toolchain definitions were OK, I tested this board on other examples and it works fine. The definitions are from a common makefile, so that doesn't change.

The folks at the symbiflow chat also told me about removing the latches, so I changed the code into different sections for each register that holds data or a counter (by the way, this is a simple counter for a picosoc RV5 I'm trying things on). It generated the bitstream correctly, simulation worked, and the compiled firmware worked too. Thanks for all the input.

Code: [Select]

always @(posedge clk) begin
    if((!resetn) || (presc_reg == 32'h 0000_0000))
      presc_cnt <= 32'h 0000_0000;
      presc_clk <= 0;
    else
      if(presc_cnt == presc_reg)
        presc_cnt <= 32'h 0000_0000;
        presc_clk <= 1;
      else
        presc_cnt <= presc_cnt + 1;
        presc_clk <= 0;
      end
    end
  end

  assign cnt_clk = (presc_reg == 0) ? clk : presc_clk;

  always @(posedge cnt_clk) begin
    if(!resetn) begin
      cnt_reg <= 32'h 0000_0000;
      cnt_wr_ready <= 0;
    else
      if((reg_we != 4'b 0000) && (reg_addr == 4'b 0010))
        cnt_reg <= reg_di;
        cnt_wr_ready <= 1;
      else
        if(cfg_reg[0] == 1)
          cnt_reg <= cnt_reg + 1;
        end
        cnt_wr_ready <= 0;
      end
    end
  end

Ok, here's another question, something I wasn't aware of. Do I understand correctly that here:

Code: [Select]

  cnt <= cnt-1;    // we count down
  if UPDOWN cnt <=cnt+1; //unless UPDOWN is high , in which case we count up.

the "if" statement takes precedence (preference?) over the statement above?

Cheers,

Alberto

[free_electron]

Ok, here's another question, something I wasn't aware of. Do I understand correctly that here:

Code: [Select]

  cnt <= cnt-1;    // we count down
  if UPDOWN cnt <=cnt+1; //unless UPDOWN is high , in which case we count up.

the "if" statement takes precedence (preference?) over the statement above?

Cheers,

Alberto

it is not because it is an 'if' statement. it's because it sits lower in the begin-end block.

"statements shall be executed in the order given". since it sits closer to the 'exit' it has priority. Like i explained : the synthesizer builds logic as it reads statements (in the order given)

counter <= counter;   : the synthesizer makes a register and wires the output to the input.
if (ENABLED) counter <= counter +1;   : the synthesizer detached what was already connected to the input of the register , injects a mux under control of 'ENABLE'. what was attached goes to the '0' state of the mux , the new clause is attached to the '1' state
if(RESET) counter <=0; : synthesizer detaches again and does the mux thing again

[Bassman59]

Ok, here's another question, something I wasn't aware of. Do I understand correctly that here:

Code: [Select]

  cnt <= cnt-1;    // we count down
  if UPDOWN cnt <=cnt+1; //unless UPDOWN is high , in which case we count up.

the "if" statement takes precedence (preference?) over the statement above?

The thing to remember, and it's been said in this thread a few times, is that in an always block, the statements are evaluated in the order you typed them, and the last assignment to any left-hand-side signal wins.

So in this example, first you see the down counter, and then you are asked "is UPDOWN true?" and if so, then the up-counter statement is evaluated.

This is the equivalent of

Code: [Select]

if UPDOWN
    cnt <= cnt + 1;
else
    cnt <= cnt - 1;
and the tools (synthesis, simulation) are smart enough to do the right thing.

Which idiom should you use? I always opt for the one which is more obvious to the casual reader. I really don't like to use "tricks."

[free_electron]

Which idiom should you use? I always opt for the one which is more obvious to the casual reader. I really don't like to use "tricks."

These are not tricks. it is part of the language definition.

If you are making complex logic this method makes things a lot easier. Nested if-then-else spaghetti can be very difficult to read and even more complicated to modify afterwards.
"oh we wanted preset to work only during enable and parallel load should have precedence over reset..."
Simply move the lines of code.

when reading such code you can build the functionality in your mind. Precedence is very clear. ( if it comes earlier it is lower)

there are other advantages :
no hidden or missing pathways/conditions. no risk of latch inferring. less logic, guaranteed clock dependence, no asynchronous stuff

[Someone]

Things in that code making it harder for the tools:
if/else reset (avoid resets for FPGA targets)

Why avoid resets? In some fabrics, an async reset (released synchronously) is required as the configuration mechanism doesn't ensure that registers have a defined initial state.

So use a reset if you need one! If you dont need one then dont use one as its just a waste of time and resources. The particular example the OP posted is a register interface endpoint, exactly the sort of situation where a cargo-cult "reset everything" is unlikely required or desirable. Way too many code snippets just include (global) resets when they are rarely needed.

But if you do use a reset, don't use an if/else block unless you want to maintain piles of redundant cases. People further into the thread start discussing these finer points.

case statement inside a wrapping if, nothing on the else

If the process is synchronous, an else is not necessary -- the flip-flops will retain their current value if you do not assign to them.

They sure will, but again why pick such a convoluted control process if its only going to make reading it harder, and the tools will have a harder time putting it together? Not everything ends up as muxes as the above posters are suggesting, the same functionality can be from register enables. As I said, its all permissible and functional but not the clearest way to do things. So whats your point?