cpu: Enable resolve update defaults#933
Conversation
Change-Id: I3318de2a96e82d52a4159b31e21d771bb2b1b1f9
📝 WalkthroughWalkthrough
ChangesresolvedUpdate class defaults, iewToFetchDelay default, and config cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates XiangShan’s BTB-based branch predictor defaults so key BTB components (MBTB/TAGE/ITTAGE) use resolve-stage (resolved) updates by default, and simplifies the XiangShan kmhv3.py configuration by removing now-redundant overrides.
Changes:
- Set
resolvedUpdate = Trueas the default forMBTB,BTBTAGE, andBTBITTAGE(inheriting fromTimedBaseBTBPredictor.resolvedUpdate = False). - Remove
kmhv3.py’s redundant per-instanceresolvedUpdateassignments for MBTB/TAGE/ITTAGE now that the defaults are updated. - Remove redundant
enabled = Trueassignments inkmhv3.py(the base predictor component default is alreadyenabled = True).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cpu/pred/BranchPredictor.py | Changes default resolvedUpdate behavior for MBTB/BTBTAGE/BTBITTAGE to resolve-stage updates. |
| configs/example/kmhv3.py | Removes redundant resolved-update and enabled overrides now covered by predictor defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 24213d67dd
ℹ️ 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".
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: Id2fb11d63a7ba4cfcbb91a25b281bc8fcdbb1f4c
![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 `@src/cpu/o3/BaseO3CPU.py`:
- Around line 123-124: The timing change is currently applied at the BaseO3CPU
default, which affects every O3 CPU inheriting from BaseO3CPU through Fetch’s
use of params.iewToFetchDelay and getWire(-iewToFetchDelay). Keep the shared
BaseO3CPU default unchanged, and move the iewToFetchDelay override into the
XiangShan-specific CPU/config class or equivalent override point used by
kmhv3/idealkmhv3 so only those targets get the 2-cycle delay.
🪄 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: 2bd45980-657b-4b06-93d5-5e1b6f7e1ca2
📒 Files selected for processing (3)
configs/example/idealkmhv3.pyconfigs/example/kmhv3.pysrc/cpu/o3/BaseO3CPU.py
💤 Files with no reviewable changes (2)
- configs/example/idealkmhv3.py
- configs/example/kmhv3.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2fc1966eb
ℹ️ 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".
| cxx_class = 'gem5::branch_prediction::btb_pred::MBTB' | ||
| cxx_header = 'cpu/pred/btb/mbtb.hh' | ||
|
|
||
| resolvedUpdate = True |
There was a problem hiding this comment.
When running the SMT path (checked configs/example/smt_idealkmhv3.py, which sets args.smt = True with DecoupledBPUWithBTB), IEW::executeInsts only clears toFetch->iewInfo[tid].resolvedCFIs for *activeThreads->begin(), and repo-wide search shows no other clear. Enabling resolved updates by default makes stale resolved-CFI entries from non-first threads retrain MBTB/TAGE/ITTAGE every cycle after they first execute a branch, corrupting SMT predictor state instead of being a harmless no-op as before.
Useful? React with 👍 / 👎.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
用CI 触发了ideal 版本的0.8c 性能测试，发现相对主线降低了0.2分，参考 本commit https://github.com/OpenXiangShan/GEM5/actions/runs/28429735723 vs 主线https://github.com/OpenXiangShan/GEM5/actions/runs/28369635805 |
|
Maybe this PR can be rebased and re-evaluate after PR #934 merged, since the delay strongly affect the resolve training performance. |
Motivation
The BTB-based XiangShan configuration should use resolve-stage predictor updates by default for the main resolved-update components.
kmhv3.pyalready forced this for MBTB, TAGE, and ITTAGE, whileidealkmhv3.pystill inherited the older commit-update default.Approach
resolvedUpdate = TrueonMBTB,BTBTAGE, andBTBITTAGEdefaults.kmhv3.pyresolved-update assignments.enabled = Trueassignments that matched predictor defaults.Scope
This keeps the default of other BTB components unchanged; uBTB, ABTB, and RAS still inherit the base
resolvedUpdate = Falsedefault.Validation
python3 -m py_compile configs/example/kmhv3.py configs/example/idealkmhv3.py src/cpu/pred/BranchPredictor.pygit diff --checkscons build/RISCV/gem5.opt --gold-linker -j64./build/RISCV/gem5.opt /tmp/check_resolve_defaults.pyprintsTrue True True False False Falsefor MBTB/TAGE/ITTAGE/uBTB/ABTB/RAS.Summary by CodeRabbit