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.
| 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.
There was a problem hiding this comment.
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.
Yes, you are right, one RegisterBank only have one rd port, so Wire(1) for s.outport_wr_rdy is fine. I thought it was for RegisterCluster.
Claude checked that if we write in that way, there will be combinational loop:
I agree with the combinational loop. But theoretically, reading and writing to the same register at the same cycle is a natural behavior, so the constraint below is a bit counterintuitive.
VectorCGRA/mem/register_cluster/RegisterBankRTL.py
Lines 167 to 168 in 8ae40fe
@tancheng Could you please ask Claude whether it is possible to remove this constraint, and let it designs from the scratch to enable simultaneous reading and writing? PR (#330) to me is kind of a patch, maybe we can avoid this from the beginning.
If Claude denies it and explains that the skip register is the only way, then I will go and review the next PR (#330)
There was a problem hiding this comment.
By the way, why we have
VectorCGRA/mem/register_cluster/RegisterClusterRTL.py
Lines 98 to 99 in 8ae40fe
Shouldn't it be
== instead of !=?
There was a problem hiding this comment.
This design does not "disallow same-cycle read+write" — it aligns the write's commit into the register file with the boundary of the reading step. The boundary cycle is itself exactly a standard same-cycle read+write: the old value is read throughout the cycle, and the new value lands at the clock edge. In the middle of a step, the pending write is held in the skid buffer instead, which both protects the in-flight read (the register stays stable for any re-reads) and keeps the producer's write handshake unblocked.
There was a problem hiding this comment.
- the old value is read throughout the cycle, and the new value lands at the clock edge
This is what we want, but the design now clearly disable this by:
and
- In the middle of a step, the pending write is held in the skid buffer instead, which both protects the in-flight read (the register stays stable for any re-reads) and keeps the producer's write handshake unblocked.
Besides, why we need the skid buffer? For me, if a value
Acan be read from the register bank atcycle N, a new valueBcan be directly written into the register bank at the rising edge ofcycle N+1, and the register bank holds valueBduringcycle N+1. Why we need a additional skid buffer to hold valueBuntilcycle N+2?@tancheng Please ask Claude if you agree with me.
- 那两行链接指向的是 master 上 PR #322 单独存在时的版本，确实会不管是不是 step 边界都卡死写入；但下一个 PR（skid buffer 那个）已经把这部分逻辑整个重写了——outport_wr_rdy 改成只看 skid buffer 是否空闲，是否直接落地则显式判断"当前 step 是否恰好在这一拍结束"（ctrl_proceed），也就是专门加了这个边界条件来实现"边界拍等价于标准同拍读写"的效果，之前那两行在新版本里已经不存在了。
- 不能固定"读的下一拍就写入"，是因为在这个架构里一次读不一定只占一个 cycle——同一个 step 可能因为下游背压或多拍重放而反复多拍读取同一个寄存器，如果写入不等 step 真正结束就直接落地，中途那些重复读会读到被提前覆盖的新值，造成数据错误。设计上把"接受这次写"（不卡生产者的握手）和"真正写进寄存器"（等到确认这个 step 不会再读旧值为止）拆成两步，正是为了避免这种情况；而在最简单的单拍 step 场景下，这套机制退化出来的效果和"读的下一拍就写入"完全一样、没有额外延迟，只有在 step 拖了多拍时才会多等，且只会等到刚好安全为止。
There was a problem hiding this comment.
- 不能固定"读的下一拍就写入"，是因为在这个架构里一次读不一定只占一个 cycle——同一个 step 可能因为下游背压或多拍重放而反复多拍读取同一个寄存器，如果写入不等 step 真正结束就直接落地，中途那些重复读会读到被提前覆盖的新值，造成数据错误。设计上把"接受这次写"（不卡生产者的握手）和"真正写进寄存器"（等到确认这个 step 不会再读旧值为止）拆成两步，正是为了避免这种情况；而在最简单的单拍 step 场景下，这套机制退化出来的效果和"读的下一拍就写入"完全一样、没有额外延迟，只有在 step 拖了多拍时才会多等，且只会等到刚好安全为止。
All right, here is a new example,
Case 1: If a value A is read from the register bank at cycle N and we also have ctrl_proceed = 1 at cycle N, a new value B (either from other tiles or FU of current tile) can be directly written into the register bank at the rising edge of cycle N+1. In this case, we don't need the skid buffer, right?
Case 2: If a value A is read from the register bank at cycle N but we have ctrl_proceed = 0 at cycle N, then due to the back pressure, the new value B will either still be in tile's ChannelRTL or just haven't been generated by FU, right? Tell me why we still need the skid buffer in this case?
@tancheng Please send my comment back to Claude.
There was a problem hiding this comment.
Case 1: exactly.
Case 2：这里的假设不成立——ctrl_proceed=0 不能保证 B 还没到。
Yufei 的推理前提是"B 要么还卡在 ChannelRTL 里，要么 FU 还没算出来"，隐含假设是"B 的到达"和"这个 step 为什么没 proceed"是同一个原因造成的。但至少有两种情况这个假设不成立：
写入源和导致 step 卡住的原因根本是两码事。 比如 read_reg_towards = BOTH（$0 既要发给 FU，又要经 routing crossbar 发到别的 tile），发往 crossbar 那条路被下游背压卡住导致 ctrl_proceed=0；但同时 write_reg_from 配置成 PORT_ROUTING_CROSSBAR，也就是别的 tile 正往 $0 写一个全新的值——这个写入跟"本 tile 往外发的那条路径卡没卡住"完全无关，它随时可能已经到达、随时可以被接受。这时候 ctrl_proceed=0，但 B 已经在门口了，不是"还没生成"也不是"卡在 channel 里"。
同一个 FU 做 vector-factor replay 时。 同一条指令的操作数可能要被反复读多次（同一个 step 内多轮 replay）才算完成，这时候 FU 可能早早就用第一次读到的 A 算出了 B 并尝试写回，但这个 step 还没结束（还要再读几轮 A），ctrl_proceed 依然是 0——B 这时候也是"已经到了"，不是"还没算出来"。
更根本的原因：outport_wr_rdy 故意不能依赖 ctrl_proceed，否则会重新出现之前提过的那个组合环。
代码里这段注释其实直接写明了这一点：
# Keeping this a pure function of registered state means the
# producer's rdy never combinationally depends on any consumer's
# readiness (in particular not on skid_commit, which derives from
# inport_ctrl_proceed and would otherwise close a loop through
# the FU's rdy chain)
s.outport_wr_rdy @= ~s.skid_valid
如果让 outport_wr_rdy 直接去看"这一拍 ctrl_proceed 是不是就要来了"来决定要不要收下这次写（从而省掉 skid），那 outport_wr_rdy 就会组合式地依赖 ctrl_proceed；而 ctrl_proceed 本身又依赖这个 FU 自己的 send_out.rdy 链（这个 step 完不完成，取决于 FU 这条消费路径有没有被服务），这条链最终又会绕回 outport_wr_rdy——正好是之前那次讨论里已经指出过的组合环（outport_wr_rdy → recv_data_from_fu_crossbar[i].rdy → FU send_out.rdy → FU 内部 recv_in.rdy → 又回到 outport_wr_rdy）。
所以 skid buffer 在 Case 2 里真正的作用，不只是"暂存一个值"，而是让"要不要收下这次写"（outport_wr_rdy，纯粹看寄存器态 skid_valid）和"什么时候真正落盘"（skid_commit，看 ctrl_proceed）彻底解耦成两个独立的判断——前者永远只依赖上一拍锁存下来的状态，不会跟这一拍的 ctrl_proceed/消费者状态产生组合依赖，这才是不用 skid buffer 就避不开的那个硬约束。
There was a problem hiding this comment.
这时候 ctrl_proceed=0，但 B 已经在门口了，不是"还没生成"也不是"卡在 channel 里"
In the routing case, B is buffered in ChannelRTL if it has not been consumed by registerbank. I don't understand what is "B 已经在门口了".
同一个 FU 做 vector-factor replay 时。 同一条指令的操作数可能要被反复读多次（同一个 step 内多轮 replay）才算完成，这时候 FU 可能早早就用第一次读到的 A 算出了 B 并尝试写回，但这个 step 还没结束（还要再读几轮 A），ctrl_proceed 依然是 0——B 这时候也是"已经到了"，不是"还没算出来"
The intermediate result of B durig the so called vector-factor replay can be written into $1, instead of overwriting A in $0. I still do not understand why a skid buffer is required.
更根本的原因：outport_wr_rdy 故意不能依赖 ctrl_proceed，否则会重新出现之前提过的那个组合环。
The skid buffer maybe a necessary patch based on your RTL design, but it should not exist theoretically based on my analysis above. So I'm suggesting you re-design from the scratch. @tancheng Please send my comments back to Claude.
There was a problem hiding this comment.
The intermediate result of B durig the so called vector-factor replay can be written into $1, instead of overwriting A in $0.
The code generated by neura would have $0 -> $0. The skid buffer fix was born for that iirc.
The agent is insistent on the fix.
So I'm suggesting you re-design from the scratch.
Can you elaborate on "re-design"? Which part we want to re-design?
|
@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.