Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tool issues with automatic variables in PLRU tree #205

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

michael-platzer
Copy link
Contributor

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.:

if (new_index[0]) begin

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
Copy link
Collaborator

niwis commented Dec 13, 2023

Thank you @michael-platzer. We recently had tool-compatibility issues with functions. 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

@michael-platzer
Copy link
Contributor Author

michael-platzer commented Dec 14, 2023

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.

This commit moves the declaration of automatic variables to the
beginning of combinatorial blocks and initializes them with a default
value, which avoids issues with Verilator issuing incorrect latch
warnings.  Additionally, it changes the type of `new_index` from integer
to logic to avoid indexing into an integer, which causes issues with
some linting tools.
@michael-platzer michael-platzer changed the title Move PLRU tree logic into functions & avoid integer indexing Fix tool issues with automatic variables in PLRU tree Dec 14, 2023
@michael-platzer
Copy link
Contributor Author

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

Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks a lot! LGTM

@niwis niwis merged commit 71a492c into pulp-platform:master Dec 14, 2023
5 checks passed
@michael-platzer michael-platzer deleted the tools/plru-tree branch December 14, 2023 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants