Skip to content

Sc ut#710

Merged
jensen-yan merged 6 commits intoxs-devfrom
sc-ut
Jan 20, 2026
Merged

Sc ut#710
jensen-yan merged 6 commits intoxs-devfrom
sc-ut

Conversation

@jensen-yan
Copy link
Copy Markdown
Collaborator

@jensen-yan jensen-yan commented Jan 14, 2026

add mgsc unit test

Summary by CodeRabbit

  • Refactor

    • Removed I-history tracking and simplified history flows across predictors.
    • Centralized initialization and storage for MGSC predictor tables for clearer startup, update, and recovery behavior.
  • Tests

    • Added a unit-test mode with deterministic, small-table configurations.
    • Introduced a comprehensive MGSC test suite validating prediction, learning, and recovery behaviors.

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

- Updated specUpdateIHist and recoverIHist methods to remove unused history parameters, passing an empty bitset instead.
- Removed references to IMLI history in DecoupledBPUWithBTB and FetchStream structures, as it is no longer utilized.
- Added notes in documentation to clarify that IMLI only uses counters, not history bits.
- Added conditional compilation for unit testing, allowing for a separate constructor and initialization logic.
- Introduced debug flags and test-specific structures to facilitate testing.
- Refactored the initStorage method to improve clarity and organization of storage initialization.
- Created a new test file for BTBMGSC to validate its functionality with unit tests.
- Updated SConscript to include the new test file in the build process.

Change-Id: Ib1ab026117ef87ebbf7097a8d32710843d32ed5b
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough

Removes IMLI (I-) history tracking and switches IMLI to counter-only, updates predictor interfaces and call sites (specUpdateIHist/recoverIHist), centralizes MGSC table allocation via initStorage(), and adds UNIT_TEST-scoped test scaffolding and a test-only constructor/namespace.

Changes

Cohort / File(s) Summary
MGSC Predictor
src/cpu/pred/btb/btb_mgsc.cc, src/cpu/pred/btb/btb_mgsc.hh
Add initStorage() to centralize allocation; introduce UNIT_TEST guards, test-only BTBMGSC() ctor and test namespace; add TestAccess; fold MgscStats to Scalar typedef in unit tests; change specUpdateIHist / recoverIHist signatures for IMLI counter-only behavior; guard commit/trace/test-only state.
Decoupled BPU
src/cpu/pred/btb/decoupled_bpred.cc, src/cpu/pred/btb/decoupled_bpred.hh
Remove s0IHistory and all ihistory allocation/propagation; update call sites to new specUpdateIHist() / recoverIHist() signatures that no longer take I-history bitsets.
Base Predictor Interface
src/cpu/pred/btb/timed_base_pred.hh, src/cpu/pred/btb/timed_base_pred.cc
Change virtual signatures: specUpdateIHist and recoverIHist drop history parameter; UNIT_TEST constructor path initializes enabled.
FetchStream / Stream Struct
src/cpu/pred/btb/stream_struct.hh
Remove ihistory member and its initialization from FetchStream.
Unit Tests & Build
src/cpu/pred/btb/test/SConscript, src/cpu/pred/btb/test/btb_mgsc.test.cc
Add mgsc.test GTest target and extensive BTBMGSC unit tests/harness; register test in UNITTESTS; many test utilities and expectations added.
Misc: minor adjustments
src/cpu/pred/btb/btb_mgsc.cc (local), timed_base_pred.*
Small internal refactors: dummy history usage for IMLI folds, lambda type dedup, spacing/comments and UNIT_TEST-scoped namespacing.

Sequence Diagram(s)

sequenceDiagram
    participant Fetch as Fetch (frontend)
    participant DBPU as DecoupledBPU
    participant Stream as FetchStream
    participant Predictor as BTBMGSC / TimedBase

    Note over Fetch,DBPU: Prior flow (with I-history)
    Fetch->>DBPU: request fetch
    DBPU->>Stream: create FetchStream (includes ihistory)
    DBPU->>Predictor: specUpdateIHist(ihistory, finalPred)
    Predictor-->>DBPU: prediction (may update history)
    DBPU->>DBPU: on squash/recover call recoverIHist(ihistory,...)

    Note over Fetch,DBPU: New flow (IMLI counter-only)
    Fetch->>DBPU: request fetch
    DBPU->>Stream: create FetchStream (no ihistory)
    DBPU->>Predictor: specUpdateIHist(finalPred)
    Predictor-->>DBPU: prediction (IMLI counters updated without history)
    DBPU->>DBPU: on squash/recover call recoverIHist(stream,...)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Mgsc2 align #686 — Modifies MGSC predictor internals and related structs; closely related to MGSC changes here.
  • bpu: test old: only enable sc imli table #707 — Changes IMLI/folded-history handling and counter behavior; directly related to switching IMLI to counter-only.
  • Use PHR in ITTAGE #628 — Updates decoupled BPU/BTB prediction APIs (specUpdateIHist/recoverIHist) and related flags; related to signature and call-site updates.

Suggested reviewers

  • Yakkhini
  • CJ362ff

Poem

🐇
I hopped through tables, trimmed a bit,
Removed old ihistory from the writ.
Counters hum where bits once played,
Tiny tests ensure the swap was made.
A carrot-cheer for cleaner trade!

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Sc ut' is vague and does not clearly convey what the changeset is about, despite the PR description clarifying it's about adding MGSC unit tests. Use a more descriptive title like 'Add MGSC unit tests' or 'Add BTBMGSC unit tests for local and path tables' to clearly indicate the main purpose of the changeset.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca94910 and 342e0c7.

📒 Files selected for processing (2)
  • src/cpu/pred/btb/btb_mgsc.cc
  • src/cpu/pred/btb/test/btb_mgsc.test.cc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T02:49:12.886Z
Learnt from: jensen-yan
Repo: OpenXiangShan/GEM5 PR: 686
File: src/cpu/pred/btb/btb_mgsc.cc:226-230
Timestamp: 2026-01-12T02:49:12.886Z
Learning: In src/cpu/pred/btb/btb_mgsc.cc, the calculateScaledPercsum function returns the unscaled percsum (weight scaling disabled) to align with RTL implementation. The RTL currently does not have a weight table, so setting weight to 1 (equivalent to disabling the weight table) maintains hardware/software consistency with minimal performance impact.

Applied to files:

  • src/cpu/pred/btb/test/btb_mgsc.test.cc
  • src/cpu/pred/btb/btb_mgsc.cc
🧬 Code graph analysis (2)
src/cpu/pred/btb/test/btb_mgsc.test.cc (1)
src/cpu/pred/btb/btb_mgsc.hh (36)
  • pc (195-195)
  • pc (240-240)
  • pc (245-245)
  • pc (252-252)
  • pc (255-255)
  • pc (258-258)
  • pc (258-258)
  • pc (272-275)
  • pc (272-272)
  • mgsc (506-506)
  • mgsc (506-506)
  • mgsc (507-507)
  • mgsc (507-507)
  • mgsc (509-509)
  • mgsc (509-509)
  • mgsc (510-510)
  • mgsc (510-510)
  • mgsc (511-511)
  • mgsc (511-511)
  • mgsc (512-512)
  • mgsc (512-512)
  • mgsc (513-513)
  • mgsc (513-513)
  • mgsc (514-514)
  • mgsc (514-514)
  • history (162-162)
  • history (163-163)
  • history (164-164)
  • history (166-166)
  • history (170-171)
  • history (172-173)
  • history (174-175)
  • history (178-179)
  • history (500-500)
  • pred (165-165)
  • pred (286-287)
src/cpu/pred/btb/btb_mgsc.cc (4)
src/cpu/pred/btb/btb_mgsc.hh (17)
  • table (202-203)
  • table (228-229)
  • numEntriesFirstLocalHistories (501-501)
  • pred (165-165)
  • pred (286-287)
  • entry (176-177)
  • entry (182-182)
  • entry (284-285)
  • history (162-162)
  • history (163-163)
  • history (164-164)
  • history (166-166)
  • history (170-171)
  • history (172-173)
  • history (174-175)
  • history (178-179)
  • history (500-500)
src/cpu/pred/btb/folded_hist.hh (2)
  • ImliFoldedHist (156-159)
  • PathFoldedHist (167-167)
src/cpu/pred/btb/timed_base_pred.hh (4)
  • pred (69-69)
  • pred (69-69)
  • entry (74-74)
  • entry (74-74)
src/cpu/pred/btb/stream_struct.hh (3)
  • shamt (606-631)
  • shamt (633-656)
  • entry (512-534)
🪛 Cppcheck (2.19.0)
src/cpu/pred/btb/test/btb_mgsc.test.cc

[error] 337-337: syntax error

(syntaxError)

