SpinalHDL icon indicating copy to clipboard operation
SpinalHDL copied to clipboard

Proper handling of blocking and non-blocking assigns in generated Verilog

Open daydreamcatcher opened this issue 4 years ago • 9 comments

I created the following code that takes a vector of inputs and counts the number of elements that surpass a threshold (100). The resulting verilog code uses non-blocking assigns -- and therefore won't evaluate correctly since the value of the signal is updated in the next simulator delta cycle. Therefore, a blocking assign should be used for clarity and proper handling by following tools like simulator and synthesizer.

Here my code snipped in SpinalHDL

//Hardware definition
class MyTopLevel extends Component {
  val io = new Bundle {
    val latch_enable = in  Bool
    val input_data = in Vec(UInt(16 bits), 4)
  }
  val larger_than_N = out UInt(8 bits)
  val larger_than_N_reg = Reg(UInt(8 bits)) init(0)

  val data_reg = Vec(Reg(UInt(16 bits)) init(0), 4)

  when(io.latch_enable){
    data_reg := io.input_data
    larger_than_N_reg := 0
    for(idx <- 0 to 3) {
      when (data_reg(idx) >= 100) {
        larger_than_N_reg := larger_than_N_reg + 1
      }
    }
  }
  larger_than_N := larger_than_N_reg
}
module MyTopLevel (
  input               io_latch_enable,
  input      [15:0]   io_input_data_0,
  input      [15:0]   io_input_data_1,
  input      [15:0]   io_input_data_2,
  input      [15:0]   io_input_data_3,
  output     [7:0]    larger_than_N,
  input               clk,
  input               reset
);
  reg        [7:0]    larger_than_N_reg;
  reg        [15:0]   data_reg_0;
  reg        [15:0]   data_reg_1;
  reg        [15:0]   data_reg_2;
  reg        [15:0]   data_reg_3;
  wire                when_MyTopLevel_l41;
  wire                when_MyTopLevel_l41_1;
  wire                when_MyTopLevel_l41_2;
  wire                when_MyTopLevel_l41_3;

  assign when_MyTopLevel_l41 = (16'h0064 <= data_reg_0);
  assign when_MyTopLevel_l41_1 = (16'h0064 <= data_reg_1);
  assign when_MyTopLevel_l41_2 = (16'h0064 <= data_reg_2);
  assign when_MyTopLevel_l41_3 = (16'h0064 <= data_reg_3);
  assign larger_than_N = larger_than_N_reg;
  always @(posedge clk or posedge reset) begin
    if(reset) begin
      larger_than_N_reg <= 8'h0;
      data_reg_0 <= 16'h0;
      data_reg_1 <= 16'h0;
      data_reg_2 <= 16'h0;
      data_reg_3 <= 16'h0;
    end else begin
      if(io_latch_enable) begin
        data_reg_0 <= io_input_data_0;
        data_reg_1 <= io_input_data_1;
        data_reg_2 <= io_input_data_2;
        data_reg_3 <= io_input_data_3;
        larger_than_N_reg <= 8'h0; // Should use blocking assign!!!
        if(when_MyTopLevel_l41) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
        if(when_MyTopLevel_l41_1) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
        if(when_MyTopLevel_l41_2) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
        if(when_MyTopLevel_l41_3) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
      end
    end
  end

endmodule

daydreamcatcher avatar Aug 02 '21 11:08 daydreamcatcher

Hi,

This is the expected behaviour, SpinalHDL kind of follow more a VHDL way of handling such things :

class MyTopLevel extends Component {
  val io = new Bundle {
    val latch_enable = in  Bool
    val input_data = in Vec(UInt(16 bits), 4)
  }
  val larger_than_N = out UInt(8 bits)
  var larger_than_N_var = UInt(8 bits) //This is a  scala var
  val larger_than_N_reg = Reg(UInt(8 bits)) init(0)

  val data_reg = Vec(Reg(UInt(16 bits)) init(0), 4)

  when(io.latch_enable){
    data_reg := io.input_data
    larger_than_N_reg := 0
    for(idx <- 0 to 3) {
      when (data_reg(idx) >= 100) {
        larger_than_N_var \= larger_than_N_var + 1 //Changed operator
      }
    }
  }
  larger_than_N := larger_than_N_reg
  larger_than_N := larger_than_N_var
}

Basicaly, the idea is to build the full graph of hardware "nodes" by creating new UInt instances (larger_than_N_var + 1) and refering to it later on the next iteration. I guess this should work as you expected. It will create a new signal at each iteration for larger_than_N_var

Dolu1990 avatar Aug 03 '21 07:08 Dolu1990

Ok, but wouldn't it be nice to hint the user with an error message, that the generated code is potentially faulty? -- Or determine the kind of operator used from the context?

daydreamcatcher avatar Aug 03 '21 07:08 daydreamcatcher

In some cases, codes structure like :

    for(idx <- 0 to 3) {
      when (data_reg(idx) >= 100) {
        larger_than_N_reg := ???
      }
    }

Isn't a faulty one, but the intended one. So far, SpinalHDL consider that multiple assignement to the same variable in the same conditional scope (fully overriding the previous value) is a faulty thing, but as soon there is some when statement, between the assignement, the described behaviour may make sense.

So, i would say, you code, even if it didn't implemented the behaviour you wanted, was still something legal to do.

Dolu1990 avatar Aug 03 '21 07:08 Dolu1990

Yeah, but only, if and only if there is exactly one condition in the loop, where the when-condition is true. Then, I would suggest to add some kind of check for that, as for instance a piece of verilog-code that executes the check and triggers an assertion if multiple assigns would occur. Also, this could be formally proved by e.g. yosys to get rid of possible mismatches. A synthesizer would ignore this code anyway so it would not harm the compilation result.

daydreamcatcher avatar Aug 03 '21 07:08 daydreamcatcher

I'm sorry, can you ilustrate an example of what should be catched ? I'm not sure i understand properly.

Dolu1990 avatar Aug 03 '21 08:08 Dolu1990

Just as an example, I use the code from above and insert some code & comments (sorry for the bad indent, i quickly wrote a mockup ;)):

module MyTopLevel (
  input               io_latch_enable,
  input      [15:0]   io_input_data_0,
  input      [15:0]   io_input_data_1,
  input      [15:0]   io_input_data_2,
  input      [15:0]   io_input_data_3,
  output     [7:0]    larger_than_N,
  input               clk,
  input               reset
);
  reg        [7:0]    larger_than_N_reg;
  reg        [15:0]   data_reg_0;
  reg        [15:0]   data_reg_1;
  reg        [15:0]   data_reg_2;
  reg        [15:0]   data_reg_3;
  wire                when_MyTopLevel_l41;
  wire                when_MyTopLevel_l41_1;
  wire                when_MyTopLevel_l41_2;
  wire                when_MyTopLevel_l41_3;

  assign when_MyTopLevel_l41 = (16'h0064 <= data_reg_0);
  assign when_MyTopLevel_l41_1 = (16'h0064 <= data_reg_1);
  assign when_MyTopLevel_l41_2 = (16'h0064 <= data_reg_2);
  assign when_MyTopLevel_l41_3 = (16'h0064 <= data_reg_3);
  assign larger_than_N = larger_than_N_reg;
  always @(posedge clk or posedge reset) begin
    if(reset) begin
      larger_than_N_reg <= 8'h0;
      data_reg_0 <= 16'h0;
      data_reg_1 <= 16'h0;
      data_reg_2 <= 16'h0;
      data_reg_3 <= 16'h0;
    end else begin
      if(io_latch_enable) begin
        data_reg_0 <= io_input_data_0;
        data_reg_1 <= io_input_data_1;
        data_reg_2 <= io_input_data_2;
        data_reg_3 <= io_input_data_3;
        larger_than_N_reg <= 8'h0; // Should use blocking assign!!!
        if(when_MyTopLevel_l41) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
        if(when_MyTopLevel_l41_1) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
        if(when_MyTopLevel_l41_2) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
        if(when_MyTopLevel_l41_3) begin
          larger_than_N_reg <= (larger_than_N_reg + 8'h01); // Should use blocking assign!!!
        end
      end
    end
  end

 // Checker code: 
wire when_MyTopLevel_l41_wires = {when_MyTopLevel_l41, when_MyTopLevel_l41_1, when_MyTopLevel_l41_2, when_MyTopLevel_l41_3};
int number_of_active_branches;

always @(posedge clk )
begin
number_of_active_branches = 0; 
for (i=0;i<4;i=i+1)
begin
 if (when_MyTopLevel_l41_wires[i])
 begin
   number_of_active_branches = number_of_active_branches  + 1;
 end
end 
if( number_of_active_branches > 1)
begin
 // Could also be realized as assertion, maybe put a link to this issue or corresponding doc
 $display("More than one branch is active, this might result in a mismatich! Please check the operators in your code"); 
end
end
end
endmodule

daydreamcatcher avatar Aug 03 '21 08:08 daydreamcatcher

Ahhhhhhhhhhh

So, do you mean you would like a addition in the spinal.lib, to have a thing which check of more than 1 bit is set in a array of bits ?

If yes, sure, it can be done ^^

Dolu1990 avatar Aug 04 '21 08:08 Dolu1990

Yes, I think that would at least help a newcommer like me to get the thing right

daydreamcatcher avatar Aug 04 '21 13:08 daydreamcatcher

Hooo, seems like there is already : spinal.lib.CountOne(xxx), i guess this is what you wanted right ?

Dolu1990 avatar Aug 05 '21 13:08 Dolu1990