ci: Verify the backend variants publicly with verilator and slang - #186
Merged
DanielKellerM merged 24 commits intoAug 17, 2026
Conversation
DanielKellerM
force-pushed
the
ci/public-verify
branch
from
August 13, 2026 10:03
6c8b784 to
f5c7607
Compare
This was referenced Aug 13, 2026
Public CI was lint and codegen only, so a port, parameter or connectivity break passed every gate; the proprietary EDA sims that would catch it run on the GitLab mirror and are skipped for fork PRs. Add a verilator elaboration job over all synthesis tops plus a concrete-typed inst64 wrapper, since idma_inst64_top has all-logic parameters and no public concrete instance.
A protocol present on both the read and the write side emitted the same key twice, so the write leg silently dropped the read channel.
Public CI could not see a port, parameter or datapath defect: the variant matrix runs on the private mirror with proprietary tools and is skipped for fork PRs, so collaborators had no visibility and every check spent a licence. Elaborate all eight backend variants under both tools across their real parameter sets, simulate the DPI-backed datapath tests byte-exact, run the negative guards with assertions enabled, and check the codegen and job tables stay consistent.
verify.yml already runs the same elaboration through idma_verify_shared, so the separate workflow only duplicated the run and installed Verilator its own way instead of through the shared setup action.
idma.mk references both, so the verification targets could not run from a clean checkout.
The existing job reports through reviewdog with -diff, so it only sees lines a pull request touched and a violation that predates it never fails anything. One had already accumulated unseen. Add a tree-scoped run over src/ and wrap the long localparam it found.
The inst64 testbench gives the frontend a concrete top, so the lint-only wrapper was a second copy of the same instantiation; the register frontend was elaborated for one variant of three, and no configuration ever turned compute on. Elaborate the testbench instead, add the missing register variants and a compute sweep, and reject sweep entries whose ops or tuning do not fit the RTL structs or enable no datapath at all.
DanielKellerM
force-pushed
the
ci/public-verify
branch
from
August 13, 2026 12:18
f5c7607 to
e75b80a
Compare
DanielKellerM
commented
Aug 13, 2026
DanielKellerM
left a comment
DanielKellerM
left a comment
Collaborator
Author
There was a problem hiding this comment.
in general lgtm, but fix commnets
Comment on lines
+45
to
+46
| # The licensed EDA pipeline runs only once the license-free matrix is green, | ||
| # so it confirms rather than being the only signal, and no licences are spent |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+35
to
+37
| # Tree-scoped counterpart to lint-sv: that job reports through reviewdog with | ||
| # -diff, so it only sees lines a PR touched and pre-existing violations never | ||
| # fail it. This one checks all of src/ so the backlog stays at zero. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+13
to
+18
| # Two tiers: elaboration first, simulation behind it. elab-backend, elab-shared-tops | ||
| # and elab-tb-shared run in parallel and every simulation job `needs:` all three, so a | ||
| # port, parameter or connectivity break fails in seconds instead of burning every | ||
| # simulation runner. `fail-fast: false` stays on the matrices, so the elaboration tier | ||
| # still reports every leg. elab-multihead, codegen-consistency and report-slang-extra | ||
| # are parallel peers, not part of the barrier: they gate nothing the simulations use. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+8
to
+11
| # License-free verification matrix. It runs on Verilator 5.020 (apt, ubuntu-24.04) | ||
| # and slang 11 (pyslang) only, so fork PRs - which never reach the proprietary EDA | ||
| # sims on the GitLab mirror - get a real gate, and the licensed run becomes a | ||
| # confirmation rather than the only signal. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+56
to
+57
| # Codegen hygiene, not verification: jobs.json and the generated tree must | ||
| # still describe the same design, and a second generation must be identical. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+26
to
+28
| // generated frontend under test: 0 = idma_reg32_3d, 1 = idma_reg64_2d, 2 = idma_reg64_1d. | ||
| // Only variant 0 is simulated; 1 and 2 are elaboration-only (their register maps differ | ||
| // and reg64_1d emits a flat 1D request), and the stimulus refuses to run on them. |
Comment on lines
+42
to
+44
| // idma data-path: reg32_3d is 32-bit over 3 ND dims, both reg64 variants are 64-bit. | ||
| // NumDim shapes the ND request type; reg64_1d takes the flat idma_req_t instead, so | ||
| // its ND type is unused and the 2-dim shape is kept only to keep the macro legal. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+61
to
+62
| // register map (idma_reg32_3d_addrmap_pkg): base + per-stream stride 0x4; the reg64 | ||
| // variants have their own map and are elaboration-only, so it is not modelled here |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
| // All three generated frontends share the parameter and port list; only the module | ||
| // name and the emitted request type differ. reg64_1d (NumDim=1) emits a flat | ||
| // idma_req_t, which is widened back to the ND shape the stimulus works on. | ||
| if (RegVariant == 32'd0) begin : gen_reg32_3d |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Comment on lines
+123
to
+124
| # Keys are direction-qualified: a protocol present on both sides (INIT) otherwise | ||
| # emits the same key twice and the write leg silently drops the read channel. |
Collaborator
Author
There was a problem hiding this comment.
remove verbose comments, max 1 line comment if really needed
Collaborator
Author
|
Comments trimmed in
Two things worth flagging. The 38-line header carried a NAMED EXCLUSIONS list - the six testbenches that are elaborated but not simulated, the DataWidth 512/1024 SIGSEGV cases, mxneg 4/11/12, and the 88 directed job files. That detail is now in the commit message and the PR body rather than the file. Say the word if you would rather it lived somewhere in-tree; my worry is that a reader of I over-reached once and backed it out. My first pass collapsed comment blocks across the whole of Re-verified after trimming: #192 is restacked on top. That rebase hit a conflict where this PR's comment trim met #192's |
The index was arbitrary, so 0 meant the 3D variant; it now matches the dimension count in the module name and the selector comes before the parameters it selects between.
The tracer covered every backend id in a single generated header, so its content depended on the id list rather than on its own inputs. That needed a recorded stamp to notice an id-list change, and the stamp made the tracked aggregates depend on a command-line variable: generating with IDMA_ADD_IDS and then without it left the tree in a state where the multihead build had already rewritten idma_generated.sv, which is how mxquant_32 came to segfault in a tree that had just run idma_verify_multihead. Each id now gets its own idma/tracer_<id>.svh, a pure function of the id in the target name; idma/tracer.svh keeps only the id-independent helpers. The tracked aggregates concatenate the literal IDMA_TREE_IDS and out-of-tree ids collect into idma_generated_add.sv behind the new add_ids bender target, so no command-line variable can change what a tracked build contains. The stamp and its IDMA_FORCE phony are gone. Both contract changes are breaking for downstream and are in the CHANGELOG.
Rewrite the twelve comments the previous trim left as sentence fragments, and restore the named-exclusion inventory: it records dropped coverage, so deleting it turned documented gaps into silent skips. Give run_vlt_sim.py a run and build timeout. Several testbenches here hang rather than fail, and a hang was reported as neither: it pinned a runner until the job limit. A timed-out leg now fails even when it is a negative test, since a hang is not a guard firing. Teach check_jobs.py that a negative-test case the testbench defines must be run or named as skipped, and that a legalizer compute guard must be proven to fire by some case or named as untested. Both lists move out of prose and into idma.mk. This immediately found ComputeDstTilelink, ComputeMxFp16Width and ComputeMxdequantLengthFits with no firing test at all.
verilator --timing lowers to C++20 coroutines and g++ 11 miscompiles them, so the wide compute simulations SIGSEGV before time 0 with an empty log. Nothing recorded that, and the one green run only worked because CXX=g++-13.2.0 was typed on the command line. Pick the newest available g++ instead, and fold it into IDMA_VLT_MAKEFLAGS rather than replacing it, so passing -j4 no longer silently drops the toolchain. idma_verify_toolchain fails with the reason instead of leaving a segfault to interpret. DataWidth 512 and 1024 were excluded from the compute sweep as verilator crashes. They are not: with the compiler pinned, mxquant runs both. The widths are now per-testbench, because mxroundtrip does not: above 256 it dies in a heap that valgrind traces to writes inside the generated root constructor, before time 0, where the same run of tb_idma_mxquant is clean. That is not a width limit and is recorded as the open bug it is. mxneg cases 4 and 12 stay skipped with the measured reason: they SIGSEGV on verilator 5.020 and pass on 5.046.
idma.mk had grown from 703 to 1188 lines, and 376 of those were the public verification machinery: verilator flag tables, slang configurations, the simulation legs and the codegen consistency check. A third of the file that describes how to build iDMA was CI plumbing, and a downstream reader looking for idma_hw_all had to walk past all of it. The split follows what each file owns rather than the line count: idma.mk keeps every variable the build defines, verify.mk only consumes them and contributes no build rules, so building iDMA never reads it. Behaviour is unchanged; the include sits after the definitions it needs and idma_clean_all still reaches idma_verify_clean across the boundary.
…iables The run set lived in make variables and the fan-out in the workflow, so the same fact was written twice and check_jobs.py existed to notice when the two copies disagreed. A drift checker is what you build once you have already lost a single source of truth, and adding two negative cases this week meant editing both files by hand. src/db/verify.yml now holds every leg, next to the protocol databases that already drive generation. util/run_verify.py runs a suite from it and emits the CI matrix from the same entries, and the workflow fans out through fromJSON rather than listing suites, so a suite added to the database gets a CI leg with no workflow edit and nothing generated is committed. Testbench tops come from bender instead of a hand-kept list of fifteen module names; it already owns the file set. The derived set covers the same tops plus the inst64 testbench the list had missed. The simulation exclusions are deliberately not subtracted from it: those tops elaborate, only their runs hang. check_jobs.py loses the matrix comparison, which policed the duplication that is now gone, and keeps the one that compares the run set against the legalizer guards, which is a real second source.
The testbench top list was a $(shell), whose exit status make discards, so any bender or uv failure emptied it and idma_verify_tb_shared passed having elaborated none of its sixteen tops. It is a file target now, checked non-empty at both ends. That was a vacuous gate of exactly the kind this branch adds checks to forbid. The compiler probe ran twenty-six subshells at parse time on every make in the repository, including builds that run no simulation, because the variable was recursive and referenced five times. One subshell now, and only from a recipe. The database was half inert: elab_widths and elab_compute were read by nothing while the real values sat in make variables, and a suite's exclude entries were never checked. Both are wired up, and re-adding an excluded width without dropping its note now fails. The per-suite simulation targets are one pattern rule taking each suite's prerequisites from the database, so a suite added there no longer gets a CI leg that fails with "No rule to make target". Also: idma_lint_elab hand-wrote a weaker verilator flag set than the one beside it, so two elaboration paths disagreed on -Werror-LATCH, -Werror-MULTIDRIVEN and -Werror-IMPLICIT; the pre-existing per-top verilator path had no references left; the slang -Wextra report was a non-gating job inside a gating workflow; and Verible was pinned to a moving branch. Comments are cut back to one line, and the exclusion inventory duplicated in the workflow header is deleted: the database carries it with the reasons.
The simulation half read from src/db/verify.yml while the elaboration half still kept its work in make: the shared synthesis tops, the multi-head ids, and the register variants as RegVariant:module records unpacked with IFS=: in a shell loop. That is the colon-packed form this branch claimed to have removed, so the claim was only true of half the file. All three are database entries now, read through the same file-target pattern as the testbench tops. What is left in verify.mk is how to invoke the tools, not what to run: paths, flag strings and bender target sets. Every list consumer now depends on its list and checks it is non-empty. Without that, a missing list makes cat fail, the loop runs zero times and make exits 0: the shared-tops leg did exactly that while this commit was being written, losing eight elaborations behind a green run, and only the OK count showed it.
The CI fan-out read the suites from src/db/verify.yml while idma_verify_all still named its six, so adding a suite gave it a CI leg and no local run. That is the duplication this branch removed from the workflow, recreated on the make side and pointing the other way. Both sides read the database now. Verified by adding a suite to a scratch copy: it appears in the CI matrix, in the local list and in its prerequisites without any other edit.
Two more vacuous gates. --tb-tops and --emit-matrix lacked the non-empty guard --emit already had, and ' '.join([]) still prints a newline, so test -s passed on an empty list and the testbench leg elaborated nothing while reporting success. The workflow then discarded the emitter's exit status inside $( ), which is what verify.mk warns against for $(shell): an empty suite list fanned out to zero jobs and the simulation tier read as green. The tb-top filter also matched /deps/ anywhere in an absolute path, so a checkout under such a directory filtered out every source in the repository. The backend-id matrix check was deleted on a false premise. CI fans the simulation matrix out over the database, but the backend ids are still a literal list in the workflow, so they can drift from IDMA_BACKEND_IDS; check_jobs.py compares them again. Three transpose geometries were dropped with no exclusion entry: StrbWidth 8 FullDuplex 0, and both 64-wide cases. All three pass under verilator, so they are legs now rather than a silent gap, and the suite goes from 23 to 26. idma_inst64_top traced a rw_axi_rw_init_rw_obi backend with the rw_axi macro, so the INIT and OBI channels were never traced. It uses its own variant's macro now. Also: IDMA_HJSON_ALL and IDMA_REG_CUST_ALL were used but never defined; .PHONY named a rule that does not exist while idma_hw_all was in no .PHONY; GIT and PRINTF were dead; md5sum | sort hid a missing file without pipefail; the multi-head "no-op assertion" now asserts; the generated lists rebuild when their emitter changes; SLANG resolves against the same environment as every other python call; and verify.mk's header claimed it adds no build rules while defining seven.
The per-variant macro name is longer than the rw_axi one it replaced, which pushed the call to 112 characters and failed the tree-scoped Verible gate.
Splitting IDMA_ADD_IDS into its own aggregate, adding an add_ids bender target and freezing IDMA_BACKEND_IDS was a build-system redesign with two breaking downstream changes, and it landed here only because the multi-head verify leg this branch adds leaves its extra ids in idma_generated.sv. Nothing on devel builds with IDMA_ADD_IDS and back, so devel never had the problem; this branch created it and then redesigned around it. The id lists, the file lists and the aggregate rules are byte-identical to devel again, and both breaking CHANGELOG entries are gone. The tracer entry stays: the per-id split is real and is what lets the recorded id stamp go. The contamination it was addressing is real, so the leg cleans up after itself instead: it drops the aggregates, regenerates, and asserts the add ids are not in the tracked file. Without that, every simulation after the multi-head leg compiles a larger model, and mxneg_6 exits 0 with its guard never firing under g++ 13 while passing under g++ 11.
The house rule is one line and the commit message owns the why, but reviewing a diff hunk by hunk never shows a comment block as a block, so eighteen of them survived two passes. Seven were in idma.mk. One is deleted outright, having described the reverted aggregate rework; the rest are one line each, including the eight-line header on the register frontend testbench.
The run set lived in src/db/verify.yml, a second database beside the one that already describes every variant, its testbench and its parameters. jobs.json is also what the internal CI generates its matrix from, so the two pipelines read two different files to answer the same question. The six suites are jobs.json entries now, each naming its testbench and carrying a verify block with its legs, expected outcome and DPI golden. The elaboration sweeps and the exclusion lists sit under a reserved _verify key. Neither shape has a synth_top, which is how the internal generator tells them apart from the variants it covers; that side is already released on the deploy tag, and its output is byte-identical with and without these entries. check_jobs.py now also requires a suite entry to name a testbench that exists, so a suite cannot reference a module nobody wrote.
Serialising the file with json.dump reflowed all 281 existing lines, because the file aligns its keys by column and the serialiser does not. The entries are appended as text now, so the diff is additions only and no existing line moves. The suite entries also declared an empty params block while every other entry carries its parameter set, leaving the standard widths implicit in the legs. They are declared per entry now and merged into each leg, so a leg spells out only what it varies.
DanielKellerM
added a commit
that referenced
this pull request
Aug 17, 2026
Follow-up to #186. Two toolchain pieces were fetched or rebuilt in every job. bender-install downloads a release tarball per job and a run spawns about twenty of them, which is how devel went red on an elaboration leg that had nothing wrong with it. The binary is cached and the install script skips the download when it finds one on PATH. The simulation legs compile a full verilator model per leg, nine in the mxneg job alone, with no object cache. ccache is installed and its directory cached; verilator uses it as OBJCACHE once present. The job prints the hit rate so a cache that stops matching is visible rather than silent. Bender moves to 0.32.1, the current release. Its flist-plus, verilator and vsim output is byte-identical to 0.32.0 on this tree. Verilator still comes from apt: it is pinned by the runner image at 5.020, and 5.046 fixes mxquant at the wide widths but breaks mxroundtrip at 256, so that swap belongs with #196.
This was referenced Aug 17, 2026
Merged
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
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.
Public CI could not see a port, parameter or datapath defect. The real variant matrix runs on the private GitLab mirror with proprietary tools and is skipped for fork PRs, so external collaborators had no visibility and every check spent a licence. This adds a license-free public matrix that gates first.
The matrix
idma_reg32_3d,idma_reg64_2d,idma_reg64_1d). Only the first was covered before.--assert.The run set lives in
src/db/verify.yml.util/run_verify.pyruns a suite from it and emits the CI matrix from the same entries, and the workflow fans out withfromJSON, so a suite added to the database gets a CI leg with no workflow edit and nothing generated is committed. Testbench tops come from bender rather than a hand-kept list.The toolchain decides whether this works at all
verilator --timinglowers to C++20 coroutines and g++ 11 miscompiles them: the wide compute simulations SIGSEGV before time 0 with an empty log. Nothing recorded that, and the only green run worked becauseCXX=g++-13.2.0happened to be on the command line.idma.mknow picks the compiler rather than inheriting it, folds it intoIDMA_VLT_MAKEFLAGSinstead of being replaced by a caller passing-j4, andidma_verify_toolchainfails with the reason instead of leaving a segfault to interpret.This also retired two exclusions that were wrong.
mxquantat DataWidth 512 and 1024 was listed as a verilator crash; it was the compiler, and both widths run now. Everything excluded is anexclude:entry in the database with a reason, andcheck_jobs.pyfails when a negative case or a legalizer guard is neither exercised nor excluded.Breaking changes
Two, both downstream-visible, both in CHANGELOG.md:
idma/tracer.svhkeeps only the shared helpers;IDMA_TRACER_<ID>lives inidma/tracer_<id>.svh. Downstream replaces the include with the per-id header. This is what removed the recorded id-list stamp: the tracer covered every id in one file, so its content depended on the id list rather than on its own inputs.IDMA_ADD_IDSvariants no longer land intarget/rtl/idma_generated.sv. They build viaidma_add_allintoidma_generated_add.svand reach Bender through a newadd_idstarget. OverridingIDMA_BACKEND_IDSfrom the command line now needs a precedingidma_rtl_clean.On the width diagnostic in the legalizer
An earlier version of this description claimed
ComputeMxdequantLengthFitscarried a real RTL bug. It does not, and the claim was wrong.(64'd1 << $bits(req_i.length))does overflow when the length field is 64 bits wide, and slang reports it where verilator does not. But the assertion conjoins($bits(req_i.length) < 64)ahead of it, so in the only case where the shift misbehaves the expression is already zero. The change to65'd1fixes a static width diagnostic and makes the intent explicit; it does not change behaviour, and the guard was never wrong at runtime.Verified
make idma_verify_allfrom a clean tree: rc=0, 24 elaboration OK, 23 sim PASS, 0 failures. Non-vacuity proven per gate by injection rather than assumed.Known limits, not closed here
tb_idma_mxroundtripabove DataWidth 256 dies in a heap abort that valgrind traces into verilator's generated model constructor, before time 0, wheretb_idma_mxquantis clean at the same width. Filed as test: tb_idma_mxroundtrip corrupts the heap at DataWidth above 256 #196; it also blocks a verilator 5.046 bump.mxnegcases 4 and 12 need DataWidth 1024, which SIGSEGVs on verilator 5.020 and passes on 5.046, soComputeMxFp16Widthhas no firing test on the pinned toolchain.mxnegcase 11 is never accepted, soComputeMxdequantLengthFitsis never sampled.ComputeDstTilelinkhas no TileLink variant to test against.util/slang_elab.pyduplicatespulp-platform/pulp-actions/slang, which cannot fail a job today. Fix proposed upstream in slang: Add an opt-in fail-on-error gate pulp-actions#41; tracked here by ci: Replace util/slang_elab.py with the shared pulp-actions slang action #197.-Wno-fataland promotes only LATCH/MULTIDRIVEN/IMPLICIT, so it is weaker than the slang leg, which runs-Werror.IDMA_REG_CPUIFis exercised only asapb4-flat.idma_reg64_2dandidma_reg64_1dgain elaboration only, never simulation.Scope
This is larger than "elaborate with slang, simulate with verilator", and a review of the branch made the case that the tracer split and the
IDMA_ADD_IDSrework are separate downstream-facing changes riding along in a CI PR. Both are here because removing the id-list stamp required them. If you would rather they landed on their own against 0.7.0, say so and I will split them out.