src/cpu/pred/btb/btb_mgsc.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)


[error] 132-132: Reference to temporary returned.

(returnTempReference)


[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)


[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

⏰ 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). (1)
  • GitHub Check: Quick Build, Unit Tests & Smoke Test
🔇 Additional comments (13)
src/cpu/pred/btb/btb_mgsc.cc (5)

42-113: Well-structured centralization of table initialization.

The initStorage() function effectively consolidates all table allocations and folded history initializations. The pow2 lambda with the assert(width < 63) guard correctly prevents undefined behavior from excessive bit shifts. The assertions at lines 80-82 for IMLI history length are appropriate given ImliFoldedHist requires foldedLen >= histLen.


116-169: UNIT_TEST constructor provides minimal, deterministic configuration for fast tests.

The small table sizes and explicit configuration values support fast, reproducible unit tests. The comments explaining parameter choices (e.g., iHistLen({2}) due to ImliFoldedHist constraint at lines 129-131) are helpful.

Note: The static analysis warnings about "Reference to temporary returned" at lines 119 and 132 are false positives—these are valid member initializer list constructions for std::vector members like bwHistLen and iHistLen.


1120-1129: IMLI now counter-only with dummy bitset.

The signature change to remove the history parameter aligns with the summary indicating IMLI switched to counter-only mode. Using an empty boost::dynamic_bitset<> as a dummy is acceptable since ImliFoldedHist::update doesn't use the bitset for its counter-based logic.


1244-1257: Consistent counter-only approach for IMLI recovery.

The recoverIHist signature and implementation mirror the specUpdateIHist changes, maintaining consistency in the IMLI counter-only design.


686-692: Lambda parameter type deduction is appropriate here.

Using auto & for correct and wrong parameters in the recordPercsum lambda allows flexibility for different statistics counter types while maintaining reference semantics. This is idiomatic modern C++.

src/cpu/pred/btb/test/btb_mgsc.test.cc (8)

22-33: Helper creates valid conditional BTB entry for tests.

The makeCondBTBEntry helper correctly initializes a BTBEntry with all required fields for conditional branch testing. Setting target = pc + 4 as a forward branch default is sensible.


99-123: History manipulation helpers are correct.

histShiftIn correctly handles the shift-and-insert pattern for GHR/BWHR/LHR. pHistShiftIn correctly only updates path history when the branch is taken, which matches the semantics that path history captures taken branch targets.


125-333: MgscHarness provides comprehensive test scaffolding.

The harness correctly simulates the full prediction-update cycle:

  1. Saves pre-prediction history state
  2. Makes prediction via putPCHistory
  3. Speculatively updates folded histories
  4. On misprediction, recovers histories and applies correct outcome
  5. Trains predictor via update

The setOnly*Table() methods isolate individual table components for targeted testing, which is valuable for verifying each table's learning behavior independently.


337-354: Basic construction test validates MGSC initialization.

This test verifies that MGSC can be constructed under UNIT_TEST mode and correctly produces metadata even with empty BTB entries. The static analysis "syntax error" warning is a false positive caused by the analyzer not recognizing the GoogleTest TEST() macro.


536-584: GTable learning test validates global-history correlation.

The alternating T/N pattern is an excellent test case—a pure bias predictor achieves ~50% accuracy, while a history-indexed table should learn the 1-bit pattern. The test correctly:

  • Tracks distinct gIndex values to verify history affects indexing
  • Uses a warmup period before measuring accuracy
  • Sets an 80% accuracy threshold, which is reasonable for this pattern

631-675: ITable learning test validates IMLI trip-count correlation.

The T-T-T-N pattern (trip count of 3) is appropriate for testing IMLI, which uses consecutive backward-taken counts. The 85% accuracy threshold is reasonable given the 4-phase pattern where 3 phases are predictable once trained.


731-789: LTable test validates independent local history tracking.

Testing two different fetch blocks with opposite-phase alternating patterns is an excellent way to verify that local histories are truly independent. The assertion at line 746 correctly verifies the two addresses map to different local history slots.


791-847: PTable test validates path-target correlation learning.

This test cleverly uses branch A's alternating targets to create distinct path contexts for branch B's prediction. The test verifies that PTable can learn to correlate branch outcomes with the path history created by previous taken branches.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

@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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/cpu/pred/btb/btb_mgsc.cc (1)

1266-1273: Fix indentation error and stray closing brace.

