Fix SpMV correctness and align RTL schedule - #332
Draft
n0thingNoob wants to merge 111 commits into
Draft
Conversation
Fixes two-cycles bug of NAH operation.
Fixes the bug of unexpected stall cycles during prologue.
…dividual CGRAs and add method to count valid tiles in ParamCGRA.
…ile2 needs to write the memory.
…ns for heterogeneous multi-CGRA architectures. Update CtrlMemDynamicRTL to correctly handle tile IDs in packet types.
…rite ports for tiles, enhancing support for heterogeneous multi-CGRA configurations.
…rts to avoid bitwidth mismatch.
[Feature] Support heterogeneous multi-CGRAs.
[Doc] Unify coordinate system of multi-cgra
…ks in single-CGRA mode, preventing MultiWriterError.
[Fix] Enhance CgraTemplateRTL to manage grounded ports for memory links in single-CGRA mode, preventing MultiWriterError.
[CleanUp][NFC] Standardize line endings to LF
Add shared const opcode definitions
Captures the non-obvious knowledge needed to work productively in this repo: the custom pymtl3 fork setup, how to actually exercise Verilog translation in tests (--test-verilog), RTLIR translator constraints, the ctrl-step protocol and val/rdy conventions (including FUs that snoop operands without handshakes), register-cluster token semantics, test-suite timing expectations, and how to distinguish local toolchain issues from real failures.
Fix/comp terminal predicate rtl
Fix/prologue synchronization
Per review: the content moves to AGENTS.md (the convention Codex and other agents read) with a tool-neutral header; CLAUDE.md becomes an @AGENTS.md import so Claude Code loads the same single source of truth. Also documents the naming conventions observed in the codebase: CamelCase classes/type names, snake_case functions and variables, UPPER_SNAKE_CASE constants, and the kCamelCase attribute keys.
Add CLAUDE.md with development guidance for AI coding agents
…tion # Conflicts: # fu/single/CompRTL.py # fu/single/test/CompRTL_test.py # lib/opt_type.py # noc/CrossbarRTL.py
Fixes #321. RegisterBankRTL previously had no notion of whether a register held an unconsumed token: a valid input directly overwrote the selected register, a configured read always asserted val (emitting garbage for never-written entries), and nothing stopped a later iteration from clobbering an earlier token whose consumer was delayed. Each register entry now has a token-valid bit: - Set when a token is written. - A read asserts val only while the selected entry holds a token that the corresponding destination path has not yet accepted. - Cleared only after every configured destination path completes a val/rdy handshake; for read_reg_towards=BOTH, per-path sticky taken bits let the FU and routing-crossbar paths accept in different cycles, and each path accepts the token at most once. - A write to an entry that still holds a token is rejected, and the bank exposes outport_wr_rdy so the cluster backpressures the selected write source (unselected sources keep their always-ready behavior, preserving register-free configurations). The write gate depends only on registered state (never on same-cycle release), avoiding a combinational cycle through the FU, whose recv_in.rdy depends on its send_out.rdy. The bank's read path towards the routing crossbar is now a real val/rdy handshake (send_data_to_xbar) instead of an unconditionally-asserted val, so the direct reg->routing_crossbar path participates in token release. Existing tests are updated for the new semantics (no leading garbage reads; each token delivered exactly once) and two new tests cover the overwrite-protection and skewed BOTH-path-acceptance behaviors.
The initial token implementation gated every configured register read on token presence, which starved existing kernels that intentionally read never-written registers as an always-valid default-token source (e.g. CgraRTL_test's INC kernels, documented as consuming data from their own register cluster without anything writing it). Those kernels deadlocked waiting for operands that never became valid, hanging CI. Each register now tracks an "armed" bit, set on its first write and kept until reset: - A never-written register keeps the exact legacy behavior: a configured read always asserts val (default-token source), reads are repeatable, and nothing is tracked. - Once armed, the full issue #321 token discipline applies: val only while a token is present, consume-once per destination path, release after all configured paths accept, and write backpressure while a token is unconsumed. Legacy (unarmed) read acceptances do not update the per-path taken bits, so they cannot poison the token bookkeeping of the first real token. The previously-failing kernels pass again (CgraRTL_test homogeneous 2x2 / ctrl_count_2 / king_mesh, CgraRTL_fir_test 4x4 terminate), and the register-cluster tests keep their original leading default-value reads, which are exactly the unarmed legacy phase.
test_multi_CGRA_fir_scalar_dynamic_migration hung (max_cycles timeout) because the token/armed/taken bits survived a task switch: after a task is terminated/migrated, the state its registers accumulated leaked into the next task on that tile, whose reads then starved on armed-but-empty registers instead of getting the legacy always-valid behavior a freshly launched task expects. The register bank now exposes a `clear` input, mirrored on the cluster, which resets all token bookkeeping (register data itself is preserved). TileWithContextSwitchRTL drives it from the same `clear` signal it already feeds to the FUs, crossbars, and const mem on CMD_TERMINATE; the plain and streaming tile variants tie it to 0 like their other clear ports.
The third CI round exposed two FU usage patterns that break handshake-based token release: - VectorAllReduceRTL snoops its register-sourced base operand (recv_in[1]) without ever asserting rdy for ADD_BASE_GLOBAL, and needs it again cycles later when the global-reduce response arrives. With release tied to a val/rdy handshake that never happens, the token was never consumed and the next iteration's write deadlocked (test_multi_CGRA_fir_vector_global_reduce hung at max_cycles). - Vector-factor replays fire the FU multiple times within one ctrl step, legitimately reading the same operand more than once. Within a ctrl step, register reads are therefore level signals, as they were before token tracking: consumers may accept or merely snoop them any number of times. The token is consumed when the ctrl step that reads the register completes, signaled by the new inport_ctrl_proceed (the same per-step signal the const queue already advances on, wired identically in all three tile variants). For read_reg_towards=BOTH, the tile's existing done-tracking only lets the step complete after both the FU and routing-crossbar paths have been served, which is exactly the release-after-all-destinations condition; the per-path sticky taken bits are no longer needed and are removed. Armed/unarmed semantics, write backpressure while a token is unconsumed, and clear-on-task-switch are unchanged. Also ties off the register cluster's send_data_to_routing_crossbar in TileWithStreamingLoadRTL, which has no direct register-to-routing- crossbar path; the interface's rdy is genuinely read now, so leaving it undriven failed elaboration (CgraWithStreamingLoadRTL tests). The cluster unit tests emulate the tile's per-step done-tracking to drive inport_ctrl_proceed where token consumption matters; harnesses without ctrl stepping tie it low (tokens held, level reads).
The comment on the FU-path priority mux still described the removed per-path taken-bit behavior; it now explains that the bank's send interfaces are direction-gated internally, which is why the cluster needs no additional read_reg_towards check. The kReadTowards* local aliases lost their last use when that check moved into the bank.
Symmetric with the send_data_to_xbar read interface introduced for the routing-crossbar path, and consistent with the cluster's own send_data_to_fu port naming.
Under token discipline, an operation that reads and writes the same register within one ctrl step (an accumulator, or #281's `NOT $0 -> $0, SOUTH` under backpressure) deadlocked: the write was rejected while the operand token was unconsumed, and the step could not complete until the write was delivered. Each bank now has a one-entry write skid buffer (one write port -> at most one blocked write). A write is accepted whenever the skid is free (outport_wr_rdy = ~skid_valid, pure registered state, so the producer's rdy never combinationally depends on any consumer's readiness and never collides with a commit on the single write port): - It lands directly if that cannot disturb an in-flight read (target token-free and not read by the current step), or at the boundary of the reading step (the write's token atomically replaces the consumed one; set wins over clear in the token update). - Otherwise it parks in the skid and commits once its target is no longer read by an open step: at that step's completion pulse, or immediately if the target is not being read and holds no token. The stability invariant — a register being read holds its value until the step completes — also fixes the partial-multicast corruption from issue #281 (a speculative register write landing while another fan-out leg is backpressured, followed by re-execution on the changed value), which the previous direct-write-if-token-free behavior still allowed. Tests: - test_reg_cluster_write_skid_buffer: three back-to-back writes with a stalled consumer flow through park/commit in order. - test_tile_same_register_accumulate: single-ctrl-word INC accumulator through one register with a stalled tile outport (the #281 shape); deadlocks without the skid buffer (verified), completes with it. - The cluster test harnesses now always emulate the tile's per-step done-tracking to drive inport_ctrl_proceed, since commit timing is tied to step completion.
This reverts commit ada5bd7.
This reverts commit b7d9b42.
…ClusterRTL" This reverts commit 84bad10.
…ndshake" This reverts commit 3d5325d.
This reverts commit 6d77971.
…en (armed)" This reverts commit 86e44ce.
This reverts commit 852fd0d.
Collaborator
Author
|
There are too many modifications in RTL files. We can review it later after the paper submission. |
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.
Summary
This PR fixes the VectorCGRA SpMV RTL validation so that the numerical result matches the CPU model and the steady-state schedule matches the compiler/Zeonica timing.
SpMV validation and memory layout
val=0,col=8,row=16,feature=24, andoutput=28.spmv.yaml.Generator fixes
invalid_iterations * II, instead of YAML/control-address order.RTL token and handshake fixes
GRANT_ONCEstate per control slot.CMD_COMPLETEso post-return stores can finish.Trace support
Root cause
The incorrect result had multiple causes: all GEP bases aliased the same memory region, constants were queued in a different order from their post-prologue consumption order, and several false/stalled tokens were retired incorrectly by the RTL.
After the numerical fixes, the measured RTL II was still 19 instead of the compiled II of 15. The four unrolled lanes were contending for the same single-port memory bank even though the compiler schedule assumed parallel accesses. Assigning one bank per lane removes that serialization.
Results
[1, 4, 9, 0, 5, 12, 21, 0].15.15.28cycles, RTL28cycles.42cycles, RTL43cycles.Validation
20 passed.git diff --check: passed.Branch note
This is intentionally a draft. The tested local validation baseline is ahead of the current remote
zeonica-bicg-validationbranch, so the PR history should be cleaned up or the base branch synchronized before final review.