Inaccurate switch-is verilog generation

[saahm]

Heyo,

I'm currently writing some basic SpinalHDL examples and encountered some case where the SpinalHDL description wont allow me to specify a default value/previous assignment for a wire. This causes the Verilog generation to be different than the SpinalHDL one (to a degree and edge case). I wanted to put it here to discuss if that is wanted behavior or not in SpinalHDL.

Consider following component (i havent made it "minimal" so forgive me for that but its not big anyway):

package tutorialExamples.comb

import spinal.core._
import spinal.lib._

object CombOperation extends SpinalEnum {
    val add, shift, sub, xor, map = newElement()
} 

case class ALUInterface() extends Bundle {
    val opA = in(UInt(32 bits))
    val opB = in(UInt(32 bits))
    val opC = in(UInt(4 bits))
    val resC = out(UInt(32 bits))
    val operation = in(CombOperation())
}

class CombCir extends Component {
    val io = new Bundle{
        val alu = ALUInterface()
        val k = in(Bits(8 bits))
        val u = out(Bits(8 bits))
    }

    val aluArea = new Area{
        val tempRes = UInt(32 bits)
        // tempRes := 0 // default value to not infer latch in case of incomplete mux
        switch(io.alu.operation){
            is(CombOperation.add){
                tempRes := io.alu.opA + io.alu.opB
            }
            is(CombOperation.shift){
                tempRes := io.alu.opA |<< io.alu.opC
            }
            is(CombOperation.sub){
                tempRes := (io.alu.opA - io.alu.opB)
            }
            is(CombOperation.xor){
                tempRes := io.alu.opA ^ io.alu.opB
            }
            is(CombOperation.map){
                tempRes := ((io.alu.opA ^ io.alu.opB) >> io.alu.opC) ^ 0x33ff11a1
            }
            // default{
            //     tempRes := 0
            // }
        }
    }

    io.alu.resC := aluArea.tempRes
    io.u(0) := io.k(1) & io.k(0)
    io.u(1) := io.k(1) ^ io.k(0)
    io.u(7 downto 2) := io.k(4 downto 2) ## io.k(7 downto 5)
}

object CombCir {
  def main(args: Array[String]) {
    SpinalVerilog(new CombCir)
  }
}

So the key thing here is that the CombOperation enum is not a power of 2 amount of elements (for the enums with 2^n many elements this probably doesnt apply).
With SpinalHDL 1.7.3 I cannot have either of the commented statements in the aluArea without SpinalHDL forbidding me to generate the Verilog.

In the current form SpinalHDL 1.7.3 generates following Verilog:

// Generator : SpinalHDL v1.7.3    git head : aeaeece704fe43c766e0d36a93f2ecbb8a9f2003
// Component : CombCir
// Git hash  : 8e8b22f6ec3191f0378aeced536ffcd14739dd33