Lines 1271-1272 have excessive indentation (12 spaces instead of 4), and line 1273 has an orphan } that shouldn't exist. This appears to be a merge or copy-paste error. The doUpdateHist call should be at the same level as the preceding for loop.

🐛 Proposed fix
     for (unsigned int k = 0; k < numEntriesFirstLocalHistories; ++k) {
         for (int i = 0; i < lTableNum; i++) {
             indexLFoldedHist[k][i].recover(predMeta->indexLFoldedHist[k][i]);
         }
     }
-            doUpdateHist(history[getPcIndex(entry.startPC, log2(numEntriesFirstLocalHistories))], shamt, cond_taken,
-                         indexLFoldedHist[getPcIndex(entry.startPC, log2(numEntriesFirstLocalHistories))]);
-        }
+    doUpdateHist(history[getPcIndex(entry.startPC, log2(numEntriesFirstLocalHistories))], shamt, cond_taken,
+                 indexLFoldedHist[getPcIndex(entry.startPC, log2(numEntriesFirstLocalHistories))]);
+}
🧹 Nitpick comments (1)
src/cpu/pred/btb/test/btb_mgsc.test.cc (1)

18-35: Good basic smoke test for BTBMGSC construction.

The test verifies that BTBMGSC can be constructed and handles empty input correctly. The assertions confirm metadata creation and empty prediction state.

Consider expanding the test suite with additional scenarios in follow-up PRs:

  • Non-empty history inputs
  • Multiple prediction cycles
  • History recovery paths
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0e63fe7 and f67d4d7.

📒 Files selected for processing (9)
  • src/cpu/pred/btb/btb_mgsc.cc
  • src/cpu/pred/btb/btb_mgsc.hh
  • src/cpu/pred/btb/decoupled_bpred.cc
  • src/cpu/pred/btb/decoupled_bpred.hh
  • src/cpu/pred/btb/stream_struct.hh
  • src/cpu/pred/btb/test/SConscript
  • src/cpu/pred/btb/test/btb_mgsc.test.cc
  • src/cpu/pred/btb/timed_base_pred.cc
  • src/cpu/pred/btb/timed_base_pred.hh
💤 Files with no reviewable changes (2)
  • src/cpu/pred/btb/stream_struct.hh
  • src/cpu/pred/btb/decoupled_bpred.hh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T02:49:12.886Z
Learnt from: jensen-yan
Repo: OpenXiangShan/GEM5 PR: 686
File: src/cpu/pred/btb/btb_mgsc.cc:226-230
Timestamp: 2026-01-12T02:49:12.886Z
Learning: In src/cpu/pred/btb/btb_mgsc.cc, the calculateScaledPercsum function returns the unscaled percsum (weight scaling disabled) to align with RTL implementation. The RTL currently does not have a weight table, so setting weight to 1 (equivalent to disabling the weight table) maintains hardware/software consistency with minimal performance impact.

Applied to files:

  • src/cpu/pred/btb/test/btb_mgsc.test.cc
  • src/cpu/pred/btb/btb_mgsc.hh
  • src/cpu/pred/btb/btb_mgsc.cc
🧬 Code graph analysis (5)
src/cpu/pred/btb/test/btb_mgsc.test.cc (2)
src/cpu/pred/btb/btb_mgsc.hh (11)
  • history (162-162)
  • history (163-163)
  • history (164-164)
  • history (166-166)
  • history (170-171)
  • history (172-173)
  • history (174-175)
  • history (178-179)
  • history (500-500)
  • pred (165-165)
  • pred (286-287)
src/cpu/pred/btb/btb_ubtb.hh (1)
  • meta (144-147)
src/cpu/pred/btb/timed_base_pred.hh (1)
src/cpu/pred/btb/btb_mgsc.hh (6)
  • pred (165-165)
  • pred (286-287)
  • history (162-162)
  • entry (176-177)
  • entry (182-182)
  • entry (284-285)
src/cpu/pred/btb/timed_base_pred.cc (2)
src/cpu/pred/btb/timed_base_pred.hh (2)
  • TimedBaseBTBPredictor (48-48)
  • TimedBaseBTBPredictor (53-53)
src/cpu/pred/BranchPredictor.py (1)
  • TimedBaseBTBPredictor (962-972)
