transpose: Test EB=8 and misaligned bases, guard oversized elements - #165
Open
DanielKellerM wants to merge 4 commits into
Open
transpose: Test EB=8 and misaligned bases, guard oversized elements#165DanielKellerM wants to merge 4 commits into
DanielKellerM wants to merge 4 commits into
Conversation
- `compute_cfg` register is added after existing registers - Controls `enable`, `op`, `mode`, `tensor_m` and `tensor_n` - Connected to the relevant fields of the ND request struct in the register top-level template - Add required additional frontend IDs to CI deploy job
- Transpose-enabled iDMA would previously always read tile-sized chunks, leading to "overreads" (reading past the source matrix's backing memory) - Also, the transposed output had its row alignment padded to the tile width (which is the bus data width) - The `idma_transpose_req_replay` module prevents over-reading (and over-writing, although those writes are zero-strobed) by redirecting redundant reads and writes to known safe addresses, specifically src/dst addresses of the first read/write bursts - Compact output matrix storage is achieved by changing the destination stride in the transpose midend to the actual row width of the transposed matrix - Compact output mode is controlled by a new field `compact` in `transpose_options_t`, which is also configurable by the register frontend
- Replace the legacy Verilator elaboration target with reusable build, run, clean, per-testbench, and full-suite targets - Generate stable Bender file lists and track RTL, headers, DPI sources, and elaboration parameters so binaries rebuild only when their inputs change. - Add configurable tracing and C/C++ compiler and linker flags - Register the directed transpose, ND midend, register frontend, and runtime midend test matrices across relevant widths and operating modes - Extend transpose tests to cover compact and padded layouts, partial tiles, back-to-back requests, backpressure, and multiple bus widths - Use clocking blocks for race-free ready/valid stimulus and sampling in the clocked directed testbenches - Make the transpose DPI model link cleanly when Verilator compiles it as C++
The transpose testbenches derived the element-size mode with a ternary missing the eb==8 case, so EB=8 (mode 3) was silently mis-programmed as mode 0 and never exercised. Fix the helper in all three TBs, add EB=8 geometries to the nd and b2b testbenches, and sweep aligned and sub-beat misaligned src/dst bases in the nd TB (widening its AW guard to the aligned window, since a misaligned dst issues the first AW aligned-down). Add a datapath-aware midend assertion rejecting element sizes wider than the bus (E > StrbWidth) instead of relying on a silent NE underflow. EB=8 on the standalone otf 512-bit configs is omitted: it hits pre-existing Verilator --timing flakiness, not an RTL issue (EB=8 is covered end-to-end by the nd and b2b testbenches).
There was a problem hiding this comment.
🟡 Not ready to approve
There are a few concrete correctness/robustness issues in the new/modified test and build plumbing (EB=8 mode encoding in the midend TB, unconditional trace dumping, rsync dependency, and guarding invalid transpose expansion to avoid invalid shifts) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR tightens transpose coverage and robustness by extending the transpose testbenches to exercise previously untested element sizes and alignment cases, and by hardening the transpose midend against oversized element configurations. It also introduces shared Verilator build/run plumbing for the expanded test matrix.
Changes:
- Extend transpose tests to cover EB=8, compact vs tile-padded destinations, and sub-beat misaligned base addresses (plus clocking blocks to avoid handshake races).
- Add a transpose edge-descriptor replay stage to redirect invalid edge-row requests to safe mapped addresses.
- Replace the legacy Verilator make plumbing with a generic, parameterized flow (plus config fragments) and ensure DPI symbols link cleanly under C++.
File summaries
| File | Description |
|---|---|
| test/tb_idma_transpose_nd.sv | Adds compact/padded + aligned/misaligned sweeps, EB=8 geometry, transpose-midend + replay stages, and updated destination checking. |
| test/tb_idma_transpose_b2b.sv | Expands b2b regression to alternate compact/padded layouts and adds EB=8 coverage with clocking-block handshakes. |
| test/tb_idma_otf_transpose.sv | Uses a shared clocking block for stream handshake stability and fixes EB=8 mode encoding. |
| test/midend/tb_idma_transpose_midend.sv | Extends unit checks to validate both compact and padded expansion. |
| test/midend/tb_idma_rt_midend.sv | Moves bypass stimulus into a clocking block to avoid races. |
| test/midend/tb_idma_nd_midend_b2b.sv | Drives ND requests via a clocking block for race-free back-to-back stimulus. |
| test/idma_transpose_dpi.c | Wraps DPI functions in extern "C" for C++ compilation/linking. |
| test/frontend/tb_idma_reg_frontend.sv | Uses a backend clocking block to avoid arbitration/backpressure races. |
| src/midend/idma_transpose_req_replay.sv | New stage that replays invalid edge-row descriptors to safe mapped addresses. |
| src/midend/idma_transpose_midend.sv | Adds compact-row stride derivation and asserts to reject element sizes larger than the datapath. |
| src/idma_pkg.sv | Extends transpose options with a compact bit and documents EB=8 support. |
| src/frontend/reg/tpl/idma_reg.sv.tpl | Captures per-transfer compute/transpose configuration into the launched descriptor. |
| src/frontend/reg/idma_reg.rdl | Adds compute_cfg register with transpose mode/dims/compact control. |
| src/backend/idma_otf_transpose.sv | Updates documentation comment to include EB=8 mode. |
| target/sim/vlt/vlt.mk | New generic Verilator build/run plumbing (shared across testbenches). |
| target/sim/vlt/testbenches.mk | Declares testbench config suffix matrices and per-config elab/run args. |
| target/sim/vlt/.gitignore | Ignores Verilator build outputs under the new flow. |
| idma.mk | Switches to including the new Verilator make fragments. |
| Bender.yml | Adds new replay midend and test sources to targets. |
| .gitignore | Adds an editor artifact ignore pattern. |
| .github/workflows/deploy.yml | Adds IDMA_ADD_FE_IDS input and forwards it into the deploy build step. |
Review details
- Files reviewed: 20/21 changed files
- Comments generated: 4
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
60
to
62
| automatic int unsigned ne = StrbWidth/eb; | ||
| automatic int unsigned mode = (eb==4) ? 2 : (eb==2) ? 1 : 0; | ||
| automatic int unsigned yt = (m + ne - 1)/ne; |
Comment on lines
66
to
+70
| yt = (m + ne - 1) >>> log2ne; // ceil(M/NE) | ||
| nt = (n + ne - 1) >>> log2ne; // ceil(N/NE) | ||
| nxe = n <<< mode; // N*E (E = 1<<mode) | ||
| mpe = yt <<< Log2Strb; // MP*E = YT*NE*E = YT*StrbWidth | ||
| me = m <<< mode; // M*E: compact destination row size | ||
| mpe = yt <<< Log2Strb; // padded destination row size |
Comment on lines
+88
to
+91
| initial begin | ||
| $dumpfile("dump.fst"); | ||
| $dumpvars(0, tb_idma_transpose_nd); | ||
| end |
| $(BENDER) script verilator \ | ||
| $(IDMA_VLT_BENDER_ARGS) \ | ||
| > $(IDMA_VLT_FILELIST_TMP_DIR)/$*.flist | ||
| rsync -c $(IDMA_VLT_FILELIST_TMP_DIR)/$*.flist $@ |
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.
Follow-up to #161 (on-the-fly transpose). Stacked on #161 - the diff shows #161's commits until it lands, then collapses to just the final commit; merge after #161.
Closes test-coverage gaps found while auditing the transpose feature:
eb==8case, so EB=8 (mode 3) was silently programmed as mode 0. Fixed the helper in all three TBs and added EB=8 geometries to the nd and b2b testbenches. The engine and midend transpose EB=8 correctly (verified end-to-end).StrbWidth-aligned bases. Added an aligned + sub-beat-misaligned sweep so the transpose exercises the offset write/read path (the 2-beat-row case). Widened its AW-bounds guard to the aligned window, since a misaligned dst legitimately issues the first AW at the aligned-down address (strobe-masked partial beat).E > StrbWidth, replacing reliance on a silent NE underflow. Permits EB=8 on datapaths >= 8 B and cleanly rejects it on narrower ones.Verified: the full Verilator transpose suite (otf/nd/b2b/midend, all bus widths, both layouts, both alignments, EB=1/2/4/8) passes.
Not addressed here (pre-existing vlt-flow reliability issues, unrelated to the transpose RTL): the standalone otf 512-bit configs are flaky under Verilator 5.046
--timing(silent "end at 0s" / constructor segfault - the EB=8 otf case is omitted for this reason), andtb_idma_reg_frontendhangs under Verilator.