RFE: Reduce unnecessary intermediate signals assignments

[tomverbeure]

This is probably a very fundamental request, but I'm going to propose it any way.

Right now, SpinalHDL creates tons of intermediate zz* signals that are not strictly required and that make debugging a whole lot more difficult than necessary.

Here's a typical example:

  assign rvfi_valid = _zz_178;
  assign rvfi_halt = _zz_179;
  ...
  always @ (*) begin
    _zz_178 = writeBack_arbitration_isFiring;
    rvfi_trap = 1'b0;
    _zz_179 = 1'b0;
    if(_zz_110)begin
      _zz_178 = 1'b1;
      rvfi_trap = 1'b1;
      _zz_179 = 1'b1;
    end
    if(writeBack_FormalPlugin_haltFired)begin
      _zz_178 = 1'b0;
    end
  end
  ...
      if((_zz_178 && _zz_179))begin
        writeBack_FormalPlugin_haltFired <= 1'b1;
      end

Except for their declaration, these are the only places where _zz_178 and _zz_179 are used.

The problem, of course, is that one has to page through the code to understand the last 'if' statement is really nothing more than:

      if((rvfi_valid && rvfi_halt))begin
        writeBack_FormalPlugin_haltFired <= 1'b1;
      end

and that the whole code can be rewritten like this:

  always @ (*) begin
    rvfi_valid = writeBack_arbitration_isFiring;
    rvfi_trap = 1'b0;
    rvfi_halt = 1'b0;
    if(_zz_110)begin
      rvfi_valid = 1'b1;
      rvfi_trap = 1'b1;
      rvfi_halt = 1'b1;
    end
    if(writeBack_FormalPlugin_haltFired)begin
      rvfi_valid = 1'b0;
    end
  end
  ...
      if((rvfi_valid && rvfi_halt ))begin
        writeBack_FormalPlugin_haltFired <= 1'b1;
      end

Not all signasl can obviously be reduced that easily (see _zz_110), but I'll take whatever I can get as long as it reduced the zz soup.

However, speaking about _zz_110, there's this:

  assign _zz_105 = 1'b0;
  ..
  always @ (posedge clk) begin
    if(reset) begin
      ..
      _zz_106 <= _zz_105;
      _zz_107 <= _zz_105;
      _zz_108 <= _zz_105;
      _zz_109 <= _zz_105;
      _zz_110 <= _zz_105;
      ..
    end else begin
      ..
      _zz_106 <= writeBack_FormalPlugin_haltRequest;
      _zz_107 <= _zz_106;
      _zz_108 <= _zz_107;
      _zz_109 <= _zz_108;
      _zz_110 <= _zz_109;

The trivial renaming step would result in _zz_105 being eliminated completely:

  always @ (posedge clk) begin
    if(reset) begin
      ..
      _zz_106 <= 1'b0;
      _zz_107 <= 1'b0;
      _zz_108 <= 1'b0;
      _zz_109 <= 1'b0;
      _zz_110 <= 1'b0;
      ..
    end else begin
      ..
      _zz_106 <= writeBack_FormalPlugin_haltRequest;
      _zz_107 <= _zz_106;
      _zz_108 <= _zz_107;
      _zz_109 <= _zz_108;
      _zz_110 <= _zz_109;

And a slightly smarter renaming post-processing step could do the following (assuming there won't be a name clash) :

  always @ (posedge clk) begin
    if(reset) begin
      ..
      writeBack_FormalPlugin_haltRequest_p1 <= 1'b0;
      writeBack_FormalPlugin_haltRequest_p2 <= 1'b0;
      writeBack_FormalPlugin_haltRequest_p3 <= 1'b0;
      writeBack_FormalPlugin_haltRequest_p4 <= 1'b0;
      writeBack_FormalPlugin_haltRequest_p5 <= 1'b0;
      ..
    end else begin
      ..
      writeBack_FormalPlugin_haltRequest_p1 <= writeBack_FormalPlugin_haltRequest;
      writeBack_FormalPlugin_haltRequest_p2 <= writeBack_FormalPlugin_haltRequest_p1;
      writeBack_FormalPlugin_haltRequest_p3 <= writeBack_FormalPlugin_haltRequest_p2;
      writeBack_FormalPlugin_haltRequest_p4 <= writeBack_FormalPlugin_haltRequest_p3;
      writeBack_FormalPlugin_haltRequest_p5 <= writeBack_FormalPlugin_haltRequest_p4;

I don't understand the SpinalHDL framework well enough to know if this kind of deriving of names is at all possible (e.g. as a post-processing step), but it would make it so much easier to debug.
Right now, making a single change will make all the zz signals in a .gtkw file useless.

Tom

[tomverbeure]

Note: I now see that I already filed issue #125 about this. :-)

However, I think this issue is about more than just the VexRiscv. For me personally, it's one of the biggest reasons that is hold me back to fully commit to SpinalHDL for my personal projects.

In isuse #125, I was also primarily talking about registers (_zz_.*<= ...) but the same is true for regular signals.

For example:

  assign _zz_101 = (memory_BRANCH_DO && memory_arbitration_isFiring);
  assign _zz_102 = memory_BRANCH_CALC;
  assign _zz_19 = decode_ALU_BITWISE_CTRL;
  assign _zz_17 = _zz_62;
  assign _zz_47 = decode_to_execute_ALU_BITWISE_CTRL;
  assign _zz_16 = decode_SRC1_CTRL;
  assign _zz_14 = _zz_57;
  assign _zz_43 = decode_to_execute_SRC1_CTRL;
  assign _zz_13 = decode_BRANCH_CTRL;
  assign _zz_23 = _zz_64;
  assign _zz_22 = decode_to_execute_BRANCH_CTRL;
  assign _zz_11 = decode_SHIFT_CTRL;
  assign _zz_8 = execute_SHIFT_CTRL;
  assign _zz_9 = _zz_63;
  assign _zz_34 = decode_to_execute_SHIFT_CTRL;
  assign _zz_32 = execute_to_memory_SHIFT_CTRL;
  assign _zz_6 = decode_ALU_CTRL;
  assign _zz_4 = _zz_68;
  assign _zz_45 = decode_to_execute_ALU_CTRL;
  assign _zz_3 = decode_SRC2_CTRL;
  assign _zz_1 = _zz_65;
  assign _zz_40 = decode_to_execute_SRC2_CTRL;

Straight up assignments like assign _zz_102 = memory_BRANCH_CALC can be optimized away.

If you really go the distance, then even _zz_101, could be optimized away like this:

Old:

  always @ (*) begin
    execute_arbitration_flushAll = 1'b0;
    if(_zz_101)begin
      execute_arbitration_flushAll = 1'b1;
    end
  end

New:

  always @ (*) begin
    execute_arbitration_flushAll = 1'b0;
    if(memory_BRANCH_DO && memory_arbitration_isFiring)begin
      execute_arbitration_flushAll = 1'b1;
    end
  end

You could use a heuristic that takes into account the fanout of _zz_101 to decide whether or not to optimize it away. (Or does it already do that?)

Tom

[Dolu1990]

1. So about the rvfi.valid && rvfi.halt and their _zz signal, the reason why SpinalHDL is doing it is because in VHDL, you can't read output signal values back in the component (and the vhdl buffer feature is broken on some tools). So if the output where VHDL it would had make sense. Now this is verilog, so i think i just have to disable this feature for verilog and it should be fine right ?
   All tools support reading back an output signal in verilog ?

2. About :

  assign _zz_105 = 1'b0;
  ..
  always @ (posedge clk) begin
    if(reset) begin
      ..
      _zz_106 <= _zz_105;
      _zz_107 <= _zz_105;
      _zz_108 <= _zz_105;
      _zz_109 <= _zz_105;
      _zz_110 <= _zz_105;

sure _zz_105 could be simplified automaticaly I will add this to my todo list.

3. About

      _zz_106 <= writeBack_FormalPlugin_haltRequest;
      _zz_107 <= _zz_106;
      _zz_108 <= _zz_107;
      _zz_109 <= _zz_108;
      _zz_110 <= _zz_109;

it is because of Delay(haltRequest, 5, init=False) which currently anomusly create stages. So it isn't a SpinalHDL compiler issue, but just a library improvment. There is already the feature to manualy make this possible in a clash safe way. xx.setCompositeName(nameable: Nameable, postfix: String)
Anyway, it could also be possible to have an automatic name proposal for chained signal as you propose. As a proposed name i would say : "_writeBack_FormalPlugin_haltRequest_s1" (adding a _ at the begining to realy note that it's a automated name)
Will add it to my todo list

4. about things like assign _zz_102 = memory_BRANCH_CALC which can be optimized away, basicaly, it is currently not optimised way for the following reason (general reason which is true in some case true):
   Some operators can't be applied on literal directly :
   "10100"[2] will fail.
   It is especialy true in VHDL where you often need to create a signal to help the VHDL tool to understand the type of the litteral you create (x downto y or y to x)
   But in most of the case, as you said this could be optimized away.

So to resume, in general i choosed safe approach to generate the VHDL/Verilog, as the generated file are only netlist which normaly doesn't need to be human checked. Then when you are learning the language and want to check things, i understand it make sense to check it out. Also it could reduce the post synthesis naming transformation :)

So wooooooooork on the stack.

Thanks for your previous feedback ^^

[Dolu1990]

I just commited in fix in the dev branch for 3) and 1)

