cpu: Align BTBTAGE update reread and bank conflicts#764
cpu: Align BTBTAGE update reread and bank conflicts#764jensen-yan wants to merge 2 commits intoxs-devfrom
Conversation
Change-Id: I39cbd80ec9fcf1bcaa443814e62cee39b5ed163a
📝 WalkthroughWalkthroughAdds per-entry reread decision logic to BTBTAGE: new helpers determine when updates must reread provider predictions, update flow now selectively recomputes or reuses metadata predictions, and bank-conflict checks are performed only when rereads are required. A test verifies bank-conflict is skipped when reread isn't needed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 94b74481c9
ℹ️ 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".
| } else { | ||
| // Provider was already recorded in metadata and can be used directly. | ||
| recomputed = meta_pred; | ||
| } |
There was a problem hiding this comment.
When updateOnRead is enabled, this new fast path skips generateSinglePrediction for provider-used, non-mispred entries and reuses prediction-time metadata directly; however, predictor tables are mutable between prediction and resolve (older targets can update/allocate first), so the recorded {table,index,way} can become stale. updatePredictorStateAndCheckAllocation then updates the entry at that stale location without a tag re-check, which can train the wrong branch entry and silently corrupt BTBTAGE state/results.
Useful? React with 👍 / 👎.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: If5b351f32a70f1397532c9eb61692efb7a2bfc2a
![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/btb_tage.cc`:
- Around line 448-455: The comment above the early return is misleading:
updateOnRead == false does not unconditionally prevent rereads because earlier
checks (missing metadata/prediction) can still force a reread; update the
comment near the block that checks updateOnRead and returns false to state that
updateOnRead disables rereads in normal cases but missing metadata/prediction
still trigger a forced reread, and keep the final sentence describing the
alignment experiment that the function returns it->second.useAlt to allow
rereads only for the alt/base path case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36734c73-a3c5-4673-b4e3-63c20193d31a
📒 Files selected for processing (2)
src/cpu/pred/btb/btb_tage.ccsrc/cpu/pred/btb/btb_tage.hh
| // Legacy mode: if update-on-read is disabled, never reread. | ||
| if (!updateOnRead) { | ||
| return false; | ||
| } | ||
|
|
||
| // Alignment experiment: | ||
| // only when alt/base path is used do we reread and potentially incur bank conflicts. | ||
| return it->second.useAlt; |
There was a problem hiding this comment.
Comment is slightly misleading vs actual control flow.
Line 448 says “never reread” when updateOnRead is disabled, but Lines 426-442 still force reread when metadata/prediction is missing. Please align the comment with actual behavior.
Suggested wording tweak
- // Legacy mode: if update-on-read is disabled, never reread.
+ // Legacy mode: if update-on-read is disabled, skip reread when meta prediction exists.
+ // Missing metadata still forces reread to recover provider information.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Legacy mode: if update-on-read is disabled, never reread. | |
| if (!updateOnRead) { | |
| return false; | |
| } | |
| // Alignment experiment: | |
| // only when alt/base path is used do we reread and potentially incur bank conflicts. | |
| return it->second.useAlt; | |
| // Legacy mode: if update-on-read is disabled, skip reread when meta prediction exists. | |
| // Missing metadata still forces reread to recover provider information. | |
| if (!updateOnRead) { | |
| return false; | |
| } | |
| // Alignment experiment: | |
| // only when alt/base path is used do we reread and potentially incur bank conflicts. | |
| return it->second.useAlt; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/pred/btb/btb_tage.cc` around lines 448 - 455, The comment above the
early return is misleading: updateOnRead == false does not unconditionally
prevent rereads because earlier checks (missing metadata/prediction) can still
force a reread; update the comment near the block that checks updateOnRead and
returns false to state that updateOnRead disables rereads in normal cases but
missing metadata/prediction still trigger a forced reread, and keep the final
sentence describing the alignment experiment that the function returns
it->second.useAlt to allow rereads only for the alt/base path case.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
2 participants
Change-Id: I39cbd80ec9fcf1bcaa443814e62cee39b5ed163a
Summary by CodeRabbit
Bug Fixes
Tests
Chores