util: support config files in parallel_sim.sh#610
Conversation
📝 WalkthroughWalkthroughThe PR systematically replaces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the 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. 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: 3
🧹 Nitpick comments (2)
util/xs_scripts/parallel_sim.sh (2)
34-40: Fix shell quoting and declare-assign separation issues.Shellcheck flags several issues with command substitution quoting (SC2046, SC2155):
- Line 35-36:
gem5_homeassignment has unquoted command substitution; should declare and assign separately- Line 38:
gem5assignment has unquotedrealpathoutput; should quote the expansionWhile these paths are unlikely to contain spaces in practice, quoting prevents word-splitting vulnerabilities and follows shell best practices.
Apply this diff:
-export gem5_home=$(dirname $(dirname $script_dir)) -export GEM5_BUILD_TYPE=${GEM5_BUILD_TYPE:-opt} -export gem5=$(realpath $gem5_home/build/RISCV/gem5.$GEM5_BUILD_TYPE) +script_dir=$(dirname -- "$( readlink -f -- "$0"; )") +gem5_home=$(dirname "$(dirname "$script_dir")") +export gem5_home +export GEM5_BUILD_TYPE=${GEM5_BUILD_TYPE:-opt} +gem5=$(realpath "$gem5_home/build/RISCV/gem5.$GEM5_BUILD_TYPE") +export gem5
116-125: Quote variables in config mode execution.Line 123 does not quote
$extra_gem5_args. While this works for the current use case (single flags like--disable-mgsc), quoting would improve robustness against edge cases with spaces or special characters.Apply this diff:
if [ "$use_legacy_mode" = true ]; then # Legacy mode: call wrapper script - bash $arch_script $1 # checkpoint + bash "$arch_script" "$1" # checkpoint check $? else # New mode: directly call gem5 with config file # config_file is already an absolute path from realpath - $gem5 $config_file --generic-rv-cpt=$1 $extra_gem5_args + "$gem5" "$config_file" --generic-rv-cpt="$1" $extra_gem5_args check $? fiNote:
$extra_gem5_argsis intentionally left unquoted to allow multiple space-separated arguments to expand correctly, as they are already properly delimited by the template and shell parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/gem5-ideal-btb-perf-nosc.yml(1 hunks).github/workflows/gem5-ideal-btb-perf-weekly.yml(1 hunks).github/workflows/gem5-ideal-btb-perf.yml(1 hunks).github/workflows/gem5-perf-template.yml(2 hunks)util/xs_scripts/parallel_sim.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
util/xs_scripts/parallel_sim.sh
[warning] 36-36: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 36-36: Quote this to prevent word splitting.
(SC2046)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 43-43: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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: perf_test / XS-GEM5 - Run performance test (spec06-0.3c)
- GitHub Check: Quick Build, Unit Tests & Smoke Test
🔇 Additional comments (3)
.github/workflows/gem5-ideal-btb-perf.yml (1)
21-21: Parameter migration to config_path looks good.The replacement of
script_pathwithconfig_pathaligns with the new dual-mode design and is correctly pointing to the Python config file..github/workflows/gem5-ideal-btb-perf-nosc.yml (1)
16-17: Good use of the new extra_args parameter.The configuration correctly demonstrates the new feature: passing
--disable-mgscviaextra_argsto augment the gem5 invocation. This aligns with the PR's goal of supporting flexible parameters..github/workflows/gem5-ideal-btb-perf-weekly.yml (1)
14-14: Consistent migration to config_path across all jobs.All three jobs (spec06, spec17, spec06_vector) have been cleanly migrated to use
config_pathpointing to the same Python config file. The migration is consistent and maintains isolation of job-specific parameters (benchmark_type,vector_type,check_result).Also applies to: 20-20, 26-26
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
jensen-yan
force-pushed
the
fix-parallel
branch
from
d215cdc to
bc36dd4
Compare
|
🚀 Performance test triggered: spec06-0.8c |
- Replaced `script_path` with `config_path` in multiple workflow files to streamline configuration management. - Updated benchmark types and paths in `gem5-ideal-btb-0.3c.yml`, `gem5-ideal-btb-perf-weekly.yml`, `gem5-ideal-btb-perf.yml`, `gem5-ideal-rvv-simple-perf.yml`, `gem5-perf.yml`, and `manual-perf.yml`. - Enhanced `gem5-perf-template.yml` to support new config path requirements and added extra arguments handling for gem5 configurations. - Modified `parallel_sim.sh` to accommodate the new config file structure and improve usability for both legacy and new modes. Change-Id: I5208b7159a160cc08b8a0e347c5f2c412df11e96
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/gem5-perf-template.yml:
- Around line 139-142: The current validation block rejects any config_path not
ending in .py, which blocks supported legacy .sh configs used by
parallel_sim.sh; update the CONFIG_PATH check to accept both ".py" and ".sh" (or
remove the extension gate) so parallel_sim.sh can receive .sh scripts. Locate
the conditional using CONFIG_PATH in the workflow and change the test to allow
*.py or *.sh (or eliminate the guard and rely on parallel_sim.sh to validate),
and ensure the error message reflects the accepted extensions if you keep the
check.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Ideal BTB PerformanceOverall Score
|
4 participants
Previously, running experiments with different configurations required creating separate wrapper scripts (kmh_v3_btb.sh, kmh_v3_ideal.sh, kmh_v3_btb_nosc.sh, etc.). This violates the DRY principle and makes configuration management difficult.
Modified
parallel_sim.shto auto-detect file type and support both modes:Legacy mode: Execute .sh wrapper scripts (backward compatible)
New mode: Directly invoke gem5 with .py config files
util/xs_scripts/parallel_sim.sh:.github/workflows/gem5-perf-template.yml:config_pathparameter (recommended, replaces script_path)extra_argsparameter for gem5 command-line argumentsscript_pathfor backward compatibility.github/workflows/gem5-ideal-btb-perf.yml:configs/example/idealkmhv3.pydirectly.github/workflows/gem5-ideal-btb-perf-nosc.yml:configs/example/idealkmhv3.pywith--disable-mgsc.github/workflows/gem5-ideal-btb-perf-weekly.yml: