Skip to content

Fix path history info target#814

Merged
jensen-yan merged 1 commit intoxs-devfrom
fix-phist-target
Apr 9, 2026
Merged

Fix path history info target#814
jensen-yan merged 1 commit intoxs-devfrom
fix-phist-target

Conversation

@Erlkonigal
Copy link
Copy Markdown
Contributor

@Erlkonigal Erlkonigal commented Apr 8, 2026

  • Align target generation in getPHistInfo with getTarget

Change-Id: I3cf46388e19780eccbaa80656db2f74652b97677

Summary by CodeRabbit

  • Bug Fixes

    • Corrected branch target handling for indirect and return branches in prediction so folded path/history uses the overridden targets consistently.
  • Tests

    • Added unit tests validating history folding and path-hash behavior with overridden targets, and confirming predictions remain taken—improving reliability of branch-prediction behavior.

Copilot AI review requested due to automatic review settings April 8, 2026 02:37
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 237b2253-5fdf-4e91-84f6-8275bb48c0ef

📥 Commits

Reviewing files that changed from the base of the PR and between bf1efb6 and 382ed1b.

📒 Files selected for processing (3)
  • src/cpu/pred/btb/common.hh
  • src/cpu/pred/btb/test/btb_mgsc.test.cc
  • src/cpu/pred/btb/test/btb_tage.test.cc
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cpu/pred/btb/common.hh

📝 Walkthrough

Walkthrough

Refactored taken-target computation in BTB prediction: added getEntryTarget() and updated getTarget()/getPHistInfo() to source indirect-target overrides from indirectTargets (by entry.pc) or returnTarget for indirect returns; added tests exercising folded-history updates with those overrides.

Changes

Cohort / File(s) Summary
BTB Target Resolution
src/cpu/pred/btb/common.hh
Introduced FullBTBPrediction::getEntryTarget(const BTBEntry&); getTarget()/getPHistInfo() now use getEntryTarget() to resolve taken-entry targets, selecting entry.target, indirectTargets[entry.pc], or returnTarget based on isIndirect/isReturn.
BTB MGSC tests
src/cpu/pred/btb/test/btb_mgsc.test.cc
Added tests verifying BTBMGSC::specUpdatePHist() uses overridden indirect/return targets from FullBTBPrediction when updating folded history.
BTB TAGE tests
src/cpu/pred/btb/test/btb_tage.test.cc
Added TAGE upper-bound path-hash tests ensuring path-hash/folded-history snapshot and predictTAGE behavior when BTB entries override targets via indirectTargets or returnTarget.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

align-kmhv3

Suggested reviewers

  • jensen-yan
  • Yakkhini
  • CJ362ff

Poem

🐰 I hop through entries, targets in sight,
Indirect paths and returns made right,
Folded histories shift with care,
Predictions follow me everywhere,
A tiny hop, a cleaner flight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix path history info target' directly aligns with the PR's stated objective to align target generation in getPHistInfo with getTarget, and accurately reflects the main refactoring of target computation in the BTB prediction logic.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-phist-target

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Aligns path-history (PHR) PC/target selection in FullBTBPrediction::getPHistInfo() with the main fetch-target selection logic in getTarget() to avoid using stale BTB targets for indirect control flow.

Changes:

  • Updates getPHistInfo() so unconditional indirect branches can source their target from indirectTargets (ITTAGE) or returnTarget (RAS), matching getTarget().
  • Fixes an incorrect comment describing the unconditional-branch PC.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Refactor FullBTBPrediction to share taken-target resolution between getTarget() and getPHistInfo()
- keeping path history consistent with the final predicted target for indirect and return branches
- Add TAGE and MGSC tests for indirect and return target overrides in path-history updates.

Change-Id: I7c111611d19a759b34199c9aa6ab823325ac7578
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 2.2665 -
This PR 2.2709 📈 +0.0044 (+0.19%)

✅ Difftest smoke test passed!

@jensen-yan jensen-yan added the perf label Apr 9, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

🚀 Performance test triggered: gcc12-spec06-0.8c

@jensen-yan
Copy link
Copy Markdown
Collaborator