src/cpu/pred/btb/btb_mgsc.hh (2)
src/cpu/pred/btb/timed_base_pred.hh (11)
  • pred (69-69)
  • pred (69-69)
  • entry (74-74)
  • entry (74-74)
  • entry (76-76)
  • entry (76-76)
  • entry (80-80)
  • history (66-66)
  • history (66-66)
  • history (67-67)
  • history (67-67)
src/cpu/pred/btb/btb_ittage.hh (1)
  • stream (115-115)
src/cpu/pred/btb/btb_mgsc.cc (3)
src/cpu/pred/btb/folded_hist.hh (2)
  • ImliFoldedHist (156-159)
  • PathFoldedHist (167-167)
src/cpu/pred/btb/timed_base_pred.hh (4)
  • pred (69-69)
  • pred (69-69)
  • entry (74-74)
  • entry (74-74)
src/cpu/pred/btb/stream_struct.hh (3)
  • shamt (606-631)
  • shamt (633-656)
  • entry (512-534)
🪛 Cppcheck (2.19.0)
src/cpu/pred/btb/btb_mgsc.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)


[error] 132-132: Reference to temporary returned.

(returnTempReference)


[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)


[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

⏰ 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). (1)
  • GitHub Check: Quick Build, Unit Tests & Smoke Test
🔇 Additional comments (17)
src/cpu/pred/btb/timed_base_pred.hh (1)

69-69: LGTM! Signature changes align with IMLI refactoring.

The removal of history parameters from specUpdateIHist and recoverIHist is consistent with the PR objective to remove IMLI history tracking. The simplified signatures match the overrides in btb_mgsc.hh (lines 164 and 175-176 in relevant snippets).

Also applies to: 74-74

src/cpu/pred/btb/decoupled_bpred.cc (2)

1218-1218: LGTM! Call site correctly updated for new signature.

The specUpdateIHist call now passes only finalPred, matching the updated signature in TimedBaseBTBPredictor. The surrounding code maintains updates for other history types (specUpdateHist, specUpdatePHist, specUpdateBwHist, specUpdateLHist), showing the I-history removal is isolated.


1314-1314: LGTM! Recovery call site properly aligned with signature change.

The recoverIHist call now passes (stream, real_bw_shamt, real_bw_taken), consistent with the updated base class signature. The parallel recovery calls for other history types remain unchanged, indicating a clean refactoring scope.

src/cpu/pred/btb/timed_base_pred.cc (1)

15-21: LGTM! Test constructor provides sensible defaults.

The UNIT_TEST constructor properly initializes all private members with reasonable test values:

  • numDelay(0) removes artificial delays for faster test execution
  • enabled(true) ensures the predictor is active during tests
  • Other values provide working defaults for unit test scenarios
src/cpu/pred/btb/test/SConscript (2)

31-36: LGTM! Test target configuration is correct.

The mgsc.test target includes the appropriate dependencies:

  • btb_mgsc.cc for the MGSC implementation
  • folded_hist.cc for folded history utilities used by MGSC
  • timed_base_pred.cc for the base predictor class

This matches the pattern used by similar tests like tage.test.


76-83: Test registration complete.

The mgsc.test is properly added to the UNITTESTS aggregate.

Note: abtb.test (defined on lines 16-21) appears to be missing from the UNITTESTS list. If this is unintentional, consider adding it in a follow-up.

src/cpu/pred/btb/btb_mgsc.cc (6)

42-113: LGTM: initStorage() centralizes table allocation logic.

The refactoring consolidates initialization code into a single method, reducing duplication between UNIT_TEST and production constructors. The pow2 lambda properly guards against shifting by 63+ bits with assert(width < 63).


115-162: LGTM: UNIT_TEST constructor with appropriate small configurations.

The test constructor uses small, deterministic values for fast unit tests. The comment at lines 127-128 correctly explains the ImliFoldedHist constraint requiring foldedLen >= histLen, and iHistLen({1}) satisfies this with foldedLen = iTableIdxWidth - log2(numCtrsPerLine) = 4 - 3 = 1.


1107-1115: LGTM: IMLI history update simplified to counter-only behavior.

The change aligns with the documented IMLI semantics (counters only, no history bits). Using an empty dummy bitset is appropriate since ImliFoldedHist::update() doesn't use the history parameter for its counter logic.


1231-1243: LGTM: IMLI recovery aligned with speculative update pattern.

The recoverIHist signature change matches specUpdateIHist, and the dummy bitset approach is consistent. The comment at line 1240 clearly documents the intent.


672-678: LGTM: Lambda signature uses auto for parameter deduction.

