Merged
Conversation
Change-Id: Ie8aef098adea8dab3d0dd7db34fa163517a850d7
Change-Id: I7d179632f0614a65654bc67e07ceca9991ee9d61
Change-Id: Idad43903835915098fcbecb2c78e3bb1442638ec
Change-Id: I9aa1d222463d6411b7481d3c93e7fd4fec17a039
📝 WalkthroughWalkthroughEnable MGSC in KMH v3 config; add/adjust MGSC table enable flags and TAGE bank-conflict setting. Add a focusBranchPC parameter to BTBMGSC and clamp/adjust MGSC threshold/gating logic. Add multiple new skill docs and CLI tools for BPU/MGSC analysis and CPT regression orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User/CI"
participant Probe as "mgsc_table_probe.py"
participant Gem5 as "gem5 (sim runner)"
participant Stats as "stats.txt / topMispredictsByBranch.csv"
participant DB as "MGSC DB (bp.db)"
participant Reporter as "Report builder"
User->>Probe: run(profiles, cases, flags)
Probe->>Gem5: spawn gem5 runs per (case,profile)
Gem5-->>Stats: write stats and topMispredicts
Gem5-->>DB: write/query MGSC trace DB
Probe->>Stats: parse stats & top CSV
Probe->>DB: query mgsc DB for per-PC metrics
Probe->>Reporter: aggregate results
Reporter-->>User: write summary.csv, branch_delta.csv, report.md/json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Change-Id: I7718a8e97835f34ff360850446f65b24740a0040
![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 the current code and only fix it if needed.
Inline comments:
In `@configs/example/kmhv3.py`:
- Around line 113-120: The comment above the MGSC table toggles is inaccurate:
it states “only enable bias and path tables” but the code also enables the
indirect table (cpu.branchPred.mgsc.enableITable = True). Update the comment to
accurately describe the enabled/disabled toggles (e.g., mention bias, path
(PTable), and ITable are enabled while BwTable, LTable, GTable and
enablePCThreshold are disabled) so it matches the actual flags
(cpu.branchPred.mgsc.enableBwTable, enableLTable, enableITable, enableGTable,
enablePTable, enableBiasTable, enablePCThreshold).
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: Iaf2a2f9504b6fddcd18f8c0ff70eb8fa981e4c21
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
- Added a new skill for extracting and summarizing BPU counters from gem5 simulation statistics, producing machine-readable JSON and CSV outputs. - Created a default configuration file for BPU counters, listing essential raw counter names. - Introduced a script to analyze BPU counters, allowing users to specify custom counter files and debug directories. Change-Id: Ie483baabbebfdef5e135738efc6e7b9579e67d57
- Introduced a new skill for analyzing branch PC semantics, determining if branches are naturally hard to predict or if the predictor needs improvement. - The skill includes guidelines for input prioritization, address classification, and a standard analysis workflow. - Outputs a comprehensive report on branch predictability, including code classification and semantic patterns. Change-Id: I65fe0f714dbea8b9eaf8a75543176c3fda76beb1
- Added a new skill for analyzing the effectiveness of SC tables in front-end micro-testing, including detailed usage instructions and output formats. - Implemented scripts and configuration files to facilitate batch analysis of SC sub-table effects and branch-level fixes. - Included reference materials for constructing effective micro-tests and evaluating their performance. Change-Id: I93290ac30d047d2251c7891f7512d7ecaae4f341
Change-Id: I85fe49785c272649ebee96e844375a46a0c4a2f8
- Changed the way BP db switches are constructed in the xiangshan configuration to ensure proper list handling. - Introduced a new parameter `focusBranchPC` in the BTBMGSC class to control trace writing for specific branch PCs. - Updated constructor and methods in BTBMGSC to accommodate the new parameter, enhancing branch prediction traceability. Change-Id: I4869bcd1f2b168ca341de4e066df67cfd6de738f
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
.codex/skills/frontend-pmu-analysis/SKILL.md (1)
17-18: Clarify which command--enable-bp-db tageapplies to.Line 18 mentions
--enable-bp-db tagebut it's ambiguous whether this is a gem5 simulation flag or an argument to the analysis script. Theanalyze_bpu_counters.pyscript doesn't have this flag; it appears to be a gem5 simulation option.Consider clarifying, e.g.: "在运行 gem5 仿真时，必要时可使用
--enable-bp-db tage来生成 tage trace db。"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/skills/frontend-pmu-analysis/SKILL.md around lines 17 - 18, The documentation is ambiguous about the context of the --enable-bp-db tage option; update the SKILL.md text around lines referencing `stats.txt` and `topMispredictsByBranch.csv` to explicitly state that `--enable-bp-db tage` is a gem5 simulation flag (not an argument to `analyze_bpu_counters.py`) and show the intended wording (e.g., "在运行 gem5 仿真时，必要时可使用 `--enable-bp-db tage` 来生成 tage trace db") so readers know to enable it during gem5 runs rather than in the analysis script..codex/skills/frontend-pmu-analysis/scripts/analyze_bpu_counters.py (1)
109-123: Consider movingrelative_toinside the try block for defensive error handling.Line 110's
relative_to(debug_dir)call is outside the try block. While unlikely with glob results, ifstats_pathis not underdebug_dir(e.g., via symlinks), this raisesValueErrorthat would bypass the error recording logic and propagate to the outer handler.♻️ Proposed defensive fix
def analyze_one(stats_path: Path, debug_dir: Path, counters: List[str]) -> CaseRecord: - case_rel = stats_path.parent.relative_to(debug_dir) + try: + case_rel = stats_path.parent.relative_to(debug_dir) + except ValueError: + case_rel = stats_path.parent record = CaseRecord( case_path=str(case_rel), stats_path=str(stats_path),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/skills/frontend-pmu-analysis/scripts/analyze_bpu_counters.py around lines 109 - 123, The call to stats_path.parent.relative_to(debug_dir) can raise ValueError and currently sits outside the try/except, so move the relative_to call inside the try block in analyze_one so any exception (ValueError or parse errors) is captured and appended to record.errors before returning; specifically, compute case_rel = stats_path.parent.relative_to(debug_dir) inside the same try that calls parse_last_stats_block (or wrap both operations in a new try) and on any exception append a message like "path relativize failed: {exc}" or reuse the existing error handling pattern and return the CaseRecord with the error recorded..codex/skills/run-cpt-regression/scripts/run_cpt_back.py (1)
145-147: Uselogging.exceptionto include traceback.When catching an exception in the executor, using
logger.exceptioninstead oflogger.errorwill automatically include the stack trace, making debugging easier.Proposed fix
except Exception as exc: fail += 1 - self.logger.error("Unhandled simulation exception on %s: %s", cfg.slice_name, exc) + self.logger.exception("Unhandled simulation exception on %s", cfg.slice_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/skills/run-cpt-regression/scripts/run_cpt_back.py around lines 145 - 147, Replace the call to self.logger.error in the exception handler with self.logger.exception so the traceback is recorded; specifically, in the except Exception as exc block that increments fail and logs for cfg.slice_name, change self.logger.error("Unhandled simulation exception on %s: %s", cfg.slice_name, exc) to self.logger.exception("Unhandled simulation exception on %s", cfg.slice_name) (keep the fail increment and the except block otherwise unchanged).src/cpu/pred/btb/btb_mgsc.cc (1)
797-797: Please add boundary tests for the new half-threshold training gateLine 797 changes training sensitivity (
abs(total_sum) < total_thres / 2), and Line 784 adds focus-PC trace filtering. Add unit tests for boundary values and focus match/non-match to lock in this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/pred/btb/btb_mgsc.cc` at line 797, Add unit tests for the new half-threshold training gate in the BTB MGSC predictor: create tests exercising the condition involving sc_pred_taken, actual_taken, total_sum and total_thres so you cover boundary cases where abs(total_sum) == total_thres/2, just below (should block training) and just above (should allow training), and also tests for sc_pred_taken != actual_taken vs equal to ensure the prediction mismatch path still trains; additionally add tests that exercise the new focus-PC trace filtering (the focus match and non-match cases introduced around the focus-PC trace filter) to confirm training is suppressed when focus doesn’t match and allowed when it does. Target the BTB_MGSC training entry point (the train/update method in btb_mgsc) and assert whether the internal counters/entries are updated or unchanged accordingly for each scenario. Ensure tests explicitly set total_sum and total_thres values and toggle focus-PC to deterministically hit each branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.codex/skills/mgsc-table-probe/scripts/mgsc_table_probe.py:
- Around line 336-339: maybe_copy_to_tmp currently writes every profile into the
shared path /tmp/{case.name}-riscv64-xs.bin which races when workers run in
parallel; modify maybe_copy_to_tmp (and the other copy site at the block
referenced around lines 357-358) to create a unique temporary file per
invocation (e.g., use tempfile.NamedTemporaryFile(delete=False, dir=str(run_dir)
or system tmp dir) or append process id / uuid to the filename) and copy
case.bin_path to that unique path with shutil.copy2, returning the unique Path
so concurrent runs do not conflict.
- Around line 246-255: Wrap the SELECT against MGSCTRACE in a try/except
catching sqlite3.OperationalError around the cur.execute(...) call (the one
populating overall_row) so a missing table doesn't raise out of the worker; on
exception assign a sensible default (e.g., tuple of zeros or None values) to
overall_row and log a warning; apply the same pattern to the other query block
referenced (the query at lines 280-287) so both queries safely handle an absent
MGSCTRACE table without aborting report generation.
- Around line 30-33: REPO_ROOT is set to Path(__file__).resolve().parents[1]
which points at .codex/skills/mgsc-table-probe instead of the project root;
update the REPO_ROOT calculation to use the correct ancestor (change parents[1]
to parents[4]) so DEFAULT_CONFIG and DEFAULT_GEM5 resolve relative to the actual
repo root, and leave a comment by DEFAULT_GEM5 noting that the binary is a build
artifact and may not exist until compilation or should be overridden at runtime.
In @.codex/skills/run-cpt-regression/scripts/run_cpt_back.py:
- Around line 71-73: The current code unconditionally overwrites entries in the
self.slices dict (self.slices[name] = str(binary)) which can silently replace
existing slices; change this to avoid silent overrides: either (preferred) skip
existing names by only assigning when name not in self.slices (i.e., move the
assignment inside the existing if block that increments discovered) so existing
entries remain untouched, or if overrides are intentional, add an explicit log
warning before overwriting (include the slice name and previous value) so
callers can detect replacements; reference the variables self.slices, name,
binary and the discovered increment logic when making the change.
- Around line 88-98: The SimConfig instances are currently created with
args=[""] which will pass an empty string as a spurious CLI argument when
expanded (see usages of config.args); change those to use an empty list
(args=[]) for both SimConfig constructions so no unintended "" argument is
included in the gem5 command line.
In @.codex/skills/run-cpt-regression/SKILL.md:
- Around line 43-50: The example uses a direct gem5 invocation with -P but the
skill is about the run_cpt_back.py wrapper; update the example to call python3
.codex/skills/run-cpt-regression/scripts/run_cpt_back.py and replace the
gem5-specific flags with the wrapper's options (e.g., use --param instead of -P,
supply --slices and --debug-dir, and keep --raw-cpt/--generic-rv-cpt semantics);
ensure the command shows the wrapper script name run_cpt_back.py and the --param
flag with the same parameter string so the example is consistent with the skill.
In `@configs/common/xiangshan.py`:
- Around line 370-373: The current construction of bp_db_switches from
args.enable_bp_db[1] can omit the required "basic" switch and thus silently
disable BPTRACE initialization in DecoupledBPUWithBTB::initDB(); update the
bp_db_switches creation (the code that reads args.enable_bp_db and assigns
bp_db_switches) to ensure "basic" is always present (e.g., check membership and
prepend/append if missing) before using or printing bp_db_switches so existing
--enable-bp-db behavior and the BPTRACE init path remain unchanged.
---
Nitpick comments:
In @.codex/skills/frontend-pmu-analysis/scripts/analyze_bpu_counters.py:
- Around line 109-123: The call to stats_path.parent.relative_to(debug_dir) can
raise ValueError and currently sits outside the try/except, so move the
relative_to call inside the try block in analyze_one so any exception
(ValueError or parse errors) is captured and appended to record.errors before
returning; specifically, compute case_rel =
stats_path.parent.relative_to(debug_dir) inside the same try that calls
parse_last_stats_block (or wrap both operations in a new try) and on any
exception append a message like "path relativize failed: {exc}" or reuse the
existing error handling pattern and return the CaseRecord with the error
recorded.
In @.codex/skills/frontend-pmu-analysis/SKILL.md:
- Around line 17-18: The documentation is ambiguous about the context of the
--enable-bp-db tage option; update the SKILL.md text around lines referencing
`stats.txt` and `topMispredictsByBranch.csv` to explicitly state that
`--enable-bp-db tage` is a gem5 simulation flag (not an argument to
`analyze_bpu_counters.py`) and show the intended wording (e.g., "在运行 gem5
仿真时，必要时可使用 `--enable-bp-db tage` 来生成 tage trace db") so readers know to enable
it during gem5 runs rather than in the analysis script.
In @.codex/skills/run-cpt-regression/scripts/run_cpt_back.py:
- Around line 145-147: Replace the call to self.logger.error in the exception
handler with self.logger.exception so the traceback is recorded; specifically,
in the except Exception as exc block that increments fail and logs for
cfg.slice_name, change self.logger.error("Unhandled simulation exception on %s:
%s", cfg.slice_name, exc) to self.logger.exception("Unhandled simulation
exception on %s", cfg.slice_name) (keep the fail increment and the except block
otherwise unchanged).
In `@src/cpu/pred/btb/btb_mgsc.cc`:
- Line 797: Add unit tests for the new half-threshold training gate in the BTB
MGSC predictor: create tests exercising the condition involving sc_pred_taken,
actual_taken, total_sum and total_thres so you cover boundary cases where
abs(total_sum) == total_thres/2, just below (should block training) and just
above (should allow training), and also tests for sc_pred_taken != actual_taken
vs equal to ensure the prediction mismatch path still trains; additionally add
tests that exercise the new focus-PC trace filtering (the focus match and
non-match cases introduced around the focus-PC trace filter) to confirm training
is suppressed when focus doesn’t match and allowed when it does. Target the
BTB_MGSC training entry point (the train/update method in btb_mgsc) and assert
whether the internal counters/entries are updated or unchanged accordingly for
each scenario. Ensure tests explicitly set total_sum and total_thres values and
toggle focus-PC to deterministically hit each branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60776e16-f506-45ff-85b9-dbe557ff8916
📒 Files selected for processing (15)
.codex/skills/branch-predictability-triage/SKILL.md.codex/skills/frontend-pmu-analysis/SKILL.md.codex/skills/frontend-pmu-analysis/configs/bpu_counters.txt.codex/skills/frontend-pmu-analysis/scripts/analyze_bpu_counters.py.codex/skills/mgsc-table-probe/SKILL.md.codex/skills/mgsc-table-probe/agents/openai.yaml.codex/skills/mgsc-table-probe/references/test-patterns.md.codex/skills/mgsc-table-probe/scripts/mgsc_table_probe.py.codex/skills/run-cpt-regression/SKILL.md.codex/skills/run-cpt-regression/scripts/run_cpt_back.pyconfigs/common/xiangshan.pyconfigs/example/kmhv3.pysrc/cpu/pred/BranchPredictor.pysrc/cpu/pred/btb/btb_mgsc.ccsrc/cpu/pred/btb/btb_mgsc.hh
✅ Files skipped from review due to trivial changes (2)
- .codex/skills/mgsc-table-probe/references/test-patterns.md
- .codex/skills/frontend-pmu-analysis/configs/bpu_counters.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/example/kmhv3.py
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
RTL bank conflict only decrease 0.1 score, no need to simulate it, code is dirty and complex Change-Id: I111af20037e6559d8abfdce5fa9c961775b91a19
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
configs/example/kmhv3.py (1)
111-118: SetenablePCThresholdexplicitly to match the alignment intent.The comment says PC-threshold is disabled, but this block currently relies on upstream defaults. Make it explicit in this config to avoid future default-drift.
Suggested fix
cpu.branchPred.mgsc.enablePTable = True cpu.branchPred.mgsc.enableBiasTable = True + cpu.branchPred.mgsc.enablePCThreshold = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/example/kmhv3.py` around lines 111 - 118, The RTL alignment intends PC threshold to be disabled but the config relies on upstream defaults; explicitly set cpu.branchPred.mgsc.enablePCThreshold = False in the same block where enableBwTable/enableLTable/enableITable/enableGTable/enablePTable/enableBiasTable are configured so the PC-threshold setting is deterministic and matches the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/pred/BranchPredictor.py`:
- Line 1060: Revert the global change to the Param.Bool default and keep
enableBankConflict as False in BranchPredictor.py (the Param named
enableBankConflict), then enable it only in the KMHV3-specific configuration
where KMHV3 is configured (set enableBankConflict = True in the KMHV3
config/class or builder that instantiates the BTBTAGE/BranchPredictor for KMHV3
runs); ensure you do not modify other global predictor defaults so BTBTAGE
behavior remains unchanged for non-KMHV3 configs.
---
Nitpick comments:
In `@configs/example/kmhv3.py`:
- Around line 111-118: The RTL alignment intends PC threshold to be disabled but
the config relies on upstream defaults; explicitly set
cpu.branchPred.mgsc.enablePCThreshold = False in the same block where
enableBwTable/enableLTable/enableITable/enableGTable/enablePTable/enableBiasTable
are configured so the PC-threshold setting is deterministic and matches the
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bba84371-c4ca-4150-86a7-1c8cff18745a
📒 Files selected for processing (2)
configs/example/kmhv3.pysrc/cpu/pred/BranchPredictor.py
Yakkhini
approved these changes
Mar 4, 2026
🚀 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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Performance Improvements
Configuration
New Features / Tools