Thank you very much for your contribution and your PR. This is indeed a serious bug in the PHR update of the model, which may affect the update performance of TAGE, etc. I'm curious how you discovered this bug, whether through testing or code reading. Currently, it seems that XiangShan RTL does not have this bug, and we are evaluating the performance impact of this PR.

@XiangShanRobot
Copy link
Copy Markdown

[Generated by GEM5 Performance Robot]
commit: 382ed1b
workflow: On-Demand SPEC Test (Tier 1.5)

Ideal BTB Performance

Overall Score

PR Master Diff(%)
Score 20.84 20.69 +0.71 🟢

@jensen-yan
Copy link
Copy Markdown
Collaborator

gcc12-spec06-0.8c performance analysis

Compared against the xs-dev Ideal BTB baseline used by the performance robot:

  • PR run: 24170827894 (382ed1b5ec397465aa42fbedcf5cca983e68dab4)
  • Baseline run: 24170690821 (86825b380fb1d020150e94de658f831c96bbdb98)
  • Data processed with python3 run.py ... --slice gcc12

Overall

Metric Baseline PR Delta Delta %
Int score / GHz 20.6907 20.8375 +0.1468 +0.71%
Total branch wrong MPKI 4.9768 4.8501 -0.1267 -2.55%
Conditional branch MPKI 4.8504 4.7279 -0.1225 -2.53%
Indirect branch MPKI 0.0636 0.0609 -0.0027 -4.29%
Return branch MPKI 0.0122 0.0124 +0.0002 +1.34%

Per-benchmark score / condMPKI

Benchmark Score (Base) Score (PR) Score Delta Score Delta % condMPKI (Base) condMPKI (PR) condMPKI Delta condMPKI Delta %
gobmk 15.612 16.143 +0.530 +3.40% 8.829 8.162 -0.666 -7.55%
sjeng 13.169 13.666 +0.497 +3.77% 5.462 4.929 -0.534 -9.77%
gcc 21.177 21.437 +0.260 +1.23% 1.736 1.561 -0.175 -10.08%
h264ref 25.234 25.325 +0.091 +0.36% 0.500 0.476 -0.025 -4.89%
perlbench 16.920 17.004 +0.084 +0.49% 1.184 1.230 +0.046 +3.92%
mcf 34.644 34.675 +0.031 +0.09% 12.709 12.697 -0.012 -0.09%
bzip2 11.265 11.283 +0.017 +0.15% 6.346 6.312 -0.034 -0.54%
omnetpp 21.798 21.807 +0.008 +0.04% 4.086 4.031 -0.056 -1.37%
astar 14.333 14.335 +0.002 +0.02% 16.088 16.083 -0.005 -0.03%
libquantum 46.858 46.858 +0.000 +0.00% 0.043 0.043 +0.000 +0.00%
hmmer 17.065 17.059 -0.006 -0.04% 0.324 0.324 +0.000 +0.00%
xalancbmk 33.961 33.659 -0.301 -0.89% 0.898 0.888 -0.010 -1.11%

Takeaway

  • The gain is real and matches the CI robot comment: +0.71% overall Int score on gcc12-spec06-0.8c.
  • The improvement is concentrated in gobmk, sjeng, and gcc, and these gains correlate well with lower conditional branch MPKI.
  • The aggregate improvement mainly comes from conditional-path prediction quality rather than a large direct improvement in indirect/return prediction. This is consistent with the PHR/path-history fix improving downstream TAGE-style learning instead of only fixing target prediction itself.

@jensen-yan jensen-yan merged commit abf85ff into xs-dev Apr 9, 2026
4 checks passed
@jensen-yan jensen-yan deleted the fix-phist-target branch April 9, 2026 06:40
jensen-yan added a commit that referenced this pull request Apr 9, 2026
Add a completed ExecPlan documenting the PHR target-alignment
investigation for PR #814.

The example records the RTL path-history findings, the consistency check
against gem5, and the supporting gcc12-spec06-0.8c performance analysis.

Change-Id: I545fb4d7a4eab68c2e339e2c4da514b62c50dc34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants