pulp-platform / common_cells

Common SystemVerilog components
Other
488 stars 138 forks source link

Fix tool issues with automatic variables in PLRU tree #205

Closed michael-platzer closed 8 months ago

michael-platzer commented 8 months ago

Verilator currently does not properly support automatic variables declared inside a combinatorial block if these are only used in some branches (it incorrectly emits a latch warning for these variables). This PR moves the update logic and the PLRU output logic in plru_tree.sv into two functions to avoid this issue.

Additionally, some linting tools (e.g. Spyglass) complain when individual bits of integers are accessed via indexing (as is commonly done with logic vectors), which occurs within the current implementation of plru_tree.sv in two instances, e.g.:

https://github.com/pulp-platform/common_cells/blob/52f83eed3e92ed4ea5f79774854b0cb68618dcbb/src/plru_tree.sv#L94

Instead of extracting bit 0 of new_index by indexing (i.e., via new_index[0]) this PR casts the integer to a width of 1 (via 1'(new_index)) instead, which is equivalent but does not cause linting issues.

The code is otherwise unchanged and remains logically and functionally equivalent.

niwis commented 8 months ago

Thank you @michael-platzer. We recently had tool-compatibility issues with functions. Do you think the two issues could also be solved by

michael-platzer commented 8 months ago

Thanks @niwis for the prompt feedback.

Do you think the two issues could also be solved by

  • adding default assignments for the variables inside the combinatorial block (to fix the latch inference)
  • changing the variable type of new_index (and possibly the other variables) to a logic array, which seems to reflect the intent better

Sure, if that's the preferred solution, then let me change that accordingly.

michael-platzer commented 8 months ago

@niwis I applied the changes you suggested and can confirm that this fixes the issues we had.