`timescale 1ns/1ps

module CombCir (
  input      [31:0]   io_alu_opA,
  input      [31:0]   io_alu_opB,
  input      [3:0]    io_alu_opC,
  output     [31:0]   io_alu_resC,
  input      [2:0]    io_alu_operation,
  input      [7:0]    io_k,
  output reg [7:0]    io_u
);
  localparam CombOperation_add = 3'd0;
  localparam CombOperation_shift = 3'd1;
  localparam CombOperation_sub = 3'd2;
  localparam CombOperation_xor_1 = 3'd3;
  localparam CombOperation_map_1 = 3'd4;

  wire       [31:0]   _zz_aluArea_tempRes;
  reg        [31:0]   aluArea_tempRes;
  `ifndef SYNTHESIS
  reg [39:0] io_alu_operation_string;
  `endif


  assign _zz_aluArea_tempRes = ((io_alu_opA ^ io_alu_opB) >>> io_alu_opC);
  `ifndef SYNTHESIS
  always @(*) begin
    case(io_alu_operation)
      CombOperation_add : io_alu_operation_string = "add  ";
      CombOperation_shift : io_alu_operation_string = "shift";
      CombOperation_sub : io_alu_operation_string = "sub  ";
      CombOperation_xor_1 : io_alu_operation_string = "xor_1";
      CombOperation_map_1 : io_alu_operation_string = "map_1";
      default : io_alu_operation_string = "?????";
    endcase
  end
  `endif

  always @(*) begin
    case(io_alu_operation)
      CombOperation_add : begin
        aluArea_tempRes = (io_alu_opA + io_alu_opB);
      end
      CombOperation_shift : begin
        aluArea_tempRes = (io_alu_opA <<< io_alu_opC);
      end
      CombOperation_sub : begin
        aluArea_tempRes = (io_alu_opA - io_alu_opB);
      end
      CombOperation_xor_1 : begin
        aluArea_tempRes = (io_alu_opA ^ io_alu_opB);
      end
      default : begin
        aluArea_tempRes = (_zz_aluArea_tempRes ^ 32'h33ff11a1);
      end
    endcase
  end

  assign io_alu_resC = aluArea_tempRes;
  always @(*) begin
    io_u[0] = (io_k[1] && io_k[0]);
    io_u[1] = (io_k[1] ^ io_k[0]);
    io_u[7 : 2] = {io_k[4 : 2],io_k[7 : 5]};
  end


endmodule

As io_alu_operation has to be encoded with at least 3 bit there will be some cases in which the circuit wont do anything. I see that SpinalHDL will treat this as "dont care" and probably takes the last "is"-case as the "default". But this is not necessarily obvious to the developer. And since one cannot leave a default statement or a default assignment (for both these commented solutions to the "unobvious behavior" SpinalHDL will generate an error). In the first case tempRes := 0 before the switch statement its ASSIGNMENT OVERLAP, the default statement will cause an UNREACHABLE STATEMENT error.

I understand that it makes sense to catch these issues on the elaboration stage, tho without the possibility it might cause unobvious behavior the developer didnt see.

I havent confirmed this yet but I would guess that older SpinalHDL versions allowed me to write the logic like that. Otherwise there is a good chance for a latch.

If I wanted to have some default case there I would need to add another enum value to consider this, giving me another hoop when I wanted to avoid writing by using enum in the first place :P

Or maybe I oversee something then someone ofcourse should correct me :)

[Dolu1990]

Hi,

If you check the switch(args), you can also specify additional stuff to relax / be more specific about the handeling of the default statement :
(value: T, coverUnreachable : Boolean = false, strict : Boolean = true)(block: => Unit)

So i think if you set coverUnreachable to true, it may be good ?

[Readon]

There could be a

   tempRes := 0

to specify a default value, if we agree on the same policy as when clause.

  tempRes := 0
  when(cond) {
     tempRes := 1
  }

Then the consistent design policy maintained.

[saahm]

Hi,

If you check the switch(args), you can also specify additional stuff to relax / be more specific about the handeling of the default statement : (value: T, coverUnreachable : Boolean = false, strict : Boolean = true)(block: => Unit)

So i think if you set coverUnreachable to true, it may be good ?

Undocumented features <3 xD
But yea I didnt know about that. so with "coverUnreachable" I can have default block in anyway. Or strict=false to not bother much i guess.
coverUnreachable will generate the default case that I pass and wont throw an error, tho its not documented aside by the code itself.

There could be a

   tempRes := 0

to specify a default value, if we agree on the same policy as when clause.

  tempRes := 0
  when(cond) {
     tempRes := 1
  }

Then the consistent design policy maintained.

If I get a pointer on where and how to enable/implement that I could try to create a pull request in SpinalHDL. If that is not too hard to implement, I havent done anything in the guts of SpinalHDL yet.

[Readon]

to specify a default value, if we agree on the same policy as when clause.

  tempRes := 0
  when(cond) {
     tempRes := 1
  }

Then the consistent design policy maintained.

If I get a pointer on where and how to enable/implement that I could try to create a pull request in SpinalHDL. If that is not too hard to implement, I havent done anything in the guts of SpinalHDL yet.

I think it is important to get a broad consensus before you start coding, which is a kind of policy.

It could be to search for "ASSIGNMENT OVERLAP" in the code to get a clue, then you can find the place for checking where switch takes a position.

[numero-744]

I don't understand 😅 is this issue is turning itself into a feature suggestion, which still requires agreement?

I think I understand what the implementation of the feature would look like, but not the actual feature (what will it make it possible to do for the user?).

[Readon]

I don't understand 😅 is this issue is turning itself into a feature suggestion, which still requires agreement?

I think I understand what the implementation of the feature would look like, but not the actual feature (what will it make it possible to do for the user?).

IMO, if the policy is consistent than it would be easy for user to study and understand.
At the same time, policy should be as simple as possible.
So agreement is required.
And furthermore, we can put the policy into documents.

[saahm]

I don't understand 😅 is this issue is turning itself into a feature suggestion, which still requires agreement?
I think I understand what the implementation of the feature would look like, but not the actual feature (what will it make it possible to do for the user?).

IMO, if the policy is consistent than it would be easy for user to study and understand. At the same time, policy should be as simple as possible. So agreement is required. And furthermore, we can put the policy into documents.

I agree to that, but i guess the actual detail behind it matters obviously.
My naive approach was "oh similar to when within case i can have first assignment wins style and have a default value outside" once I saw default wasnt working by default. Having to also activate some option within the case as a parameter (as was mentioned by Dolu1990) is another hoop to jump through for something thats available by default on a different structure (when-else).
But the default not "working" by default is a result of the amount of elements present inside the enum.
On use of a "ASSIGNMENT OVERLAP" style default value the user should be informed for a switch-case.

I don't understand 😅 is this issue is turning itself into a feature suggestion, which still requires agreement?

I think I understand what the implementation of the feature would look like, but not the actual feature (what will it make it possible to do for the user?).

In the code I posted above this following would work (check the "HERE"):

    val aluArea = new Area{
        val tempRes = UInt(32 bits)
        // HERE: this doesnt work yet and would be of same use _as_ the default statement, but causing an ASSIGNMENT OVERLAP at the moment 
        // tempRes := 0 // default value to not infer latch in case of incomplete mux
        switch(io.alu.operation){
            is(CombOperation.add){
                tempRes := io.alu.opA + io.alu.opB
            }
            is(CombOperation.shift){
                tempRes := io.alu.opA |<< io.alu.opC
            }
            is(CombOperation.sub){
                tempRes := (io.alu.opA - io.alu.opB)
            }
            is(CombOperation.xor){
                tempRes := io.alu.opA ^ io.alu.opB
            }
            is(CombOperation.map){
                tempRes := ((io.alu.opA ^ io.alu.opB) >> io.alu.opC) ^ 0x33ff11a1
            }
            // this can work by switch(op, coverUnreachable = true)
            // default{
            //     tempRes := 0
            // }
        }
    }

As Readon mentioned it would be a bit more consistent with the when statement where implicitly it works (to usually prevent a latch for example)

There could be a

   tempRes := 0

to specify a default value, if we agree on the same policy as when clause.

  tempRes := 0
  when(cond) {
     tempRes := 1
  }

Then the consistent design policy maintained.

If that is really good for case-statement aswell is not fully clear even to me, I asked out of interest as I stumbled on this when I wrote some tutorial examples for students. With coverUnreachable you would have a whole seperate block for default values that makes it easer to have them all there. I'm also not sure if that leads to writing more optimal generators when using this instead of the default-value style causing assingment overlap. I cannot judge that either.

[Readon]

It's a little bit complex, that I tried to remove the "ASSIGNMENT OVERLAP" check to switch.

However, if the default value is not assigned, then "LATCH DETECTED" is reported. It's reasonable, if there is no default value assigned, when clause also would report this error also.

So the thing is about a decision to make, if the default value for switch signal is a must then a bunch of existing code would break. If not assigning with default value would cause a report of "ASSIGNMENT OVERLAP".
If both approaches are accepted, then multiple grammatical crossovers are even more poisonous.

The proper way now is to use "coverUnreachable".

[saahm]

It's a little bit complex, that I tried to remove the "ASSIGNMENT OVERLAP" check to switch.

However, if the default value is not assigned, then "LATCH DETECTED" is reported. It's reasonable, if there is no default value assigned, when clause also would report this error also.

So the thing is about a decision to make, if the default value for switch signal is a must then a bunch of existing code would break. If not assigning with default value would cause a report of "ASSIGNMENT OVERLAP". If both approaches are accepted, then multiple grammatical crossovers are even more poisonous.

The proper way now is to use "coverUnreachable".

That actually makes a lot of sense, changing the would likely break a lot of existing code. And since coverUnreachable exist it should then just be documented.
Then all my questions regarding that are clarified. Thanks a lot unless something else needs to be discussed on that topic!

[Readon]

It's a little bit complex, that I tried to remove the "ASSIGNMENT OVERLAP" check to switch.
However, if the default value is not assigned, then "LATCH DETECTED" is reported. It's reasonable, if there is no default value assigned, when clause also would report this error also.
So the thing is about a decision to make, if the default value for switch signal is a must then a bunch of existing code would break. If not assigning with default value would cause a report of "ASSIGNMENT OVERLAP". If both approaches are accepted, then multiple grammatical crossovers are even more poisonous.
The proper way now is to use "coverUnreachable".

That actually makes a lot of sense, changing the would likely break a lot of existing code. And since coverUnreachable exist it should then just be documented. Then all my questions regarding that are clarified. Thanks a lot unless something else needs to be discussed on that topic!

If it's applicable, would you add this into the documentation? :)