bpu: test tage not use alt table#690
Conversation
Change-Id: I4d1d26b637f35f566120a3fda6119699be7c32cf
📝 WalkthroughWalkthroughThe PR disables the alternate TAGE prediction provider within the BTB TAGE predictor. The alternate prediction path is removed from the logic while maintaining minimal metadata compatibility. State updates and allocation decisions no longer reference the alternate provider. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 323d9eead9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🧹 Nitpick comments (2)
src/cpu/pred/btb/btb_tage.cc (2)
474-479: Consider removing dead humility mechanism code.Since
alt_is_correct_and_strongis hardcoded tofalse(line 474), the humility mechanism at lines 477-479 is now dead code:// a. Special reset (humility mechanism) if (alt_is_correct_and_strong && main_is_correct) { // ← never true way.useful = 0; DPRINTF(TAGEUseful, "useful bit reset to 0 due to humility rule\n"); }This is intentional (alt provider removed), but the dead code could be removed or documented more clearly.
🔎 Optional cleanup
// Update useful bit based on several conditions bool main_is_correct = main_info.taken() == actual_taken; - const bool alt_is_correct_and_strong = false; // no alt provider - - // a. Special reset (humility mechanism) - if (alt_is_correct_and_strong && main_is_correct) { - way.useful = 0; - DPRINTF(TAGEUseful, "useful bit reset to 0 due to humility rule\n"); - } else if (main_info.taken() != alt_taken) { - // b. Original logic to set useful bit high + // Humility mechanism (alt_is_correct_and_strong check) disabled with alt provider removal + if (main_info.taken() != alt_taken) { + // Set useful bit high when main differs from base and is correct if (main_is_correct) { way.useful = 1; } }
500-504: Optional: Simplify condition since alt_info.found is always false.Since the alternate provider is disabled,
alt_info.foundis alwaysfalse(line 214), making the condition!alt_info.foundalways true:if (used_alt && !alt_info.found) { // ← !alt_info.found always trueThe condition could be simplified to
if (used_alt)for clarity, though this doesn't affect correctness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cpu/pred/btb/btb_tage.cc
🧰 Additional context used
🧬 Code graph analysis (1)
src/cpu/pred/btb/btb_tage.cc (1)
src/cpu/pred/btb/btb_tage.hh (5)
TageTableInfo(85-85)TageTableInfo(86-87)btb_entry(416-418)startPC(170-171)startPC(430-437)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
- GitHub Check: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
🔇 Additional comments (1)
src/cpu/pred/btb/btb_tage.cc (1)
211-294: Prediction path correctly implements alt provider removal.The changes properly disable the alternate TAGE provider while maintaining the base fallback mechanism. Key aspects:
alt_infois kept empty for schema compatibility (line 214)- First match becomes main prediction; loop breaks to prevent finding a second match (line 254)
- Alt prediction now means base fallback:
alt_taken = base_taken(line 272)use_alt_on_namechanism preserved to choose between weak main and base fallback (lines 276-288)The logic is clear and internally consistent.
| Addr base_idx = getBaseTableIndex(startPC); | ||
| unsigned branch_idx = getBranchIndexInBlock(entry.pc, startPC); | ||
| bool base_taken = baseTable[base_idx][branch_idx] >= 0; | ||
| bool alt_taken = alt_info.found ? alt_info.taken() : base_taken; | ||
| // Alt provider disabled: "alt" is always the base fallback. | ||
| const bool alt_taken = base_taken; |
There was a problem hiding this comment.
Fix inconsistent base_taken calculation in update path.
The update path recalculates base_taken using simplified logic that always reads from baseTable (line 441), but the prediction path uses conditional logic based on getDelay() and usingBasetable (lines 266-269):
// Prediction path (lines 266-269):
const bool base_taken =
getDelay() != 0 ?
(usingBasetable ? baseTable[base_idx][branch_idx] >= 0 : btb_entry.ctr >= 0) :
btb_entry.ctr >= 0;
// Update path (line 441):
bool base_taken = baseTable[base_idx][branch_idx] >= 0; // ← always baseTableThis inconsistency means the update path may determine alt_correct (line 451) based on a different prediction than what was actually made, leading to incorrect use_alt_on_na counter updates and statistics.
Solution: Use pred.altPred which already contains the correctly calculated prediction from generateSinglePrediction:
🔎 Proposed fix
- // Use base table instead of entry.ctr for fallback prediction
- Addr startPC = stream.getRealStartPC();
- Addr base_idx = getBaseTableIndex(startPC);
- unsigned branch_idx = getBranchIndexInBlock(entry.pc, startPC);
- bool base_taken = baseTable[base_idx][branch_idx] >= 0;
- // Alt provider disabled: "alt" is always the base fallback.
- const bool alt_taken = base_taken;
+ // Alt provider disabled: use pred.altPred which was calculated in generateSinglePrediction
+ const bool alt_taken = pred.altPred;This ensures consistency between prediction and update, and you can remove the now-unused base_idx and branch_idx calculations from lines 439-440.
🤖 Prompt for AI Agents
In src/cpu/pred/btb/btb_tage.cc around lines 439-443, the update path recomputes
base_taken directly from baseTable which is inconsistent with the prediction
path that considers getDelay() and usingBasetable; replace the recomputed
base/alt logic by using the already-computed pred.altPred (and/or pred.basePred
as appropriate) from generateSinglePrediction for determining base_taken and
alt_taken, and remove the now-unused base_idx and branch_idx calculations so the
update uses the exact prediction made during the prediction phase.
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
2 participants
Change-Id: I4d1d26b637f35f566120a3fda6119699be7c32cf
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.