Using auto &correct, auto &wrong allows the lambda to work with both statistics::Scalar (production) and uint64_t (UNIT_TEST) types, maintaining flexibility across build configurations.


1416-1418: LGTM: Test namespace properly closed.

The test namespace wrapper is correctly closed only when UNIT_TEST is defined, maintaining consistency with the opening at line 38-40.

src/cpu/pred/btb/btb_mgsc.hh (5)

34-37: LGTM: Conditional namespace wrapper for test isolation.

The test namespace is properly wrapped with #ifdef UNIT_TEST guards, and correctly opened (lines 35-37) and closed (lines 543-545). This isolates the test BTBMGSC from the production version, preventing symbol conflicts during testing.

Also applies to: 541-545


165-177: LGTM: Updated method signatures match base class.

Both specUpdateIHist (line 165) and recoverIHist (lines 176-177) now omit the history parameter, aligning with the base class signatures in timed_base_pred.hh (lines 68 and 73). The override specifier ensures compile-time verification of the virtual function override.


418-422: LGTM: Scalar typedef enables dual-mode statistics.

The typedef pattern (uint64_t for UNIT_TEST, statistics::Scalar otherwise) allows MgscStats fields to be simple counters in tests while using gem5's statistics framework in production. The brace initialization {} for each field ensures zero-initialization in UNIT_TEST mode.

Also applies to: 425-490


184-186: LGTM: commitBranch excluded in UNIT_TEST builds.

This method depends on DynInstPtr which requires full gem5 infrastructure. Excluding it under UNIT_TEST avoids unnecessary dependencies for focused unit testing.


504-504: LGTM: initStorage() declared as private helper.

The new private method declaration supports the refactored initialization pattern in the implementation file.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 2.1691 -
This PR 2.1691 ➡️ 0.0000 (0.00%)

✅ Difftest smoke test passed!

- Increased index width for various tables in BTBMGSC to improve capacity.
- Added a TestAccess struct for unit testing, providing access to private members of BTBMGSC.
- Implemented new utility functions for handling BTB entries and indices in unit tests.
- Enhanced unit tests to validate the behavior of BTBMGSC under different conditions.

Change-Id: I4b76a70b1270d7fb96db816779b66dfaf192c370
- Introduced a new test case to validate GTable's learning of alternating patterns.

Change-Id: I2f55b712b92d119a6b4682d7a191ed54fb6c7607
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: 1

🤖 Fix all issues with AI agents
In `@src/cpu/pred/btb/btb_mgsc.cc`:
- Around line 1276-1278: In recoverLHist, the doUpdateHist(...) call and the
following closing brace are mis-indented and there is an extra closing brace
causing a compile error; locate the
doUpdateHist(history[getPcIndex(entry.startPC,
log2(numEntriesFirstLocalHistories))], shamt, cond_taken,
indexLFoldedHist[getPcIndex(entry.startPC,
log2(numEntriesFirstLocalHistories))]) line and the stray closing brace after
it, remove the superfluous brace and fix indentation so the doUpdateHist call
aligns with the surrounding block scope in recoverLHist.
🧹 Nitpick comments (1)
src/cpu/pred/btb/test/btb_mgsc.test.cc (1)

34-54: Consider adding bounds checking in lineLaneForHistIndex/lineLaneForBiasIndex.

These helpers assume table index 0 exists. While this is valid for the current test configuration (all tables have at least 1 entry), adding an assertion would make failures easier to debug.

♻️ Optional: Add defensive assertion
 std::pair<unsigned, unsigned>
 lineLaneForHistIndex(BTBMGSC &mgsc, Addr start_pc, Addr branch_pc, unsigned table_idx_width)
 {
     const unsigned num_bits = BTBMGSC::TestAccess::numCtrsPerLineBits(mgsc);
+    assert(table_idx_width > num_bits && "table_idx_width must be larger than numCtrsPerLineBits");
     const unsigned idx_bits = table_idx_width - num_bits;
     Addr line_idx = BTBMGSC::TestAccess::getHistIndex(mgsc, start_pc, idx_bits, /*foldedHist=*/0);
     auto [idx1, idx2] = BTBMGSC::TestAccess::posHash(mgsc, branch_pc, line_idx);
     return {idx1, idx2};
 }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f67d4d7 and ad08419.

📒 Files selected for processing (3)
  • src/cpu/pred/btb/btb_mgsc.cc
  • src/cpu/pred/btb/btb_mgsc.hh
  • src/cpu/pred/btb/test/btb_mgsc.test.cc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T02:49:12.886Z
