Skip to content

align RTL: tage window method, block resolveQ if update failed due to bank conflict.#644

Merged
jensen-yan merged 3 commits intoxs-devfrom
tage-bank1-align
Dec 12, 2025
Merged

align RTL: tage window method, block resolveQ if update failed due to bank conflict.#644
jensen-yan merged 3 commits intoxs-devfrom
tage-bank1-align

Conversation

@jensen-yan
Copy link
Copy Markdown
Collaborator

@jensen-yan jensen-yan commented Dec 10, 2025

Align with RTL, use an 8-window approach to block resolveQ.
When there is a conflict in tage updates, resolveQ cannot dequeue and fails to update the entire BPU.
After accumulating 8 times, it blocks BPU prediction to allow tage to update successfully once.

Summary by CodeRabbit

  • New Features

    • Configurable prediction-blocking during branch-predictor updates with a threshold to temporarily pause predictions.
    • Two-phase update flow: probe for bank conflicts, defer or apply updates accordingly.
  • Improvements

    • Added resolve success/failure notifications to drive update flow.
    • New metrics for deferred updates and blocked predictions to aid observability.
    • Deferral replaces immediate dropping for bank-conflict cases, improving robustness.

✏️ Tip: You can customize this high-level summary in your review settings.

otherwise main BTB may do resolve udpate twice if tage resolveUpdate failed

Change-Id: I660cb52b7f70a4202ca7741d8c7ed564fa7cd5b8
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 10, 2025

Walkthrough

Implements a probe-then-apply two‑phase resolve mechanism for predictor updates: components are probed for bank/conflict readiness, updates are applied only if all components can resolve, and repeated probe failures can temporarily block prediction. Changes touch fetch stage, decoupled BPU, BTBTAGE, and base predictor interfaces.

Changes

Cohort / File(s) Summary
Fetch stage resolve handling
src/cpu/o3/fetch.cc
resolveUpdate() call site changed to treat a boolean return: calls notifyResolveSuccess() on true and notifyResolveFailure() on false; only pops resolve queue on success.
Decoupled BPU + stats
src/cpu/pred/btb/decoupled_bpred.cc, src/cpu/pred/btb/decoupled_bpred.hh, src/cpu/pred/btb/decoupled_bpred_stats.cc
DecoupledBPUWithBTB::resolveUpdate changed from voidbool. Added notifyResolveSuccess(), notifyResolveFailure(), blockPredictionOnce(), blockPredictionPending, resolveDequeueFailCounter, resolveBlockThreshold and predictionBlockedForUpdate stat; prediction can be blocked after thresholded failures.
BTBTAGE predictor
src/cpu/pred/btb/btb_tage.cc, src/cpu/pred/btb/btb_tage.hh
Introduced canResolveUpdate(const FetchStream&) (bank‑conflict probe) and doResolveUpdate(const FetchStream&) (deferred apply). Refactored update flow into probe/apply phases and renamed stat updateDroppedDueToConflictupdateDeferredDueToConflict.
Base BTB interface
src/cpu/pred/btb/timed_base_pred.hh
Added virtual canResolveUpdate(const FetchStream&) (default true) and doResolveUpdate(const FetchStream&) (default delegates to update()), enabling two‑phase resolve across predictors.
Predictor config
src/cpu/pred/BranchPredictor.py
Added resolveBlockThreshold Param to DecoupledBPUWithBTB to configure consecutive resolve dequeue failures before blocking predictions.
Tests
src/cpu/pred/btb/test/btb_tage.test.cc
Tests updated to use canResolveUpdate()/doResolveUpdate() two‑step flow and assert deferred vs applied behavior for bank conflicts and non‑conflicts.

Sequence Diagram

sequenceDiagram
    participant Fetch as Fetch Stage
    participant DBPU as DecoupledBPU
    participant TAGE as BTBTAGE
    participant Other as Other Predictors

    Fetch->>DBPU: resolveUpdate(stream)
    activate DBPU
    Note over DBPU: Phase 1 — Probe all components
    DBPU->>TAGE: canResolveUpdate(stream)
    TAGE-->>DBPU: true/false
    DBPU->>Other: canResolveUpdate(stream)
    Other-->>DBPU: true/false

    alt Any probe returned false
        DBPU->>DBPU: resolveDequeueFailCounter++
        alt Counter >= resolveBlockThreshold
            DBPU->>DBPU: blockPredictionOnce()
        end
        DBPU-->>Fetch: false
        Fetch->>DBPU: notifyResolveFailure()
        Fetch->>Fetch: keep stream in queue
    else All probes true
        Note over DBPU: Phase 2 — Apply updates
        DBPU->>TAGE: doResolveUpdate(stream)
        TAGE-->>DBPU: done
        DBPU->>Other: doResolveUpdate(stream)
        Other-->>DBPU: done
        DBPU-->>Fetch: true
        Fetch->>DBPU: notifyResolveSuccess()
        Fetch->>Fetch: pop stream from queue
    end
    deactivate DBPU
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Pay special attention to:
    • decoupled_bpred.cc — two‑phase probe/apply logic, counter and block state transitions.
    • btb_tage.cc — bank‑conflict detection semantics, deferred vs dropped updates, and interaction with snapshot/recompute paths.
    • fetch.cc — correct handling of queue popping and notify calls to avoid losing or reordering resolve entries.

Possibly related PRs

Suggested labels

perf, align-kmhv3

Suggested reviewers

  • Yakkhini
  • CJ362ff

Poem

🐰 A probe, a pause before the leap,

I check each bank so updates keep,
If conflicts stack, I count and wait,
Then let predictions leap — or hesitate.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.48% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ 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 directly describes the main implementation change: introducing an 8-window mechanism (RTL alignment) to block resolveQ when TAGE bank conflict updates fail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tage-bank1-align

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.

@XiangShanRobot
Copy link
Copy Markdown

[Generated by GEM5 Performance Robot]
commit: e685026
workflow: gem5 Ideal BTB Performance Test

Ideal BTB Performance

Overall Score

PR Master Diff(%)
Score 15.82 15.63 +1.23 🟢

[Generated by GEM5 Performance Robot]
commit: e685026
workflow: gem5 Ideal BTB Performance Test

Ideal BTB Performance

Overall Score

PR Previous Commit Diff(%)
Score 15.82 15.82 -0.01 🔴

Change-Id: I44f3e6433d110d7625f4fbdb29df0e95d5ab9181
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/cpu/pred/btb/test/btb_tage.test.cc (3)

891-910: Same-bank conflict path correctly exercises canResolveUpdate; optionally tighten invariants

The new canResolveUpdate(stream) usage plus the conflict-counter and predBankValid checks look consistent with the intended “detect & defer” behavior. If you want this test to guard more strongly against regressions, you could additionally assert that no update-related stats (e.g., updateAllocSuccess) change in this path, to prove the update was fully skipped rather than partially applied.


913-928: No-conflict path two-phase flow looks good; consider checking post-update state if it matters

Using canResolveUpdate(stream) with ASSERT_TRUE before doResolveUpdate(stream) cleanly exercises the non-conflict path and confirms the conflict counter stays unchanged. If doResolveUpdate is also expected to clear or update any state such as predBankValid after a successful resolve, you might add an assertion here to encode that contract in the test.


931-948: Conflict-disabled path is correct; optional: isolate subcases with a fresh predictor

With enableBankConflict = false, the canResolveUpdate / doResolveUpdate pair correctly bypasses conflict accounting even for same-bank accesses. Since all three subcases share the same bankTage instance, consider (optionally) constructing a fresh BTBTAGE for each sub-block or explicitly resetting any relevant state if you ever tighten the implementation semantics; this would make the test more robust to future changes in internal bookkeeping.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e685026 and 3135da1.

📒 Files selected for processing (1)
  • src/cpu/pred/btb/test/btb_tage.test.cc (3 hunks)
⏰ 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)

@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 1.6750 -
This PR 1.8606 📈 +0.1856 (+11.08%)

✅ Difftest smoke test passed!

@XiangShanRobot
Copy link
Copy Markdown

[Generated by GEM5 Performance Robot]
commit: 3135da1
workflow: gem5 Ideal BTB Performance Test

Ideal BTB Performance

Overall Score

PR Previous Commit Diff(%)
Score 15.83 15.82 +0.08 🟢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants