Prevent overwriting unconsumed tokens in register banks - #322
Conversation
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.
|
Hi @ShangkunLi, this PR is generated by Claude. It might take me some time to understand how it fixes the issue.. I will try to update during our weekly meeting. |
Thanks~ @tancheng, I am working on paper/proposal/interview issues these days (orz) and will find some for all these code reviews. |
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.
| # Token discipline only applies to armed registers; a never-written | ||
| # register keeps the legacy behavior of always asserting `val` on a | ||
| # configured read (acting as a default-token source). | ||
| s.armed = Wire(num_registers) |
There was a problem hiding this comment.
What is a default-token source, do we have a scenario that requires this?
There was a problem hiding this comment.
default is like read trash data from a register. For those cases, it default has one token, or don't care "token" concept.
Once a register is written, the "token" concept takes effect.
scenario given by Claude:
there are in-tree kernels that require it. In CgraRTL_test.py, the INC kernels execute OPT_INC with the operand read from bank 0 reg[2], which is never written by anything (there is no write_reg_from in that whole test)
| s.read_token_valid @= s.token_valid[r] | ||
| s.read_armed @= s.armed[r] | ||
| if s.inport_opt.write_reg_idx[reg_bank_id] == r: | ||
| s.outport_wr_rdy @= ~s.token_valid[r] |
There was a problem hiding this comment.
I think both s.read_token_valid and s.outport_wr_rdy should be Wire(num_registers) rather than Wire(1)
Besides,
s.read_token_valid @= s.token_valid[r] and s.outport_wr_rdy @= ~s.token_valid[r] prevent simultaneous reading and writing to the same register.
I guess maybe s.outport_wr_rdy @= ~s.token_valid[r] | (s.read_towards_fu & s.send_data_to_fu.val & s.send_data_to_fu.rdy) | (s.read_towards_xbar & s.send_data_to_xbar.val & s.send_data_to_xbar.rdy) | (s.read_towards_both......) can fix this?
There was a problem hiding this comment.
Thanks for the comment Yufei.
s.read_token_valid and s.outport_wr_rdy should be Wire(num_registers) rather than Wire(1)
token[] is already num_registers, so checking the or of them (i.e., s.outport_wr_rdy) is fine for each cycle. s.inport_opt.read_reg_idx[] should indicate the current accessing register at current cycle.
I guess maybe s.outport_wr_rdy @= ~s.token_valid[r] | (s.read_towards_fu & s.send_data_to_fu.val & s.send_data_to_fu.rdy) | (s.read_towards_xbar & s.send_data_to_xbar.val & s.send_data_to_xbar.rdy) | (s.read_towards_both......) can fix this?
Claude checked that if we write in that way, there will be combinational loop:
outport_wr_rdy
→ cluster: recv_data_from_fu_crossbar[i].rdy (写源的握手 rdy)
→ FU 的 send_out.rdy (FU 输出被谁接收)
→ FU 内部: recv_in[i].rdy @= recv_all_val & send_out.rdy (AdderRTL 的写法)
→ cluster: send_data_to_fu[i].rdy = FU 的 recv_in.rdy
→ 如果 wr_rdy 里 OR 了 (send_data_to_fu.val & send_data_to_fu.rdy)
→ 回到 outport_wr_rdy ←←← 环闭合了
And we insert a skip register in next PR (#330) to enable simultaneous read/write.
|
@yyan7223 @ShangkunLi can i get an approval? |
3 participants
Fixes #321.
Problem
RegisterBankRTLhad no notion of whether a register held an unconsumed token: a valid input directly overwrote the selected register, a configured read always assertedval, and nothing stopped a later iteration from clobbering an earlier token whose consumer was delayed by a variable-latency operation.Design
Each register entry tracks a token-valid bit and an "armed" bit (set on first write):
val, acting as a repeatable default-token source. Load-bearing for existing kernels (e.g.CgraRTL_test's INC kernels intentionally consume data from their own register cluster that nothing writes; CI round 1 hung without this).valonly while an unconsumed token is present (no more garbage reads).inport_ctrl_proceed— the same per-step signalconst_memalready advances on, wired identically in all three tile variants. Within a step, reads are level signals (as they were pre-token-tracking): FUs may accept the operand several times (vector-factor replays) or snoop it without any val/rdy handshake (VectorAllReduceRTLnever assertsrecv_in[1].rdyforADD_BASE_GLOBALyet needs the operand across both of its phases — handshake-based release deadlocked it in CI round 3).outport_wr_rdylets the cluster backpressure the selected write source (unselected sources stay always-ready; register-free configs untouched).read_reg_towards=BOTH, the tile's existing done-tracking only lets a step complete after both the FU and routing-crossbar paths are served — exactly the issue's release-after-both-destinations condition, with no per-register counter and no per-path state in the bank.clearinput driven byTileWithContextSwitchRTL's existing clear signal (CI round 2: state left by a terminated/migrated task starved the next task on that tile —test_multi_CGRA_fir_scalar_dynamic_migration). Register data itself is preserved.Structural notes:
recv_in.rdyderives combinationally from itssend_out.rdy, so same-cycle write-after-release would close a combinational loop through the FU.send_data_to_xbar);TileWithStreamingLoadRTLhas no reg→routing-crossbar path and ties it off (itsrdyis genuinely read now; undriven, it failed elaboration — theCgraWithStreamingLoadRTLfailures in CI round 3).Tests
DataType(0,0)reads are exactly the unarmed phase). Harnesses without ctrl stepping tieinport_ctrl_proceedlow; the two new tests emulate the tile's per-step done-tracking.test_reg_cluster_token_not_overwritten: back-to-back writes with a stalled consumer — the second write waits for the first token's step to complete; the FU sees100, 200(previously100was silently overwritten).test_reg_cluster_both_paths_skewed_acceptance:read_reg_towards=BOTHwith the xbar path accepting cycles after the FU — the token is held (not overwritten) until both paths are served and the step completes; each sink sees both tokens in order.CgraRTL_test×3,CgraRTL_fir_testterminate,CgraWithStreamingLoadRTL×2,MeshMultiCgraRTL_test::test_multi_CGRA_fir_vector_global_reduce,MultiCgraRTL_migration_test×3 — 10/10 passed (44 min).Known limitation
A same-register read-modify-write within a single ctrl step (FU output written back to its own operand register) deadlocks under this PR alone: the write is blocked while the token is present, and the step cannot complete until the write is delivered. This is resolved by the follow-up PR #330 (write skid buffer), which stacks on this branch.