Skip to content

util: support config files in parallel_sim.sh#610

Merged
jensen-yan merged 1 commit intoxs-devfrom
fix-parallel
Jan 23, 2026
Merged

util: support config files in parallel_sim.sh#610
jensen-yan merged 1 commit intoxs-devfrom
fix-parallel

Conversation

@jensen-yan
Copy link
Copy Markdown
Collaborator

@jensen-yan jensen-yan commented Nov 24, 2025

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.sh to 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:

    • Auto-detect .sh vs .py files using file extension
    • Support optional 5th parameter for extra gem5 arguments
    • Update help message with examples
    • Maintain full backward compatibility with existing wrapper scripts
  • .github/workflows/gem5-perf-template.yml:

    • Add config_path parameter (recommended, replaces script_path)
    • Add extra_args parameter for gem5 command-line arguments
    • Keep script_path for backward compatibility
  • .github/workflows/gem5-ideal-btb-perf.yml:

    • Use configs/example/idealkmhv3.py directly
  • .github/workflows/gem5-ideal-btb-perf-nosc.yml:

    • Use configs/example/idealkmhv3.py with --disable-mgsc
  • .github/workflows/gem5-ideal-btb-perf-weekly.yml:

    • Migrate all jobs to use config files directly
  1. DRY compliance: Configuration logic only in .py files
  2. Flexible parameters: Support arbitrary gem5 arguments without new scripts
  3. Better traceability: Experiment logs show full config + arguments
  4. Backward compatible: Existing wrapper scripts continue to work
bash parallel_sim.sh kmh_v3_btb.sh workload.lst /cpt my_exp

bash parallel_sim.sh configs/example/idealkmhv3.py workload.lst /cpt my_exp

bash parallel_sim.sh configs/example/idealkmhv3.py workload.lst /cpt my_exp "--disable-mgsc"

Change-Id: I4c1f515b3913311fb05c0b9274e103460d349966

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

* **Chores**
  * Migrated performance testing workflows from shell script paths to Python configuration file paths across all test jobs
  * Enhanced test runner with support for additional arguments and configuration path validation
  * Updated all performance test workflows for consistency with the new configuration approach

<sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 24, 2025

📝 Walkthrough

Walkthrough

The PR systematically replaces the script_path parameter with config_path across multiple GitHub Actions workflows, migrating from shell script-based invocation to Python configuration-based invocation for gem5 performance tests. The supporting template and shell script are updated to handle both execution modes.

Changes

Cohort / File(s) Summary
Workflow Callers
.github/workflows/gem5-ideal-btb-perf-weekly.yml, gem5-ideal-btb-perf.yml, gem5-ideal-btb-0.3c.yml, gem5-ideal-rvv-simple-perf.yml, gem5-perf.yml, manual-perf.yml, on-demand-spec.yml
Replaces script_path input parameter with config_path; updates referenced values from shell script paths (e.g., ../kmh_v3_ideal.sh) to Python config paths (e.g., configs/example/idealkmhv3.py); minor consolidation of output handling in manual-perf.yml.
Workflow Template
.github/workflows/gem5-perf-template.yml
Replaces required input script_path with config_path and adds optional extra_args input; introduces validation logic to enforce repo-root-relative and .py file requirements; computes CONFIG_PATH from inputs and threads extra arguments through parallel_sim.sh; adds explicit environment variable setup for XS-GEM5 Run step.
Execution Script
util/xs_scripts/parallel_sim.sh
Introduces dual-mode execution: legacy mode (shell script) and config mode (Python config); detects execution mode from first parameter; adds config path transformation, environment initialization for gem5 (gem5_home, GEM5_BUILD_TYPE), and support for optional extra gem5 arguments in config mode; expands help text and adds diagnostic logging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • tastynoob
  • CJ362ff

Poem

🐰 Hops through workflows with glee,
Scripts become configs, now flowing free,
Dual modes dancing, legacy and new,
Gem5 runs swifter—what a delightful brew!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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 'util: support config files in parallel_sim.sh' directly and clearly describes the main change: adding support for config files (.py) to the parallel_sim.sh script.

✏️ 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.

❤️ 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: 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_home assignment has unquoted command substitution; should declare and assign separately
  • Line 38: gem5 assignment has unquoted realpath output; should quote the expansion

While 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 $?
     fi

Note: $extra_gem5_args is 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

📥 Commits

Reviewing files that changed from the base of the PR and between ed698e7 and d215cdc.

📒 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_path with config_path aligns 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-mgsc via extra_args to 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_path pointing 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

@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

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

✅ Difftest smoke test passed!

tastynoob
tastynoob previously approved these changes Nov 26, 2025
@github-actions
Copy link
Copy Markdown

🚀 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
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 @.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.

@github-actions
Copy link
Copy Markdown

🚀 Coremark Smoke Test Results

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

✅ Difftest smoke test passed!

@XiangShanRobot
Copy link
Copy Markdown

[Generated by GEM5 Performance Robot]
commit: bc36dd4
workflow: On-Demand SPEC Test (Tier 1.5)

Ideal BTB Performance

Overall Score

PR Master Diff(%)
Score 20.29 20.29 -0.02 🔴

@jensen-yan jensen-yan merged commit 2294581 into xs-dev Jan 23, 2026
4 checks passed
@jensen-yan jensen-yan deleted the fix-parallel branch January 23, 2026 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants