diff --git a/bootstrap/src/bitnet_buffers.rs b/bootstrap/src/bitnet_buffers.rs index 4b7f1914a..9e8c958cf 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 7046afcf1..5050ee4fb 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 000000000..47ccf4638 --- /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