new Component {
    val input = in Bool()
    val output = out Bool()
    val readableOutput = out Bool()

    output := RegNext(Delay(RegNext(input),4))
    val yolo = RegNext(input)

    readableOutput := True
    val miaou = Bool
    miaou := readableOutput
  }

will give

module unamed (
      input   input_1,
      output  output_1,
      output  readableOutput,
      input   clk,
      input   reset);
  reg  input_1_regNext;
  reg  input_1_regNext_delay_1;
  reg  input_1_regNext_delay_2;
  reg  input_1_regNext_delay_3;
  reg  input_1_regNext_delay_4;
  reg  input_1_regNext_delay_4_regNext;
  reg  yolo;
  wire  miaou;
  assign output_1 = input_1_regNext_delay_4_regNext;
  assign readableOutput = 1'b1;
  assign miaou = readableOutput;
  always @ (posedge clk) begin
    input_1_regNext <= input_1;
    input_1_regNext_delay_1 <= input_1_regNext;
    input_1_regNext_delay_2 <= input_1_regNext_delay_1;
    input_1_regNext_delay_3 <= input_1_regNext_delay_2;
    input_1_regNext_delay_4 <= input_1_regNext_delay_3;
    input_1_regNext_delay_4_regNext <= input_1_regNext_delay_4;
    yolo <= input_1;
  end

endmodule

[tomverbeure]

Now this is verilog, so i think i just have to disable this feature for verilog and it should be fine right ?
All tools support reading back an output signal in verilog ?

It's extremely common in Verilog to read back an output signal. A tool that doesn't support it would essentially be useless.

So wooooooooork on the stack.

Thanks a lot for considering all this! I'm convinced that it will lower the barrier of entry for people who are considering SpinalHDL.

I wish I could be more helpful with the implementation, but I'm still too green about Scala to figure this out. :-) (I couldn't even find the place in the code where to added the trailing "_" ...)

Tom

[tomverbeure]

So to resume, in general i choosed safe approach to generate the VHDL/Verilog, as the generated file are only netlist which normaly doesn't need to be human checked.

In a traditional Verilog flow, you need to look at the source code to figure out which signals to display on the waveform viewer.

You comment above makes me wonder if you do things differently when debugging SpinalHDL? At the end of the day, you're going to be looking at waves just the same, right? Am I overlooking some debugging technique?

Tom

[Dolu1990]

So my debuging technique is, running the sim, looking the wave, then checking the SpinalHDL code acordingly. I'm never watching the generated VHDL/Verilog, unless i'm realy doing some "meta hardware description" which had gone wrong. So debuging SpinalHDL is like debuging Verilog, you just look the wave and the source code, which is SpinalHDL then ^^

[Dolu1990]

Fixed the literal stuff in 9902346
I think all the issue are fixed, right ?

[tomverbeure]

It's inevitable that the generated code will still have anonymous signals, and some of this is more a matter of heuristics, but it's much better than what it used to be.

A bunch intermediate signals are because you split combinatorial and sequential processes. This is consequence of issue #94 (which I strongly disagree with, but that's a different issue.)

Tom