Possible inferred latches in riscv_pmp.sv

[MikeOpenHWGroup]

DSIM is giving a warning about inferred latches in riscv_pmp.sv. I do not see this warning with xrun, and I lack sufficient understanding of synthesis to determine if this warning is real or not.

I'm assigning this issue to @davideschiavone because he knows the code well, @aimeepsutton because she knows DSIM and @ludmila-bta because she is a synthesis expert.

Details below:

URL: https://github.com/openhwgroup/cv32e40p
Branch: master
Hash: de82624
Path: rtl/riscv_pmp.sv, lines 575..640
Below is the snipit of code for the first warning. DSIM warns about an inferred latch on data_match_region. A similar snipit of code generates the same warning about instr_match_region. Note that there are similar warnings in other RTL files which I have not yet investigated.

It seems to me that data_match_region is written on every path through the code, but I do not know if synthesis will see it that way.

   always_comb
   begin
      for(j=0;j<N_PMP_ENTRIES;j++)
      begin

         if( EN_rule[j] & ((~data_we_i & R_rule[j]) | (data_we_i & W_rule[j])) )
         begin
             case(MODE_rule[j])
         `ifdef ENABLE_TOR
               2'b01:
               begin : TOR_CHECK_DATA
                      if ( ( data_addr_i[31:2] >= start_addr[j])  &&  ( data_addr_i[31:2] < stop_addr[j])  )
                      begin
                         data_match_region[j] = 1'b1;
                         `ifdef DEBUG_RULE $display("HIT on TOR RULE %d: [%8h] <= data_addr_i [%8h] <= [%8h], RULE=%2h, X=%b, W=%b, R=%d", j, (start_addr[j])>>2 , data_addr_i , (stop_addr[j])>>2, pmp_cfg_i[j][4:3], X_rule[j], W_rule[j], R_rule[j]); `endif
                      end
                      else
                      begin
                         data_match_region[j] = 1'b0;
                      end
               end
         `endif

               2'b10:
               begin : NA4_CHECK_DATA
                  if ( data_addr_i[31:2] == start_addr[j][29:0] )
                  begin
                     data_match_region[j] = 1'b1;
                     `ifdef DEBUG_RULE $display("HIT on NA4 RULE %d: [%8h] == [%8h] , RULE=%2h, X=%b, W=%b, R=%d", j, (start_addr[j])>>2 , data_addr_i , pmp_cfg_i[j][4:3], X_rule[j], W_rule[j], R_rule[j]); `endif
                  end
                  else
                  begin
                     data_match_region[j] = 1'b0;
                  end
               end

         `ifdef ENABLE_NAPOT
               2'b11:
               begin : NAPOT_CHECK_DATA
                     //$display("Checking NAPOT RULE [%d]: %8h, == %8h", j, data_addr_i[31:2] &  mask_addr[j][29:0], start_addr[j][29:0]);
                     if ( (data_addr_i[31:2] & mask_addr[j][29:0]) == start_addr[j][29:0] )
                     begin
                        data_match_region[j] = 1'b1;
                     end
                     else
                     begin
                        data_match_region[j] = 1'b0;
                        //$display("NO MACHING NAPOT: %8h, == %8h", (data_addr_i[31:2] &  mask_addr[j][29:0]), start_addr[j][29:0]);
                     end
               end
         `endif

               default:
               begin
                  data_match_region[j] = 1'b0;
               end
            endcase // MODE_rule[j]

         end
         else
         begin
               data_match_region[j] = 1'b0;
         end

      end
   end

To Reproduce:

Run the comp rule (clones the RTL, compiles, and quits).

$ git clone https://github.com/openhwgroup/core-v-verif
$ cd core-v-verif/cv32/sim/uvmt_cv32
$ make SIMULATOR=dsim comp

[ludmila-bta]

I have checked my synthesis trials and there are no warnings about data_match_region (as well as instr_match_region). It looks like the tool optimized them out as they are not in the netlist either.

Based on the Genus default settings, a latch whose output never changes is replaced by the corresponding constant value (optimize_constant_latches is true).

I've found only one warning about always_comb not matching the expected behaviour, but it looks like this thing is related to a different module:

Warning : Generated logic differs from the expected logic. [CDFG2G-615]
        : Signal 'trap_base_addr' in module 'riscv_if_stage_N_HWLP2_RDATA_WIDTH32_FPU0_DM_HaltAddress32h1a110800' modeled as latch instead of wire in file '/home/lrubin/proj/pnr/work/risc
v_core_Mar19_ewm2_usf/inputs/rtl/riscv_if_stage.sv' on line 131, column 36.
        : The logic generated for an always_comb, always_latch or always_ff process may not match the behavior specified in the input HDL.

[davideschiavone]

Looking at the code indeed it is not a Latch. Maybe writing them to 0 as in the else case before the if statement would remove the problem. What do you think @aimeepsutton ?

[GTumbush]

Agreed, I don't see a latch. I would recommend using:
for(j=0;j<N_PMP_ENTRIES;j++)
begin
data_match_region[j] = 1'b0; // default

and removing all duplicate assignments to 0, unless you want them for clarity. This will simplify the code greatly and hopefully git rid of the bogus latch warning.

[aimeepsutton]

I had a hunch that the for loop might be the problem. If N_PMP_ENTRIES is 0, none of the subsequent code is executed and data_match_region is not updated.

I added this:

always_comb
begin
  data_match_region = 'h0;  // Initial value is always assigned
   
  for(j=0;j<N_PMP_ENTRIES;j++)
  begin
... 
     `

Which I believe is standard practice for combinational logic. The warning disappeared.

That said, I will discuss the validity of the warning with our R&D team at Metrics. Thanks for reporting the issue!

[ludmila-bta]

I was going through some Genus documentation and found that the tool may give an error in case for loop is used at the top of always block. They recommend having if statement at the top (before the for loop) to infer a latch or flip-flop.
In this case there was no error in the synthesis, but it may explain the warning you are seeing.

[MikeOpenHWGroup]

Hi @ludmila-bta, do you mean something like this:

   always_comb
   begin
      if( EN_rule[j] & ((~data_we_i & R_rule[j]) | (data_we_i & W_rule[j])) )
      begin
         for(j=0;j<N_PMP_ENTRIES;j++)
         begin
             case(MODE_rule[j])
                   // snip...
             endcase // MODE_rule[j]
         end
      else
         begin
               data_match_region[j] = 1'b0;
         end
      end
   end

I think this is a good suggestion. It appears to be logically equivalent and should eliminate the inferred latch. On the other hand, the suggestion from @aimeepsutton should also eliminate the latch and @GTumbush pointed out it allows for some code simplification.

Either way, I'm more convinced than before that this is something that needs to be fixed.

[ludmila-bta]

Hi @MikeOpenHWGroup. Yes, this is exactly what Cadence recommends as a tool friendly coding. Or another solution would be something like

for(j=0;j<N_PMP_ENTRIES;j++)
begin
      always_comb     
      begin
         if( EN_rule[j] & ((~data_we_i & R_rule[j]) | (data_we_i & W_rule[j])) )
         begin
             case(MODE_rule[j])
             ...

I've found it in their description of how to resolve the VLOGPT-46 error : An 'if' statement is required at the top of an always block to infer a latch or flip-flop. [VLOGPT-46] [read_hdl]

I am still not sure why the tool didn't complain about this one in my trials. Would be good to debug it, but unfortunately I don't have the license anymore. I can send this question to our Genus AE if you think it would be beneficial.