Sc test align#852
Conversation
Change-Id: Ie2b38fd880551def364b0b3f0a56ba115dc14a7c
Change-Id: Ic34c0efd1c02cbb88bd4c27e248012eee11bd09d
Change-Id: I6d9d8fb3aae8aa28b4848ca6e0d62f5196e9e140
Change-Id: I2e585ec892473c4b83747f05018cf2464a0d5f52
Change-Id: Ib9c372a0183349af7d106f6f60cfaac9f02d1905
Change-Id: I060cbdf18dc7c0dbf75df01faf88538cb1be7f9c
Change-Id: I89179c4c5b1e5b7b30001d9ea535c43c84dc8eb4
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEnables MGSC PC-threshold in KMHV3 DecoupledBPUWithBTB config and raises SC gating thresholds in BTBMGSC so SC overrides TAGE only at larger margin values; trace ChangesMGSC threshold + SC/TAGE gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
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: f56da4ab67
ℹ️ 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".
| cpu.branchPred.mgsc.enableBwTable = False | ||
| cpu.branchPred.mgsc.enableLTable = False | ||
| cpu.branchPred.mgsc.enableITable = False | ||
| cpu.branchPred.mgsc.enableGTable = False | ||
| cpu.branchPred.mgsc.enablePTable = False |
There was a problem hiding this comment.
For any kmhv3.py run using the default DecoupledBPUWithBTB path, these assignments disable every MGSC component table except bias, and there is no CLI gate or later override to restore the normal predictor. The SimObject defaults keep the BW/I/G/P tables enabled, so baking a “gcc15 slice CI” ablation into the shared config silently turns normal SPEC/checkpoint experiments into a bias-only SC configuration and will skew performance results outside that one test slice.
Useful? React with 👍 / 👎.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/example/kmhv3.py`:
- Around line 114-120: The MGSC table ablation lines in kmhv3.py (inside the
DecoupledBPUWithBTB block) unconditionally disable enableBwTable, enableLTable,
enableITable, enableGTable and enablePTable, which should be gated: add a
CLI/config flag (e.g., --mgsc-ablation or --mgsc-ablation-table with a table
name) that defaults to false/None and only when set overrides the
cpu.branchPred.mgsc.enable* flags to perform the ablation; leave the default
behavior as full-MGSC enabled, and wire the flag into the config loading path so
the code that sets cpu.branchPred.mgsc.enableBwTable, enableLTable,
enableITable, enableGTable, enablePTable checks the flag before forcing False
(or selectively disables only the requested table).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
🚀 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
|
Change-Id: Id1906f09efc28fe566bf1223a765e8b57576b917
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cpu/pred/btb/btb_mgsc.cc`:
- Around line 795-797: The ternary used to compute effective_gate can fall
through to total_thres/4 when no confidence flags are set, producing a
misleading margin; change the calculation to mirror the prediction-side if/else
if structure: test pred.tage_conf_high, then else if pred.tage_conf_mid, then
else if pred.tage_conf_low, and only if none are set use a sensible default
(e.g. treat as no gate or skip logging) before computing margin =
std::abs(total_sum) - effective_gate; update the logic around effective_gate,
margin, and any code paths involving forceUseSC so margin reflects the same gate
selection rules as the predictor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f30d5a7-27c5-428d-a72b-378bb479cf31
📒 Files selected for processing (2)
configs/example/kmhv3.pysrc/cpu/pred/btb/btb_mgsc.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/example/kmhv3.py
| auto effective_gate = pred.tage_conf_high ? total_thres | ||
| : (pred.tage_conf_mid ? (total_thres / 2) : (total_thres / 4)); | ||
| auto margin = std::abs(total_sum) - effective_gate; |
There was a problem hiding this comment.
effective_gate defaults to total_thres/4 when no confidence flag is set
The ternary chain falls through to total_thres / 4 when all three conf flags (tage_conf_high, tage_conf_mid, tage_conf_low) are false — e.g. when forceUseSC = true with no conf flag asserted, or any future code path where the flags are all clear. In that state the logged margin is computed against a phantom gate value, making the trace misleading for those rows.
🐛 Proposed fix — mirror the prediction-side `if/else if` structure
- auto effective_gate = pred.tage_conf_high ? total_thres
- : (pred.tage_conf_mid ? (total_thres / 2) : (total_thres / 4));
+ auto effective_gate = pred.tage_conf_high ? total_thres
+ : pred.tage_conf_mid ? (total_thres / 2)
+ : pred.tage_conf_low ? (total_thres / 4)
+ : 0; // no confidence tier active (e.g. forceUseSC); gate is irrelevant📝 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.
| auto effective_gate = pred.tage_conf_high ? total_thres | |
| : (pred.tage_conf_mid ? (total_thres / 2) : (total_thres / 4)); | |
| auto margin = std::abs(total_sum) - effective_gate; | |
| auto effective_gate = pred.tage_conf_high ? total_thres | |
| : pred.tage_conf_mid ? (total_thres / 2) | |
| : pred.tage_conf_low ? (total_thres / 4) | |
| : 0; // no confidence tier active (e.g. forceUseSC); gate is irrelevant | |
| auto margin = std::abs(total_sum) - effective_gate; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cpu/pred/btb/btb_mgsc.cc` around lines 795 - 797, The ternary used to
compute effective_gate can fall through to total_thres/4 when no confidence
flags are set, producing a misleading margin; change the calculation to mirror
the prediction-side if/else if structure: test pred.tage_conf_high, then else if
pred.tage_conf_mid, then else if pred.tage_conf_low, and only if none are set
use a sensible default (e.g. treat as no gate or skip logging) before computing
margin = std::abs(total_sum) - effective_gate; update the logic around
effective_gate, margin, and any code paths involving forceUseSC so margin
reflects the same gate selection rules as the predictor.
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: Ia9832e1fd0d060625391939ef0de4ba5e54549cc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 847b2e4d5a
ℹ️ 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".
| if (!use_sc_pred) { | ||
| if (tage_info.tage_pred_conf_high) { | ||
| if (abs(total_sum) > total_thres / 2) { | ||
| if (abs(total_sum) > total_thres) { |
There was a problem hiding this comment.
With the unchanged BTBMGSCTest.GateHighConfUsesSCWhenStrong vector, total_sum is 18 and the default total_thres is 35. After this threshold bump, high-confidence predictions require 18 > 35, so use_mgsc stays false and the prediction falls back to TAGE/not-taken instead of the test's expected SC/taken result; please update the unit test inputs/expectations along with the new gate semantics so the MGSC test suite does not fail.
Useful? React with 👍 / 👎.
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
2 participants
test enable each SC table
Summary by CodeRabbit
New Features
Chores