Learnt from: jensen-yan
Repo: OpenXiangShan/GEM5 PR: 686
File: src/cpu/pred/btb/btb_mgsc.cc:226-230
Timestamp: 2026-01-12T02:49:12.886Z
Learning: In src/cpu/pred/btb/btb_mgsc.cc, the calculateScaledPercsum function returns the unscaled percsum (weight scaling disabled) to align with RTL implementation. The RTL currently does not have a weight table, so setting weight to 1 (equivalent to disabling the weight table) maintains hardware/software consistency with minimal performance impact.

Applied to files:

  • src/cpu/pred/btb/btb_mgsc.cc
  • src/cpu/pred/btb/test/btb_mgsc.test.cc
  • src/cpu/pred/btb/btb_mgsc.hh
🧬 Code graph analysis (3)
src/cpu/pred/btb/btb_mgsc.cc (3)
src/cpu/pred/btb/folded_hist.hh (2)
  • ImliFoldedHist (156-159)
  • PathFoldedHist (167-167)
src/cpu/pred/btb/timed_base_pred.hh (4)
  • pred (69-69)
  • pred (69-69)
  • entry (74-74)
  • entry (74-74)
src/cpu/pred/btb/stream_struct.hh (3)
  • shamt (606-631)
  • shamt (633-656)
  • entry (512-534)
src/cpu/pred/btb/test/btb_mgsc.test.cc (4)
src/cpu/pred/btb/btb_mgsc.hh (36)
  • pc (195-195)
  • pc (240-240)
  • pc (245-245)
  • pc (252-252)
  • pc (255-255)
  • pc (258-258)
  • pc (258-258)
  • pc (272-275)
  • pc (272-272)
  • mgsc (506-506)
  • mgsc (506-506)
  • mgsc (507-507)
  • mgsc (507-507)
  • mgsc (509-509)
  • mgsc (509-509)
  • mgsc (510-510)
  • mgsc (510-510)
  • mgsc (511-511)
  • mgsc (511-511)
  • mgsc (512-512)
  • mgsc (512-512)
  • mgsc (513-513)
  • mgsc (513-513)
  • mgsc (514-514)
  • mgsc (514-514)
  • history (162-162)
  • history (163-163)
  • history (164-164)
  • history (166-166)
  • history (170-171)
  • history (172-173)
  • history (174-175)
  • history (178-179)
  • history (500-500)
  • pred (165-165)
  • pred (286-287)
src/cpu/pred/btb/btb_mgsc.cc (4)
  • getHistIndex (907-918)
  • getHistIndex (908-908)
  • getBiasIndex (920-930)
  • getBiasIndex (921-921)
src/cpu/pred/btb/folded_hist.hh (1)
  • dynamic_bitset (105-105)
src/cpu/pred/btb/btb_ubtb.hh (1)
  • meta (144-147)
src/cpu/pred/btb/btb_mgsc.hh (1)
src/cpu/pred/btb/timed_base_pred.hh (7)
  • pred (69-69)
  • pred (69-69)
  • entry (74-74)
  • entry (74-74)
  • entry (76-76)
  • entry (76-76)
  • entry (80-80)
🪛 Cppcheck (2.19.0)
src/cpu/pred/btb/btb_mgsc.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)


[error] 132-132: Reference to temporary returned.

(returnTempReference)


[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)


