Conversation
Change-Id: Id1b9d9e656ce068f27a04ec885344c5f325019d6
📝 WalkthroughWalkthroughThis PR refactors the allocation policy in BTB TAGE and MicroTAGE predictors, replacing the previous logic with a three-tier RTL-aligned priority: first prefer invalid ways, then weak and not-useful ways, finally any not-useful way. The changes introduce a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/pred/btb/test/btb_tage.test.cc`:
- Around line 876-880: The test leaks the existing fixture predictor by
overwriting the pointer `tage` with a new BTBTAGE in the test; before
reassigning `tage = new BTBTAGE(1, 2, 10)` delete or reset the existing instance
created in SetUp() (or replace the raw pointer with a smart pointer) so you
don't leak memory—e.g., call delete on the current `tage` (or use tage.reset())
before creating the new BTBTAGE; ensure this change references the same `tage`
pointer used in SetUp() and preserves subsequent uses of `tage`, and update any
teardown if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9893ea15-eb17-4469-8707-648d7d1a2d92
📒 Files selected for processing (3)
src/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/microtage.ccsrc/cpu/pred/btb/test/btb_tage.test.cc
| tage = new BTBTAGE(1, 2, 10); // only 1 predictor table, 2 ways | ||
| memset(&tage->tageStats, 0, sizeof(BTBTAGE::TageStats)); | ||
| history.resize(64, false); | ||
| stagePreds.resize(2); | ||
|
|
There was a problem hiding this comment.
Avoid leaking the fixture predictor when reinitializing tage.
On Line 876, tage is overwritten without deleting the instance created in SetUp(), which leaks one object per test run.
💡 Minimal fix
TEST_F(BTBTAGETest, AllocationReplacesStrongNotUsefulEntry) {
- tage = new BTBTAGE(1, 2, 10); // only 1 predictor table, 2 ways
+ delete tage;
+ tage = new BTBTAGE(1, 2, 10); // only 1 predictor table, 2 ways
memset(&tage->tageStats, 0, sizeof(BTBTAGE::TageStats));
history.resize(64, false);
stagePreds.resize(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/pred/btb/test/btb_tage.test.cc` around lines 876 - 880, The test
leaks the existing fixture predictor by overwriting the pointer `tage` with a
new BTBTAGE in the test; before reassigning `tage = new BTBTAGE(1, 2, 10)`
delete or reset the existing instance created in SetUp() (or replace the raw
pointer with a smart pointer) so you don't leak memory—e.g., call delete on the
current `tage` (or use tage.reset()) before creating the new BTBTAGE; ensure
this change references the same `tage` pointer used in SetUp() and preserves
subsequent uses of `tage`, and update any teardown if needed.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
1 similar comment
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
2 participants
Background
This PR aligns the BTB TAGE allocation policy in gem5 with the RTL fix from XiangShan PR #5677.
During perf comparison against RTL, gem5 showed significantly higher allocation failure and noticeably worse MPKI. The main mismatch was in the replacement eligibility of TAGE entries during allocation.
Root Cause
Before this patch, gem5 only allowed allocation into:
useful == 0and a weak prediction counter.This is too restrictive.
After global useful reset, many entries may become
useful == 0but still keep a strong counter. In that case, they should become replaceable again. However, the old gem5 logic still blocked replacing them, so useful reset could not effectively free space for new allocations.This leads to two visible problems:
updateAllocFailureNoValidTablestays unnecessarily highgem5 also had an extra "age penalty" path that slowly weakened strong-but-not-useful entries instead of allowing immediate replacement. That behavior does not match the RTL after the fix.
Fix
This patch changes the allocation priority to match RTL:
useful == 0and a weak counteruseful == 0In other words,
strong && !usefulentries are now directly replaceable, which is the key behavior change.The same allocation rule is applied to both:
BTBTAGEMicroTAGEThe old age-penalty path is removed because it is no longer needed and does not match RTL behavior.
Validation
Added/updated unit tests to cover the new behavior, including the case where a strong-but-not-useful entry must be replaceable.
Local checks performed:
scons build/RISCV/cpu/pred/btb/test/tage.test.debug --unit-test -j100./build/RISCV/cpu/pred/btb/test/tage.test.debug --gtest_filter=BTBTAGETest.AllocationBehaviorWithMultipleWays:BTBTAGETest.AllocationReplacesStrongNotUsefulEntryExpected Impact
This patch is expected to: