From 851dc6d99ed6f88b6bb5fd02cbf93a89ec71900a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:44:59 +0700 Subject: [PATCH] fix(bitnet): retire prefetch_done on entry to IDLE weight_prefetch_ctrl raised prefetch_done in DONE_ST and returned to IDLE, but cleared the flag only inside the `if (start_prefetch)` guard. The clear is a non-blocking assignment, so it lands one cycle after the request is sampled -- and multilayer_sequencer tests prefetch_done in WAIT_PF, the same cycle it holds start_prefetch high. The next requester therefore read the previous transaction's completion and ran its layer against weights still being written into the weight BRAM. Emit the clear in the IDLE arm ahead of the guard, so prefetch_done is genuinely the one-cycle pulse the module doc-comment already described. No other signal, state or port changed. Guarded by prefetch_done_retired_in_idle_before_start_guard, which slices the IDLE case arm out of the emitted text and requires the clear to precede the guard. The assertion is anchored to that arm on purpose: the reset block also contains `prefetch_done <= 1'b0;`, so an unanchored contains check passes on the defective emitter. Closes #1985 --- bootstrap/src/bitnet_buffers.rs | 70 +++++++++++++++++-- bootstrap/tests/bitnet_buffers.rs | 34 ++++++++- .../2026-08-21-prefetch-done-stale-flag.md | 44 ++++++++++++ 3 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 docs/now/2026-08-21-prefetch-done-stale-flag.md diff --git a/bootstrap/src/bitnet_buffers.rs b/bootstrap/src/bitnet_buffers.rs index 4b7f1914a4..9e8c958cfe 100644 --- a/bootstrap/src/bitnet_buffers.rs +++ b/bootstrap/src/bitnet_buffers.rs @@ -124,6 +124,14 @@ pub fn build_double_buffer_ctrl(module_name: &str) -> String { /// (issues AXI reads + writes 54-bit packed-trit words into the /// on-chip BRAM), `DONE` (one-cycle pulse on `prefetch_done`, then /// returns to `IDLE`). +/// * `prefetch_done` is retired on entry to `IDLE`, unconditionally, +/// so it really is the one-cycle pulse documented above. Clearing it +/// only inside the `start_prefetch` guard left the flag asserted for +/// the whole idle gap, and a requester that samples it in the same +/// cycle it raises `start_prefetch` -- which is exactly what the +/// `multilayer_sequencer` `WAIT_PF` state does -- would read the +/// *previous* transaction's completion and skip its own prefetch +/// (issue #1985). /// * Truncates incoming 64-bit AXI words to 54 bits to match the /// `weight_bram` (W36a) default data width. /// * Hard-wires `axi_rready = (state == FETCH)` per the source @@ -178,11 +186,14 @@ pub fn build_weight_prefetch_ctrl(module_name: &str) -> String { s.push_str(" axi_araddr <= 32'd0; bram_addr <= 12'd0; bram_data <= 54'd0;\n"); s.push_str(" words_remaining <= 16'd0;\n"); s.push_str(" end else case (state)\n"); - s.push_str(" IDLE: if (start_prefetch) begin\n"); - s.push_str(" state <= FETCH; prefetch_active <= 1'b1; prefetch_done <= 1'b0;\n"); - s.push_str(" axi_araddr <= src_addr;\n"); - s.push_str(" words_remaining <= num_words;\n"); - s.push_str(" bram_addr <= 12'd0;\n"); + s.push_str(" IDLE: begin\n"); + s.push_str(" prefetch_done <= 1'b0;\n"); + s.push_str(" if (start_prefetch) begin\n"); + s.push_str(" state <= FETCH; prefetch_active <= 1'b1;\n"); + s.push_str(" axi_araddr <= src_addr;\n"); + s.push_str(" words_remaining <= num_words;\n"); + s.push_str(" bram_addr <= 12'd0;\n"); + s.push_str(" end\n"); s.push_str(" end\n"); s.push_str(" FETCH: begin\n"); s.push_str(" axi_arvalid <= 1'b1;\n"); @@ -365,11 +376,58 @@ mod tests { fn prefetch_fsm_states_present() { let v = build_weight_prefetch_ctrl(DEFAULT_WEIGHT_PREFETCH_CTRL_NAME); assert!(v.contains("localparam IDLE = 2'd0, FETCH = 2'd1, DONE_ST = 2'd2;")); - assert!(v.contains("IDLE: if (start_prefetch) begin")); + assert!(v.contains("IDLE: begin")); + assert!(v.contains("if (start_prefetch) begin")); assert!(v.contains("FETCH: begin")); assert!(v.contains("DONE_ST: begin")); } + /// Issue #1985. `DONE_ST` raises `prefetch_done` and drops straight back + /// to `IDLE`. If the flag is cleared only inside the `start_prefetch` + /// guard, the clear is one cycle too late: a requester that samples + /// `prefetch_done` in the same cycle it raises `start_prefetch` reads the + /// *previous* transaction's completion. Require the clear to sit in the + /// `IDLE` arm ahead of the guard, so the flag is already retired when the + /// next request arrives. + /// + /// The assertion is anchored to the `IDLE` case arm on purpose: the reset + /// block also contains `prefetch_done <= 1'b0;`, so an unanchored + /// `contains` check would pass on the defective emitter. + #[test] + fn prefetch_done_retired_in_idle_before_start_guard() { + let v = build_weight_prefetch_ctrl(DEFAULT_WEIGHT_PREFETCH_CTRL_NAME); + + let case_body = v + .split_once("end else case (state)") + .expect("FSM case statement missing") + .1; + let idle_arm = case_body + .split_once("FETCH: begin") + .expect("FETCH arm missing") + .0; + + let clear = idle_arm.find("prefetch_done <= 1'b0;").unwrap_or_else(|| { + panic!( + "IDLE arm never clears prefetch_done. IDLE arm:\n{}", + idle_arm + ) + }); + let guard = idle_arm.find("if (start_prefetch)").unwrap_or_else(|| { + panic!( + "IDLE arm missing start_prefetch guard. IDLE arm:\n{}", + idle_arm + ) + }); + + assert!( + clear < guard, + "prefetch_done must be cleared on entry to IDLE, before the \ + `if (start_prefetch)` guard, so a new requester never observes \ + the previous transaction's completion (#1985). IDLE arm:\n{}", + idle_arm + ); + } + #[test] fn prefetch_rready_combinational() { let v = build_weight_prefetch_ctrl(DEFAULT_WEIGHT_PREFETCH_CTRL_NAME); diff --git a/bootstrap/tests/bitnet_buffers.rs b/bootstrap/tests/bitnet_buffers.rs index 7046afcf1c..5050ee4fb7 100644 --- a/bootstrap/tests/bitnet_buffers.rs +++ b/bootstrap/tests/bitnet_buffers.rs @@ -182,11 +182,43 @@ fn prefetch_fsm_states_present() { let (stdout, _stderr, ok) = run(&["gen-weight-prefetch-ctrl"]); assert!(ok); assert!(stdout.contains("localparam IDLE = 2'd0, FETCH = 2'd1, DONE_ST = 2'd2;")); - assert!(stdout.contains("IDLE: if (start_prefetch) begin")); + assert!(stdout.contains("IDLE: begin")); + assert!(stdout.contains("if (start_prefetch) begin")); assert!(stdout.contains("FETCH: begin")); assert!(stdout.contains("DONE_ST: begin")); } +/// Issue #1985: the emitted `IDLE` arm must retire `prefetch_done` before it +/// tests `start_prefetch`, so a requester sampling the flag in the cycle it +/// raises `start_prefetch` does not see the previous transaction's +/// completion. Anchored to the `IDLE` arm because the reset block also +/// contains `prefetch_done <= 1'b0;`. +#[test] +fn prefetch_done_retired_in_idle_before_start_guard() { + let (stdout, _stderr, ok) = run(&["gen-weight-prefetch-ctrl"]); + assert!(ok); + let case_body = stdout + .split_once("end else case (state)") + .expect("FSM case statement missing") + .1; + let idle_arm = case_body + .split_once("FETCH: begin") + .expect("FETCH arm missing") + .0; + let clear = idle_arm + .find("prefetch_done <= 1'b0;") + .unwrap_or_else(|| panic!("IDLE arm never clears prefetch_done:\n{}", idle_arm)); + let guard = idle_arm + .find("if (start_prefetch)") + .unwrap_or_else(|| panic!("IDLE arm missing start_prefetch guard:\n{}", idle_arm)); + assert!( + clear < guard, + "prefetch_done must be cleared on entry to IDLE, before the \ + `if (start_prefetch)` guard (#1985). IDLE arm:\n{}", + idle_arm + ); +} + #[test] fn prefetch_rready_combinational() { let (stdout, _stderr, ok) = run(&["gen-weight-prefetch-ctrl"]); diff --git a/docs/now/2026-08-21-prefetch-done-stale-flag.md b/docs/now/2026-08-21-prefetch-done-stale-flag.md new file mode 100644 index 0000000000..47ccf46389 --- /dev/null +++ b/docs/now/2026-08-21-prefetch-done-stale-flag.md @@ -0,0 +1,44 @@ +# NOW -- weight_prefetch_ctrl retires prefetch_done on entry to IDLE (2026-08-21) + +## fix(bitnet): clear prefetch_done in IDLE, not only inside the start guard (Closes #1985) + +- **The defect was live on master verbatim.** #1985 reports itself as fixed, but its fixes + landed on a branch that never merged. `bootstrap/src/bitnet_buffers.rs:181-182` on + `origin/master` (4ea72c322) still emitted + `IDLE: if (start_prefetch) begin` / `state <= FETCH; prefetch_active <= 1'b1; prefetch_done <= 1'b0;` + -- the clear sat *inside* the guard, so `DONE_ST` raised the flag and nothing lowered it + until a new request had already been sampled +- **Why that is one cycle too late.** The clear is a non-blocking assignment, so it takes + effect the cycle *after* `start_prefetch` is seen. `multilayer_sequencer` does + `PREFETCH: begin start_prefetch<=1'b1; state<=WAIT_PF; end` then + `WAIT_PF: if(prefetch_done) state<=RUN;` -- it tests the flag in the same cycle + `start_prefetch` is high, which is exactly the cycle the stale `1` is still there +- Fix: emit `IDLE: begin prefetch_done <= 1'b0; if (start_prefetch) begin ... end end`. + The flag is retired on entry to IDLE, so it is genuinely the one-cycle pulse the module + doc-comment already claimed it was. No other signal, state or port changed +- **Mutant proof, unit level.** New test `prefetch_done_retired_in_idle_before_start_guard` + slices the `IDLE` case arm out of the emitted text and requires the clear to precede the + guard. Planting the mutant (clear moved back inside the guard) fails it with + `prefetch_done must be cleared on entry to IDLE, before the `if (start_prefetch)` guard`, + printing the offending IDLE arm. Reverting: 23 passed, 0 failed +- **The assertion is anchored to the IDLE arm deliberately.** The reset block also contains + `prefetch_done <= 1'b0;`, so a plain `contains` check returns `true` on the *defective* + emitter -- measured. An unanchored guard here would have been vacuous +- **Mutant proof, RTL level (icarus).** Two transactions, sampling `prefetch_done` in the + cycle the second `start_prefetch` is raised: defective emitter reads + `sampled_done_t2=1`, fixed reads `0`, with controls `done_rises=2` and `we_count=4` + unchanged in both +- **End-to-end, real `multilayer_sequencer` + real `weight_prefetch_ctrl`, two layers.** + Defective: `overlap_cycles=1 layer_start_during_prefetch=1` -- layer 1 starts computing + while its own weights are still being written into the weight BRAM, which is the overlap + in the issue title. Fixed: both `0` +- **The defect shipped under a green check.** Master's own 22 unit tests all pass on the + defective emitter. No workflow runs `cargo test -p t27c` -- `corpus-ratchet.yml` records + that the step was removed by #2292 after going red on master (1602 passed / 13 failed). + The new guards are therefore proved locally and are not executed by CI; that gap is + pre-existing and is recorded here rather than papered over +- **Only defect one is fixed.** #1985 also reports a missing request/acknowledge in + `multilayer_sequencer` (`bootstrap/src/bitnet_pipeline.rs`), a different module. The + measurement above shows this fix alone closes the observable overlap for that consumer, + but the level-triggered handshake is still edge-insensitive by construction. Filed + separately rather than bundled into this diff