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

bp: add BHT with private history #2793

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ricted98
Copy link
Contributor

This PR adds a new two-level BHT predictor with private history, as depicted below. The new BPType parameters allow choosing between the original BHT and the new one.

immagine

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

@@ -62,6 +62,8 @@ package cva6_config_pkg;
localparam CVA6ConfigRASDepth = 2;
localparam CVA6ConfigBTBEntries = 0;
localparam CVA6ConfigBHTEntries = 32;
localparam CVA6ConfigBHTHist = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to not use localparam, this can generate bugs because localparam is static and parameter is dynamic (can be configured from outside).

Copy link
Contributor Author

@ricted98 ricted98 Feb 27, 2025

Choose a reason for hiding this comment

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

Are you referring only to the CVA6ConfigBHTHist parameter? What would be the recommended solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

The solution is to proceed as in cv32a60x configuration: remove localparam declaration and set it as seen below:
image

Copy link
Contributor Author

@ricted98 ricted98 Mar 4, 2025

Choose a reason for hiding this comment

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

However, BHTHist should be a tuneable parameter like the remaining ones. My understanding is that cv32a60x and cv32a65x are hardcoded, while remaining config packages have the localparam pattern to tune them. Do you think BHTHist only should be hardcoded?

Copy link
Contributor

❌ failed run, report available here.

2 similar comments
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

github-actions bot commented Mar 1, 2025

❌ failed run, report available here.

@ricted98 ricted98 requested a review from JeanRochCoulon March 3, 2025 09:35
@ricted98 ricted98 marked this pull request as ready for review March 3, 2025 13:46
Co-authored-by: Riccardo Tedeschi <[email protected]>
Copy link
Contributor

github-actions bot commented Mar 4, 2025

❌ failed run, report available here.

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.

3 participants