scripts/run_vsim.sh: fix regression-script bugs and optimize vsim ci jobs for better speed and coverage. - #442
Open
nikgiu wants to merge 11 commits into
Open
scripts/run_vsim.sh: fix regression-script bugs and optimize vsim ci jobs for better speed and coverage.#442nikgiu wants to merge 11 commits into
nikgiu wants to merge 11 commits into
Conversation
…coverage (#438) - Merge the two duplicate `axi_xbar` case branches into one: the second branch (added in 78f2999) was shadowed by the first and never ran. The two parameter sweeps are complementary (exclusive/unique-id handling vs. ID-width usage, data width and pipelining), so both are now executed. - Scope SEEDS to each test (`local` copy) so per-TB seed additions (e.g. axi_lite_regs) no longer leak into subsequent tests of a full run. - Redirect vsim's stderr into the log (`2>&1` before `tee`, not after). - Reject any unrecognized dash-prefixed flag (`-*`) instead of only the `-*--*` pattern, which missed typos like `--foo`. - Fix stale `--random` reference in the header comment (`--random-seed`).
- Add VSIM_JOBS (bounded vsim/license concurrency); set to 2 to evaluate CI impact. 1 = legacy sequential path.
imchenwu
requested changes
Aug 25, 2026
imchenwu
left a comment
imchenwu
left a comment
Collaborator
There was a problem hiding this comment.
Hi Niccolo,
- I feel it unsafe to use
jobs -rpas it only looks at the running jobs, and if a job already faied at elaboration time, it won't be catched injobs -rpand thus the failure log would be missing in the ci. - For tb_axi_xbar parameter sweep: maybe add the case when
IdUsed == IdWidthfor the sweep to have better coverage? - I think all the
+acccan be dropped because it's a batch-only simulation.
Address review feedback on the bounded-parallelism approach: - Track launched vsim PIDs explicitly and wait on each one instead of scanning `jobs -rp`, which lists only running jobs and would silently drop a job that already failed (e.g. an elaboration error) before the drain, yielding a false-green CI. - Restore the IdUsed == IdWidth corner in the xbar sweep (distinct ID handling path). - Drop `+acc` from the xbar sweep: it only adds signal visibility for waveform/PLI, useless in a batch-only CI run.
Per review suggestion: rather than driving parallelism from run_vsim.sh with a bash job pool (VSIM_JOBS/throttle/drain), split the axi_xbar parameter sweep into two testbench-sharing CI jobs, axi_xbar and axi_xbar2, that the runner schedules concurrently. This keeps run_vsim.sh simple and sequential (fail-fast via set -e), makes per-shard results/logs easy to inspect, and lets CI concurrency limits govern simulator-license usage. - run_vsim.sh: drop the bounded-parallelism machinery; call_vsim is sequential again. Add the axi_xbar2 shard (sweep 2: ID-width usage, data width, pipelining) reusing tb_axi_xbar; axi_xbar keeps sweep 1. - .gitlab-ci.yml: add axi_xbar2 job; factor the shared xbar trigger set into a reusable anchor and add tb_axi_xbar_pkg.sv to it (was missing). Trade-off to evaluate with this trial: license capping now lives with CI job concurrency rather than the script. Backup of the bash approach kept at branch ng/vsim_opt-backup.
…ility +acc tells vopt to preserve access to internal objects for interactive debugging (waves, single-stepping, PLI). The CI runs are batch-only, so this only slows the simulations down. Coverage instrumentation is unaffected: +cover=bcesfx is kept where it was.
imchenwu
changed the title
The +cover=bcesfx / -coverage / -classdebug flags have collected coverage into memory since 2020, but no UCDB was ever saved, merged, or reported anywhere in the repository's history, so the data was discarded on every vsim exit. The instrumentation only cost simulation time (measured with questa-2025.1: tb_axi_addr_test 46 s -> 34 s, tb_axi_iw_converter 4.8 s -> 3.1 s per configuration). If code coverage is ever wanted for real, it needs the full loop (save per-run UCDBs, vcover merge, report, review); that should be introduced deliberately as its own change.
…lits Index every simulation in enumeration order and filter on CI_NODE_INDEX/CI_NODE_TOTAL, so splitting a heavy sweep is just `parallel: N` on its CI job. This removes the axi_xbar2 pseudo-test (sweep merged back into axi_xbar, identical configurations, verified via --list diff against the old enumeration). Also: per-config log files (kept as CI artifacts on failure) instead of one overwritten vsim.log, a --list mode to inspect sweep coverage, and a loud failure for a shard that executed zero simulations.
…DED opt-in GitLab sets CI_NODE_INDEX/CI_NODE_TOTAL on parallel:matrix jobs too, where they mean the matrix position. Trusting them unconditionally made the 12-module matrix job shard itself 12 ways: some modules executed nothing and failed, others silently ran a fraction of their configs and passed. Shard only when the job declares VSIM_SHARDED, as the axi_xbar job now does alongside its parallel: count. Also drop the vsim log upload from the test jobs.
Measured on pipeline 6db402a: dropping the dead coverage flags cut most test jobs by 30-70%, but axi_xbar barely benefits (132 min total, was 148) and its two shards (69/63 min) are now the longest jobs by far. Four shards of ~33 min each match the heaviest remaining job and the runner's four concurrent slots. Also complete the hand-maintained per-job trigger lists with the transitive dependencies they were missing, audited against the actual instantiation tree of every testbench. The recurring gaps: axi_err_slv internally instantiates axi_atop_filter, axi_demux_simple instantiates axi_demux_id_counters, axi_multicut instantiates axi_cut, axi_mux instantiates axi_id_prepend, axi_dw_converter instantiates both sizers, and axi_burst_splitter pulls in a whole chain via its _gran stage.
The rule was folded into an explicit path list when axi_xbar2 shared it (the $TEST_MODULE-derived paths would not exist for the pseudo-test). With axi_xbar2 gone, use the same anchor as every other job again; the extra rule keeps only the testbench package and the submodules.
Collaborator
|
I did some modifications.
|
imchenwu
marked this pull request as ready for review
August 27, 2026 13:31
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
2 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.
scripts/run_vsim.sh: fix regression-script bugs and restore axi_xbar coverage
Addresses the issues reported in #438.
Fixed
Scope SEEDS to each test (local copy) so per-TB seed additions (e.g. axi_lite_regs) no longer leak into the subsequent tests of a full run.
Redirect vsim's stderr into the log by moving 2>&1 before tee (it previously captured tee's stderr instead).
Reject any unrecognized dash-prefixed flag (-); the old ---* pattern missed typos such as --foo, which were then misread as testbench names.
Update the stale --random reference in the header comment to --random-seed.
Changed
Merge the two duplicate axi_xbar case branches into one. The second branch (added in 78f2999) was shadowed by the first and never executed; the two parameter sweeps are complementary (exclusive-access / unique-id handling vs. ID-width usage, data width and pipelining), so both are now run.
Not included (follow-up)
This does not implement the parallel execution requested in #299: the axi_xbar merge increases wall-clock time, and any parallelization must be license-bounded (a fixed concurrency cap) to avoid exhausting the shared Questa license pool — left as a separate change.