[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

src/cpu/pred/btb/test/btb_mgsc.test.cc

[error] 263-263: syntax error

(syntaxError)

🔇 Additional comments (18)
src/cpu/pred/btb/btb_mgsc.hh (6)

34-37: LGTM on UNIT_TEST namespace wrapper.

The conditional namespace wrapper pattern isolates the test build properly, preventing symbol conflicts with production code.


42-47: LGTM on conditional constructor declaration.

The UNIT_TEST guard appropriately provides a default constructor for testing while maintaining the production constructor that requires parameters.


165-177: Interface changes align with base class declarations.

The specUpdateIHist and recoverIHist signatures now match the virtual declarations in timed_base_pred.hh. The removal of the history parameter is documented in the implementation as IMLI using counter-only behavior.


418-422: LGTM on Scalar typedef for statistics.

Using uint64_t for UNIT_TEST mode avoids dependency on the gem5 statistics framework while maintaining type compatibility for counter operations.


504-555: TestAccess helper is well-designed for unit testing.

The friend-like accessor pattern using static methods is a clean way to expose private members for testing without modifying the class interface. The accessors cover all necessary tables, flags, and index calculation methods.


596-600: LGTM on namespace closing.

The conditional namespace wrapper is properly closed to match the opening guard.

src/cpu/pred/btb/btb_mgsc.cc (5)

42-113: Good refactor: initStorage centralizes table allocation.

Extracting storage initialization into initStorage() eliminates duplication between the UNIT_TEST and production constructors. The local lambdas pow2 and allocPredTable keep the allocation logic readable.


1111-1120: LGTM on IMLI counter-only history update.

The change to pass a dummy empty bitset to doUpdateHist for IMLI is well-documented. The comment explains that ImliFoldedHist::update doesn't use the history bits, only the counter behavior.


1236-1248: LGTM on IMLI counter-only recovery.

Consistent with specUpdateIHist, the recovery path now uses a dummy bitset for IMLI, maintaining the counter-only behavior.


677-683: Lambda parameter type is appropriate.

The auto &correct, auto &wrong parameters in recordPercsum work correctly since they accept both uint64_t (UNIT_TEST) and statistics::Scalar (production) types through template deduction.


115-165: Assertions at line 82 correctly protect against shift UB; static analysis warnings are false positives.

The code is safe:

  1. Line 82 asserts iHistLen[i] < 63, preventing shift UB before any shift operations in pow2()
  2. Brace-initialized vectors (bwHistLen({4}), lHistLen({4}), etc.) are not references to temporaries—static analyzer false positives
  3. UNIT_TEST config with iHistLen({1}) satisfies the constraint: foldedLen = 5 - log2(8) = 2 >= histLen = 1
  4. calculateScaledPercsum() correctly returns unscaled percsum (weight scaling disabled) to align with RTL, as intended
src/cpu/pred/btb/test/btb_mgsc.test.cc (7)

21-32: LGTM on makeCondBTBEntry helper.

The helper properly constructs a minimal conditional BTB entry for testing with reasonable defaults.


98-122: LGTM on history shift helpers.

histShiftIn and pHistShiftIn correctly implement the history update semantics matching the production code's behavior for global and path histories.


124-259: MgscHarness provides comprehensive test scaffolding.

The harness correctly implements:

  1. Prediction via putPCHistory
  2. Speculative history updates
  3. Misprediction recovery with history rollback
  4. Training update via update()

The step() method mirrors the actual prediction/recovery/update flow in DecoupledBPUWithBTB.


263-280: Good basic construction test.

This test validates that:

  1. Default construction works under UNIT_TEST
  2. putPCHistory creates valid metadata
  3. Empty BTB entries result in empty condTakens

The Cppcheck "syntax error" is a false positive - TEST is a valid GoogleTest macro.


282-318: GateHighConfUsesSCWhenStrong test is well-designed.

The test verifies the high-confidence gating threshold: when abs(total_sum) > total_thres/2, SC should be used. The counter setup produces total_sum = 18 which exceeds 35/2 = 17, triggering SC usage.


398-460: UpdateOnlyOnWrongOrLowMargin test validates training conditions.

This test correctly verifies that:

  1. Correct predictions with high confidence don't trigger training
  2. Wrong predictions do trigger training (counter decrement)

This matches the update condition if (sc_pred_taken != actual_taken || abs(total_sum) < total_thres) in the implementation.


462-510: GTableLearnsAlternatingPattern is a valuable learning test.

This test validates that:

  1. The G-table can learn distinct contexts from GHR
  2. After warmup, accuracy exceeds 80% on an alternating pattern
  3. Multiple GHR contexts are actually observed (seen_g_indices.size() >= 2)

The 80% accuracy threshold is reasonable for a pattern that a simple predictor can only achieve ~50% on.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 2.1664 -
This PR 2.1664 ➡️ 0.0000 (0.00%)

✅ Difftest smoke test passed!

- Added new test cases to validate the learning of alternating patterns for different tables (BwTable, ITable, BiasTable).

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

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 2.1664 -
This PR 2.1664 ➡️ 0.0000 (0.00%)

✅ Difftest smoke test passed!

- Added new test cases to validate the learning of local and path tables under different conditions.
@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

Branch IPC Change
Base (xs-dev) 2.1664 -
This PR 2.1664 ➡️ 0.0000 (0.00%)

✅ Difftest smoke test passed!

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.

2 participants