pulp-platform / spatz

Spatz is a compact RISC-V-based vector processor meant for high-performance, small computing clusters.
https://arxiv.org/abs/2309.10137
Apache License 2.0
67 stars 15 forks source link

Latch Inference while synthesizing Spatz #12

Closed Abdullah-Shaaban closed 5 months ago

Abdullah-Shaaban commented 6 months ago

Hi I tried synthesizing Spatz using Synopsys DC, but it is inferring latches for automatic variables defined in conditional code, which is a coding style used in multiple modules. Here is an example:

always_comb begin
  sig_o = '0;
  if (sig1_i) begin
    automatic logic pnt;
    pnt = sig2_i;
    sig_o = pnt + 1'b1;
  end
end

In this code, pnt is just a name, and we can just use sig_o = sig2_i + 1. This is similar to how a VHDL variable might be used for readability. But DC thinks it is a latch. Is there some special settings in DC that are necessary for synthesizing Spatz without inferring latches?

gvillanovanm commented 6 months ago

Hi,

My best guess is that this snippet is intended for low-power optimization compared to a flip-flop implementation. Therefore, if you cannot utilize it effectively, consider replacing it with an implementation akin to that of a flip-flop, for example:

always_ff @(posedge clk) begin if (!rst_n) begin sig_o <= '0; end else if (sig1_i) begin sig_o <= sig2_i + 1'b1; end end

You must ensure that this snippet provides the same functionality as your intended design.

BR, Gabriel Villanova NM

On Mon, 25 Mar 2024 at 13:40 Abdullah Allam @.***> wrote:

Hi I tried synthesizing Spatz using Synopsys DC, but it is inferring latches for automatic variables defined in conditional code, which is a coding style used in multiple modules. Here is an example:

always_comb begin sig_o = '0; if (sig1_i) begin automatic logic pnt; pnt = sig2_i; sig_o = pnt + 1'b1; end end

In this code, pnt is just a name, and we just do sig_o = sig2_i + 1. This is similar to how a VHDL variable might be used for readability. But DC thinks it is a latch. Is there some special settings in DC that are necessary for synthesizing Spatz without inferring latches?

— Reply to this email directly, view it on GitHub https://github.com/pulp-platform/spatz/issues/12, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVQ4FOQRM2EWN4ZLXOYRTTY2BHOXAVCNFSM6AAAAABFHLGASOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGIYDMMJZGI3TCMY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

FrancescoConti commented 6 months ago

No this is certainly not intended as a latch (otherwise always_latch would be used). It looks, frankly speaking, only a terrible way to code combinational logic. @mp-17 @mattsini1 maybe you should have a look at this…

mp-17 commented 6 months ago

Hello @Abdullah-Shaaban, can you please point us to the real occurrences in the codebase? I assume that the one you reported is just a toy example. And I confirm that the only latches intended in the vector architecture are in its VRF

Abdullah-Shaaban commented 6 months ago

Hi @mp-17 . Yes the example I provided was for illustration. Here is one part of the code from line 427 in spatz_vfu.sv

        if (vrf_rvalid_i[0] && vrf_rvalid_i[1]) begin
          automatic logic [idx_width(N_FU*ELENB)-1:0] pnt;

          reduction_operand_ready_d = 1'b1;
          reduction_pointer_d       = reduction_pointer_q + 1;
          reduction_state_d         = Reduction_Reduce;

          // Request next word
          pnt = reduction_pointer_d << int'(spatz_req.vtype.vsew);
          if (!(|pnt))
            word_issued = 1'b1;
        end

This if statement is inside an always_comb block. The variable pnt is declared and used only inside this condition.

mp-17 commented 6 months ago

I would say it is weird that pnt is synthesized as a latch, as its scope is limited to that block of code. Even if it's interpreted as a latch, its output should not propagate anywhere and I would expect the latch to be simplified away. Do you see the latch from the elaboration report? I will also check, and we can definitely change that code snippet

FrancescoConti commented 6 months ago

Referring to the example code, I suspect that DC interprets the code as similar to this:

logic pnt;

always_comb begin
  sig_o = '0;
  // missing default initialization of pnt
  if (sig1_i) begin
    pnt = sig2_i;
    sig_o = pnt + 1'b1;
  end
end

which in turn results in the creation of a latch for pnt. Clearly, this is a problem in the synthesis tool, but in general, it is also this coding style that gets exposed to such issues.

I usually suggest refraining from coding always_comb procedural blocks that are too bloated and complicated with many nested conditions. It is almost invariably better (although less "software-like" and so somewhat less intuitive) to split them into multiple blocks that can be immediately "mapped" to MUXes and other logic.

Abdullah-Shaaban commented 6 months ago

@mp-17. The elaboration of spatz_vfu ends with the following warning: Netlist for always_comb block contains a latch. (ELAB-974) This refers to the block where pnt is used. Actually, two latches are inferred in this always block, one for each time pnt is coded with that style. Similarly, other automatic signals that are used in a similar way in the decoder, for example, are inferred as latches. @FrancescoConti. I agree that pnt should not describe a latch given that its scope is limited to the if statement, but this is why I was asking about maybe a setting in DC that avoids this. It seems a general problem.

mp-17 commented 5 months ago

@Abdullah-Shaaban, we can modify those lines just to be on the safe side. However, I checked with both Fusion and Spyglass and no latch is reported. Which version of DC are you using? If you can provide a list of all the lines of code that generate a latch during elaboration with your DC, we can address all the problems at once.

Abdullah-Shaaban commented 5 months ago

Hi @mp-17 I have looked carefully into the Verilog netlist, and it seems that even the warning is still there, there are no latch cells instantiated (cells with LAT* in their name) for the particular parts of the code where the warning occurs. Therefore, I believe the warning is a false alarm. It indeed seems just a tool problem (also depending on the settings). Verilator does have a similar issue with this coding style. I guess the issue can be closed.