Conversation
📝 WalkthroughWalkthroughReplaces previous kmhv3/idealkmhv3 references with three variants (kmhv3_align, kmhv3_18, kmhv3_22); updates CI workflow job script_path entries; adds Changes
Sequence Diagram(s)(omitted — changes are configuration/script swaps and new config/script additions; no new multi-component control flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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 |
![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
🤖 Fix all issues with AI agents
In `@configs/example/kmhv3_22.py`:
- Around line 130-133: The comment above the assignments to args.bp_type and
args.l2_size is misleading because the code unconditionally overwrites those
values; either update the comment to state these are intentional forced settings
for the "ideal_kmhv3" config, or change the code to only set defaults when not
provided (e.g., check if args.bp_type is falsy before assigning). Specifically,
modify the comment to reflect unconditional assignment for the ideal config or
wrap the assignments to args.bp_type and args.l2_size in a conditional that
respects existing user-provided values (and reference the ideal_kmhv3 flag in
the comment if needed).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/gem5-ideal-btb-perf-nosc.yml.github/workflows/gem5-ideal-btb-perf-weekly.yml.github/workflows/manual-perf.yml.github/workflows/pr-quick-check.ymlconfigs/example/kmhv3_18.pyconfigs/example/kmhv3_22.pyconfigs/example/kmhv3_align.pyutil/xs_scripts/kmh_v3_btb_18.shutil/xs_scripts/kmh_v3_btb_18_nosc.shutil/xs_scripts/kmh_v3_btb_22.shutil/xs_scripts/kmh_v3_btb_align.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T08:28:53.334Z
Learnt from: Lingrui98
Repo: OpenXiangShan/GEM5 PR: 649
File: configs/common/xiangshan.py:0-0
Timestamp: 2025-12-16T08:28:53.334Z
Learning: In XiangShan trace mode (configs/common/xiangshan.py), bootloaders are not needed. The correct approach is to set test_sys.workload.bootloader = '' (empty string) to indicate no bootloader, rather than providing a bootloader path.
Applied to files:
configs/example/kmhv3_22.py
🧬 Code graph analysis (1)
configs/example/kmhv3_22.py (4)
configs/example/kmhv3_18.py (1)
setKmhV3IdealParams(22-120)configs/common/FUScheduler.py (1)
KMHV3Scheduler(187-293)configs/common/xiangshan.py (2)
xiangshan_system_init(493-510)build_xiangshan_system(198-490)configs/common/Simulation.py (2)
setMemClass(95-98)run_vanilla(774-856)
🪛 Ruff (0.14.11)
configs/example/kmhv3_22.py
7-7: from m5.objects import * used; unable to detect undefined names
(F403)
9-9: from m5.util.fdthelper import * used; unable to detect undefined names
(F403)
15-15: from common.FSConfig import * used; unable to detect undefined names
(F403)
16-16: from common.SysPaths import * used; unable to detect undefined names
(F403)
17-17: from common.Benchmarks import * used; unable to detect undefined names
(F403)
19-19: from common.Caches import * used; unable to detect undefined names
(F403)
20-20: from common.xiangshan import * used; unable to detect undefined names
(F403)
49-49: KMHV3Scheduler may be undefined, or defined from star imports
(F405)
106-106: XSDRRIPRP may be undefined, or defined from star imports
(F405)
115-115: LayerBandwidthConfig may be undefined, or defined from star imports
(F405)
116-116: LayerBandwidthConfig may be undefined, or defined from star imports
(F405)
126-126: xiangshan_system_init may be undefined, or defined from star imports
(F405)
138-138: build_xiangshan_system may be undefined, or defined from star imports
(F405)
142-142: Root may be undefined, or defined from star imports
(F405)
🪛 Shellcheck (0.11.0)
util/xs_scripts/kmh_v3_btb_18_nosc.sh
[warning] 10-10: gem5 is referenced but not assigned.
(SC2154)
[warning] 10-10: gem5_home is referenced but not assigned.
(SC2154)
util/xs_scripts/kmh_v3_btb_22.sh
[warning] 10-10: gem5 is referenced but not assigned.
(SC2154)
[warning] 10-10: gem5_home is referenced but not assigned.
(SC2154)
util/xs_scripts/kmh_v3_btb_18.sh
[warning] 10-10: gem5 is referenced but not assigned.
(SC2154)
[warning] 10-10: gem5_home is referenced but not assigned.
(SC2154)
util/xs_scripts/kmh_v3_btb_align.sh
[warning] 10-10: gem5 is referenced but not assigned.
(SC2154)
[warning] 10-10: gem5_home is referenced but not assigned.
(SC2154)
🔇 Additional comments (14)
util/xs_scripts/kmh_v3_btb_18.sh (1)
10-10: LGTM!The config path update to
kmhv3_18.pyis consistent with the PR's renaming scheme. The shellcheck warnings about$gem5and$gem5_homeare false positives since these variables are sourced fromcommon.shon line 4.util/xs_scripts/kmh_v3_btb_18_nosc.sh (1)
10-10: LGTM!The config path update to
kmhv3_18.pyis consistent with the PR's renaming scheme while preserving the--disable-mgscflag. The shellcheck warnings are false positives as variables are sourced fromcommon.sh..github/workflows/gem5-ideal-btb-perf-nosc.yml (1)
16-16: LGTM!The script path update correctly references the renamed
kmh_v3_btb_18_nosc.shscript, maintaining consistency with the shell script renaming in this PR.util/xs_scripts/kmh_v3_btb_align.sh (1)
1-10: LGTM!The new script follows the same established pattern as other scripts in the
xs_scriptsfamily, correctly sourcingcommon.shand validating required variables before invoking thekmhv3_align.pyconfiguration. The shellcheck warnings are false positives as the variables are sourced fromcommon.sh.configs/example/kmhv3_22.py (2)
7-20: Star imports are conventional for gem5 configs.While Ruff flags these star imports (F403/F405), this pattern is consistent with other gem5 configuration files including
kmhv3_18.py. The symbols likeKMHV3Scheduler,XSDRRIPRP,LayerBandwidthConfig,Root, etc. are all provided by these imports as expected.
23-121: LGTM!The
setKmhV3IdealParamsfunction follows the same structure askmhv3_18.pywith well-organized parameter groupings for each CPU component (fetch, decode, rename, dispatch, scheduler, ROB, LSU, LSQ, caches). The L2 cache configuration properly handles both classic and non-classic modes with appropriate DRRIP replacement policy settings.util/xs_scripts/kmh_v3_btb_22.sh (1)
10-10: Config path update looks correct.The change from
idealkmhv3.pytokmhv3_22.pyaligns with the PR's renaming objectives..github/workflows/pr-quick-check.yml (2)
38-38: Config path update is consistent.The change from
kmhv3.pytokmhv3_align.pyis correctly applied to the Difftest smoke test step.
70-70: Base branch test uses matching config.The same
kmhv3_align.pyconfig is correctly used for the base branch comparison, ensuring consistent IPC measurements between branches..github/workflows/manual-perf.yml (2)
10-15: Configuration options correctly updated.The default and options are properly updated to reflect the new naming scheme:
- Default changed to
kmhv3_18.py- Old options (
idealkmhv3.py,kmhv3.py) removed- New options (
kmhv3_align.py,kmhv3_18.py,kmhv3_22.py) added
55-63: Script mappings are correct and all referenced scripts exist.Each configuration correctly maps to its corresponding script:
kmhv3_align.py→kmh_v3_btb_align.sh✓kmhv3_18.py→kmh_v3_btb_18.sh✓kmhv3_22.py→kmh_v3_btb_22.sh✓All scripts are present at
./util/xs_scripts/..github/workflows/gem5-ideal-btb-perf-weekly.yml (3)
14-21: Align test script paths updated correctly.Both
align_test_spec06andalign_test_spec17now referencekmh_v3_btb_align.sh, matching the renamed script.
26-33: Perf test script paths updated correctly.Both
perf_test_spec06andperf_test_spec17now usekmh_v3_btb_18.sh, aligning with the rename fromkmh_v3_ideal.sh.
35-45: New perf_test_22 jobs extend test coverage.The addition of
perf_test_22_spec06andperf_test_22_spec17jobs usingkmh_v3_btb_22.shextends the weekly performance test coverage to include the new KMHV3 22 configuration variant.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Set default bp_type based on ideal_kmhv3 flag | ||
| # If user didn't specify bp_type, set default based on ideal_kmhv3 | ||
| args.bp_type = 'DecoupledBPUWithBTB' | ||
| args.l2_size = '2MB' |
There was a problem hiding this comment.
Misleading comment: values are set unconditionally.
The comment states "If user didn't specify bp_type, set default based on ideal_kmhv3" but the code unconditionally overwrites bp_type and l2_size regardless of user input. If this is intentional for the "ideal" config, consider updating the comment to reflect that:
📝 Suggested comment fix
- # Set default bp_type based on ideal_kmhv3 flag
- # If user didn't specify bp_type, set default based on ideal_kmhv3
+ # Force ideal bp_type and l2_size (overrides command-line arguments)
args.bp_type = 'DecoupledBPUWithBTB'
args.l2_size = '2MB'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Set default bp_type based on ideal_kmhv3 flag | |
| # If user didn't specify bp_type, set default based on ideal_kmhv3 | |
| args.bp_type = 'DecoupledBPUWithBTB' | |
| args.l2_size = '2MB' | |
| # Force ideal bp_type and l2_size (overrides command-line arguments) | |
| args.bp_type = 'DecoupledBPUWithBTB' | |
| args.l2_size = '2MB' |
🤖 Prompt for AI Agents
In `@configs/example/kmhv3_22.py` around lines 130 - 133, The comment above the
assignments to args.bp_type and args.l2_size is misleading because the code
unconditionally overwrites those values; either update the comment to state
these are intentional forced settings for the "ideal_kmhv3" config, or change
the code to only set defaults when not provided (e.g., check if args.bp_type is
falsy before assigning). Specifically, modify the comment to reflect
unconditional assignment for the ideal config or wrap the assignments to
args.bp_type and args.l2_size in a conditional that respects existing
user-provided values (and reference the ideal_kmhv3 flag in the comment if
needed).
|
#610 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/gem5-ideal-btb-perf-weekly.yml:
- Around line 35-45: Remove the trailing whitespace after the closing quote in
the benchmark_type value for the perf_test_22_spec17 job (the string
"spec17-1.0c") so the YAML line ends cleanly; locate the perf_test_22_spec17
block (and perf_test_22_spec06 for comparison) and delete the extra space
character after the closing quote to ensure no trailing whitespace remains.
🧹 Nitpick comments (1)
.github/workflows/gem5-ideal-btb-perf-weekly.yml (1)
23-33: Consider cleaning up or removing disabled job blocks.The commented-out
perf_test_spec06andperf_test_spec17blocks use an unusual YAML comment style where the#is placed at the content's indentation level rather than at the line start. While functional, this can make future maintenance harder.If these jobs are intentionally disabled (per the commit message about disabling 18config weekly regression), consider either:
- Removing the blocks entirely if they won't be re-enabled soon.
- Using a consistent comment style with
#at the start of each line.♻️ Suggested cleanup (if keeping commented blocks)
- # perf_test_spec06: - # uses: ./.github/workflows/gem5-perf-template.yml - # with: - # script_path: ../kmh_v3_btb_18.sh - # benchmark_type: "spec06-1.0c" - # - # perf_test_spec17: - # uses: ./.github/workflows/gem5-perf-template.yml - # with: - # script_path: ../kmh_v3_btb_18.sh - # benchmark_type: "spec17-1.0c" + # perf_test_spec06: + # uses: ./.github/workflows/gem5-perf-template.yml + # with: + # script_path: ../kmh_v3_btb_18.sh + # benchmark_type: "spec06-1.0c" + # + # perf_test_spec17: + # uses: ./.github/workflows/gem5-perf-template.yml + # with: + # script_path: ../kmh_v3_btb_18.sh + # benchmark_type: "spec17-1.0c"
重命名配置文件： - idealkmhv3.py -> kmhv3_18.py - kmhv3.py -> kmhv3_align.py 重命名脚本文件： - kmh_v3_btb_nosc.sh -> kmh_v3_btb_18_nosc.sh - kmh_v3_btb.sh -> kmh_v3_btb_align.sh - kmh_v3_ideal.sh -> kmh_v3_btb_22.sh 更新脚本文件中的配置文件路径引用： - kmh_v3_btb_align.sh: 更新为使用 kmhv3_align.py - kmh_v3_btb_18_nosc.sh: 更新为使用 kmhv3_18.py - kmh_v3_btb_22.sh: 使用 kmhv3_22.py 更新 GitHub Actions workflow： - gem5-ideal-btb-perf-weekly.yml: 更新所有脚本路径引用以匹配新文件名 Change-Id: Ied453dba5571f51ff2e53cef13789228dc380d00
重命名配置文件： - idealkmhv3.py -> kmhv3_18.py - kmhv3.py -> kmhv3_align.py 重命名脚本文件： - kmh_v3_btb_nosc.sh -> kmh_v3_btb_18_nosc.sh - kmh_v3_btb.sh -> kmh_v3_btb_align.sh - kmh_v3_ideal.sh -> kmh_v3_btb_22.sh 更新脚本文件中的配置文件路径引用： - kmh_v3_btb_align.sh: 更新为使用 kmhv3_align.py - kmh_v3_btb_18_nosc.sh: 更新为使用 kmhv3_18.py - kmh_v3_btb_22.sh: 使用 kmhv3_22.py 更新 GitHub Actions workflow： - gem5-ideal-btb-perf-weekly.yml: 更新所有脚本路径引用以匹配新文件名 - manual-perf.yml: 更新配置选项和脚本路径映射，支持新的配置文件命名 * 移除旧选项 idealkmhv3.py 和 kmhv3.py * 新增选项 kmhv3_align.py、kmhv3_18.py、kmhv3_22.py * 更新默认配置为 kmhv3_18.py - pr-quick-check.yml: 更新配置文件路径引用 (kmhv3.py -> kmhv3_align.py) Change-Id: Ied453dba5571f51ff2e53cef13789228dc380d00
Change-Id: If90afc89aecc3353db47e1e9067d45d81eb9c870
Change-Id: Ic35df437ada363cafb71263e566a65b5c12ae14e
Change-Id: I4694b325c8401d8d703ef1c53a98e3609358bd60
2 participants
重命名配置文件：
idealkmhv3.py -> kmhv3_18.py
kmhv3.py -> kmhv3_align.py
重命名脚本文件：
kmh_v3_btb_nosc.sh -> kmh_v3_btb_18_nosc.sh
kmh_v3_btb.sh -> kmh_v3_btb_align.sh
kmh_v3_ideal.sh -> kmh_v3_btb_22.sh
更新脚本文件中的配置文件路径引用：
kmh_v3_btb_align.sh: 更新为使用 kmhv3_align.py
kmh_v3_btb_18_nosc.sh: 更新为使用 kmhv3_18.py
kmh_v3_btb_22.sh: 使用 kmhv3_22.py
更新 GitHub Actions workflow：
gem5-ideal-btb-perf-weekly.yml: 更新所有脚本路径引用以匹配新文件名
manual-perf.yml: 更新配置选项和脚本路径映射，支持新的配置文件命名
移除旧选项 idealkmhv3.py 和 kmhv3.py
新增选项 kmhv3_align.py、kmhv3_18.py、kmhv3_22.py
更新默认配置为 kmhv3_18.py
pr-quick-check.yml: 更新配置文件路径引用 (kmhv3.py -> kmhv3_align.py)
Summary by CodeRabbit
Chores
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.