From 9adec7490ab416f7bd859bbd388812eadb514988 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 09:02:59 +0700 Subject: [PATCH 1/4] test(sim): differential harness for the zero-count layer_sequencer hang (Refs #1977) `layer_sequencer` never left RUN when asked for zero work: both terminators are `index == count-1` against an unsigned port, and the bare literal 1 widens each subtraction to 32 bits, so a zero count borrows to 32'hFFFFFFFF while the index zero-extends. Elaborates the PR #2337 pre-fix and post-fix renderings in one simulation. Reproduces the published numbers: 200,000 cycles with no `done` and `neuron_id` reaching exactly 50,000, plus six non-zero controls that are identical on every output every cycle. Reporting, not a gate: `vvp` exits 0 on FAIL as well as PASS. Refs #2348 --- sim/tb_bitnet_sequencer_zero_count.v | 419 +++++++++++++++++++++++++++ 1 file changed, 419 insertions(+) create mode 100644 sim/tb_bitnet_sequencer_zero_count.v diff --git a/sim/tb_bitnet_sequencer_zero_count.v b/sim/tb_bitnet_sequencer_zero_count.v new file mode 100644 index 000000000..ebdf172ef --- /dev/null +++ b/sim/tb_bitnet_sequencer_zero_count.v @@ -0,0 +1,419 @@ +`timescale 1ns/1ps +// =========================================================================== +// tb_bitnet_sequencer_zero_count -- differential harness for issue #1977 +// =========================================================================== +// `layer_sequencer` never left the RUN state when it was asked for zero work. +// Both of the FSM's terminators are `index == count-1` compares against an +// unsigned input port: +// +// last_chunk <= (chunk_id == num_chunks-1); +// if(chunk_id==num_chunks-1) begin chunk_id<=0; +// if(neuron_id==num_neurons-1) state<=DONE_ST; else neuron_id<=neuron_id+1; +// end else chunk_id<=chunk_id+1; +// +// `num_neurons` is a 16-bit port and `num_chunks` an 8-bit one, but the bare +// literal `1` makes each subtraction 32 bits wide. A zero count therefore +// BORROWS rather than saturating: `0 - 1` is 32'hFFFFFFFF, while the index on +// the left zero-extends into the same 32 bits. No value a 16-bit `neuron_id` +// or an 8-bit `chunk_id` can hold ever equals 32'hFFFFFFFF, so the compare can +// never fire, the FSM never reaches DONE_ST, `done` never pulses, and `valid` +// is asserted forever for work nobody requested. +// +// The fix retires a zero count straight to DONE_ST with `valid` low: +// +// if(num_neurons==0 || num_chunks==0) begin valid<=0; state<=DONE_ST; end +// else begin ...original body... end +// +// This harness elaborates BOTH the pre-fix and the post-fix emitter output in +// one simulation, drives them from identical stimulus, and compares them. The +// two variants differ only in the `module_name` passed to the emitter, so the +// comparison is between two renderings of the same design, not two designs. +// +// Build (see sim/README.md for the emit step): +// iverilog -g2005 -o tb.vvp sim/tb_bitnet_sequencer_zero_count.v \ +// seq_old.v seq_new.v +// vvp tb.vvp +// +// The measured property is "a request for zero work terminates, and a request +// for non-zero work is unchanged". The second half is not decoration: a guard +// that retired EVERY request to DONE_ST would satisfy the first half alone. +// The six non-zero controls below are what stop that, and they compare the +// full output vector on EVERY cycle rather than just the final state -- a +// controller that reaches the same endpoint by a different path is a +// behavioural change and must be caught. +// =========================================================================== + +module tb_bitnet_sequencer_zero_count; + + // Long enough that "still running" is not a matter of opinion. The pre-fix + // rendering is asked for zero neurons and is still counting at cycle + // 200,000; with num_chunks=4 that is exactly 50,000 spurious neurons. + localparam integer HANG_CYCLES = 200000; + + reg clk = 0, rst_n = 0; + always #5 clk = ~clk; + + integer errors = 0; + + reg d_start = 0; + reg [15:0] d_neurons = 0; + reg [7:0] d_chunks = 0; + + wire [15:0] o_nid, n_nid; + wire [7:0] o_cid, n_cid; + wire o_first, n_first, o_last, n_last, o_valid, n_valid, o_done, n_done; + + seq_old u_old ( + .clk(clk), .rst_n(rst_n), .start(d_start), + .num_neurons(d_neurons), .num_chunks(d_chunks), + .neuron_id(o_nid), .chunk_id(o_cid), + .first_chunk(o_first), .last_chunk(o_last), + .valid(o_valid), .done(o_done) + ); + + seq_new u_new ( + .clk(clk), .rst_n(rst_n), .start(d_start), + .num_neurons(d_neurons), .num_chunks(d_chunks), + .neuron_id(n_nid), .chunk_id(n_cid), + .first_chunk(n_first), .last_chunk(n_last), + .valid(n_valid), .done(n_done) + ); + + // ----------------------------------------------------------------------- + // Observers. + // + // Sampled on the NEGEDGE, deliberately. Every output of this FSM is a + // registered (non-blocking) output, so an `always @(posedge clk)` observer + // reads the value from BEFORE that edge's update and its high-water mark + // trails the design by exactly one cycle. Sampling at the negedge reads the + // settled value of the cycle the design is actually in. The first draft of + // this harness sampled at the posedge and reported max_nid=49999 for a run + // whose port plainly read 50000 -- a phase error in the instrument, not a + // property of either rendering. + // + // `done` is a single-cycle pulse, so it must be latched: sampling it once at + // the end of a case would miss it entirely and report every rendering as + // hung. `valid_seen` is latched for the same reason and is what makes the + // zero-work claim about `valid` meaningful. + // + // The high-water marks copy the port into an integer BEFORE comparing: a + // direct `o_nid > max` promotes the whole expression to unsigned, which is + // harmless while max >= 0 but is the trap that silently froze the + // high-water mark in a sibling harness. Keep it signed by construction + // rather than by luck. + // + // No separate clear signal: `hard_reset` holds `rst_n` low across several + // negedges, which is what zeroes these. + // ----------------------------------------------------------------------- + reg o_done_seen = 0, n_done_seen = 0; + reg o_valid_seen = 0, n_valid_seen = 0; + integer o_max_nid = 0, n_max_nid = 0; + integer o_max_cid = 0, n_max_cid = 0; + integer o_done_pulses = 0, n_done_pulses = 0; + integer o_a, n_a; + + always @(negedge clk) if (!rst_n) begin + o_done_seen = 0; n_done_seen = 0; + o_valid_seen = 0; n_valid_seen = 0; + o_max_nid = 0; n_max_nid = 0; + o_max_cid = 0; n_max_cid = 0; + o_done_pulses = 0; n_done_pulses = 0; + end else begin + if (o_done) begin o_done_seen = 1; o_done_pulses = o_done_pulses + 1; end + if (n_done) begin n_done_seen = 1; n_done_pulses = n_done_pulses + 1; end + if (o_valid) o_valid_seen = 1; + if (n_valid) n_valid_seen = 1; + o_a = o_nid; if (o_a > o_max_nid) o_max_nid = o_a; + n_a = n_nid; if (n_a > n_max_nid) n_max_nid = n_a; + o_a = o_cid; if (o_a > o_max_cid) o_max_cid = o_a; + n_a = n_cid; if (n_a > n_max_cid) n_max_cid = n_a; + end + + // ----------------------------------------------------------------------- + // Cycle-by-cycle equality comparator, used by the non-zero controls. + // + // Every observable output is compared on every cycle while `cmp_en` is high, + // not merely the final state. Two renderings that arrive at the same place + // by different routes are NOT the same rendering, and a control that only + // checked the endpoint would let that through. + // + // `first_chunk` and `last_chunk` are compared only while `valid` is high. + // That is not a convenience: NEITHER rendering resets them. The emitted + // reset block is + // + // state<=IDLE; neuron_id<=0; chunk_id<=0; valid<=0; done<=0; + // + // with no mention of `first_chunk`/`last_chunk`, so both sit at X from + // power-up until the first RUN cycle assigns them, and X !== X. That is a + // property of the design, identical in old and new, and unrelated to the + // zero-count guard under test. Qualifying on `valid` -- the strobe these two + // flags accompany -- compares them exactly when they carry meaning. + // + // `qual_cycles` counts how often that qualified comparison actually ran, and + // the caller asserts it equals the number of work cycles. Without that + // counter the qualification could silently disable the check. + // ----------------------------------------------------------------------- + reg cmp_en = 0; + integer cmp_cycles = 0, cmp_mismatch = 0, qual_cycles = 0; + integer first_bad_cycle = -1; + reg vec_bad, qual_bad; + + always @(negedge clk) if (rst_n && cmp_en) begin + cmp_cycles = cmp_cycles + 1; + vec_bad = (o_nid !== n_nid ) || (o_cid !== n_cid ) || + (o_valid !== n_valid) || (o_done !== n_done ); + qual_bad = 1'b0; + if (o_valid || n_valid) begin + qual_cycles = qual_cycles + 1; + qual_bad = (o_first !== n_first) || (o_last !== n_last); + end + if (vec_bad || qual_bad) begin + if (cmp_mismatch == 0) begin + first_bad_cycle = cmp_cycles; + $display(" FAIL control diverged at compared cycle %0d:", cmp_cycles); + $display(" old nid=%0d cid=%0d first=%b last=%b valid=%b done=%b", + o_nid, o_cid, o_first, o_last, o_valid, o_done); + $display(" new nid=%0d cid=%0d first=%b last=%b valid=%b done=%b", + n_nid, n_cid, n_first, n_last, n_valid, n_done); + end + cmp_mismatch = cmp_mismatch + 1; + end + end + + task expect_eq(input [511:0] what, input integer got, input integer want); + begin + if (got !== want) begin + errors = errors + 1; + $display(" FAIL %0s: got %0d, want %0d", what, got, want); + end + end + endtask + + // All stimulus changes on the NEGEDGE, so `start` is stable across exactly + // one posedge no matter what phase the caller is in. Driving it from the + // posedge instead makes the pulse race the DUT's own sampling: the stimulus + // can clear `start` at the same timestep the DUT reads it, and the run + // silently never begins. + task pulse_start; + begin + @(negedge clk); d_start = 1; + @(negedge clk); d_start = 0; + end + endtask + + // `cmp_en` is dropped here so the comparator never sees the reset window. + // `rst_n` is held low across several negedges, which is what clears the + // negedge-sampled observers above. + task hard_reset; + begin + @(posedge clk); cmp_en = 0; + @(negedge clk); rst_n = 0; + repeat (4) @(posedge clk); + @(negedge clk); rst_n = 1; + @(posedge clk); + end + endtask + + // ----------------------------------------------------------------------- + // One non-zero control: identical stimulus, full-vector comparison, and a + // liveness requirement so that "identical" cannot mean "both did nothing". + // ----------------------------------------------------------------------- + integer ctl_idx = 0; + task run_control(input [15:0] neurons, input [7:0] chunks); + integer budget; + begin + ctl_idx = ctl_idx + 1; + hard_reset; // leaves us on a posedge, opposite phase to the comparator + cmp_cycles = 0; cmp_mismatch = 0; qual_cycles = 0; first_bad_cycle = -1; + d_neurons = neurons; + d_chunks = chunks; + cmp_en = 1; + pulse_start; + // neurons*chunks RUN cycles, plus DONE_ST, plus slack. + budget = neurons * chunks + 32; + repeat (budget) @(posedge clk); + // Settle on the negedge the comparator uses, then step past it before + // reading its counters, so the read cannot race the final comparison. + @(negedge clk); #1; + cmp_en = 0; + + $display(" control %0d: num_neurons=%0d num_chunks=%0d -> compared %0d cycles (%0d qualified), %0d mismatches; old(done=%0d valid_seen=%b) new(done=%0d valid_seen=%b)", + ctl_idx, neurons, chunks, cmp_cycles, qual_cycles, cmp_mismatch, + o_done_pulses, o_valid_seen, n_done_pulses, n_valid_seen); + + if (cmp_mismatch !== 0) begin + errors = errors + 1; + $display(" FAIL control %0d: %0d cycle(s) differ, first at %0d", + ctl_idx, cmp_mismatch, first_bad_cycle); + end + + // Anti-vacuity: an "identical" verdict from two renderings that never + // ran is worth nothing. Both must have completed real work. + if (!o_valid_seen || !n_valid_seen) begin + errors = errors + 1; + $display(" FAIL control %0d: valid never asserted (old=%b new=%b) -- the control did no work and proves nothing", + ctl_idx, o_valid_seen, n_valid_seen); + end + if (o_done_pulses !== 1 || n_done_pulses !== 1) begin + errors = errors + 1; + $display(" FAIL control %0d: expected exactly one done pulse each, got old=%0d new=%0d", + ctl_idx, o_done_pulses, n_done_pulses); + end + if (cmp_cycles < neurons * chunks) begin + errors = errors + 1; + $display(" FAIL control %0d: only %0d cycles compared, fewer than the %0d work cycles", + ctl_idx, cmp_cycles, neurons * chunks); + end + // The `first_chunk`/`last_chunk` comparison is qualified on `valid`. + // Pin how often it actually ran, so the qualification cannot quietly + // reduce that half of the vector to a no-op: exactly one qualified + // cycle per (neuron, chunk) pair. + if (qual_cycles !== neurons * chunks) begin + errors = errors + 1; + $display(" FAIL control %0d: first/last compared on %0d cycles, want %0d -- the valid-qualified half of the vector is not being exercised", + ctl_idx, qual_cycles, neurons * chunks); + end + end + endtask + + // ----------------------------------------------------------------------- + // Stimulus + // ----------------------------------------------------------------------- + initial begin + hard_reset; + + // ------------------------------------------------------------------ + // CASE 1 -- the reported defect: zero neurons, non-zero chunks. + // + // num_chunks is deliberately non-zero so the chunk terminator still + // fires normally. That isolates the neuron compare: the only reason the + // FSM cannot finish is `neuron_id == num_neurons-1` against 32'hFFFFFFFF. + // ------------------------------------------------------------------ + d_neurons = 16'd0; + d_chunks = 8'd4; + pulse_start; + repeat (HANG_CYCLES) @(posedge clk); + @(negedge clk); #1; // step past the observer's own sampling edge + + $display("== CASE 1: zero neurons (num_neurons=0, num_chunks=4), %0d cycles ==", + HANG_CYCLES); + $display(" old: done_seen=%b done_pulses=%0d valid_seen=%b max_nid=%0d final_nid=%0d", + o_done_seen, o_done_pulses, o_valid_seen, o_max_nid, o_nid); + $display(" new: done_seen=%b done_pulses=%0d valid_seen=%b max_nid=%0d final_nid=%0d", + n_done_seen, n_done_pulses, n_valid_seen, n_max_nid, n_nid); + + // The defect must still be present in the OLD rendering, or this harness + // is not measuring what it claims to measure. + if (o_done_seen !== 1'b0) begin + errors = errors + 1; + $display(" FAIL harness: old reported done for a zero-neuron request -- " + , "the non-termination this harness exists to demonstrate is not " + , "reproducing"); + end + // Not merely "no done": still actively counting. A rendering that had + // wedged with all outputs frozen would also show done=0, and that is a + // different defect from the one under test. + if (o_max_nid < 1000) begin + errors = errors + 1; + $display(" FAIL harness: old only reached neuron_id=%0d in %0d cycles -- expected it to keep counting, not to freeze", + o_max_nid, HANG_CYCLES); + end + // num_chunks=4 advances neuron_id once every 4 RUN cycles, so after + // HANG_CYCLES posedges in RUN the index is exactly HANG_CYCLES/4 and has + // not yet wrapped its 16 bits (50000 < 65536). Pin the arithmetic: an + // off-by-a-lot here would mean the FSM is not doing what the analysis says. + expect_eq("old neuron_id after the hang", o_max_nid, HANG_CYCLES / 4); + if (o_valid_seen !== 1'b1) begin + errors = errors + 1; + $display(" FAIL harness: old never asserted valid -- expected it to emit " + , "work strobes for a request of zero neurons"); + end + + // The fix: terminate, and do not emit a single work strobe on the way. + if (n_done_seen !== 1'b1) begin + errors = errors + 1; + $display(" FAIL new never reported done for a zero-neuron request"); + end + expect_eq("new neuron_id after the run", n_max_nid, 0); + if (n_valid_seen !== 1'b0) begin + errors = errors + 1; + $display(" FAIL new asserted valid for a request of zero neurons"); + end + + // ------------------------------------------------------------------ + // CASE 2 -- the other zero: zero chunks, non-zero neurons. + // + // The chunk terminator has its own borrow, and a guard that tested only + // num_neurons would leave this half hanging. Separate case, separate + // claim. + // ------------------------------------------------------------------ + hard_reset; + d_neurons = 16'd8; + d_chunks = 8'd0; + pulse_start; + repeat (HANG_CYCLES) @(posedge clk); + @(negedge clk); #1; + + $display("== CASE 2: zero chunks (num_neurons=8, num_chunks=0), %0d cycles ==", + HANG_CYCLES); + $display(" old: done_seen=%b valid_seen=%b max_nid=%0d max_cid=%0d final_cid=%0d", + o_done_seen, o_valid_seen, o_max_nid, o_max_cid, o_cid); + $display(" new: done_seen=%b valid_seen=%b max_nid=%0d max_cid=%0d final_cid=%0d", + n_done_seen, n_valid_seen, n_max_nid, n_max_cid, n_cid); + + if (o_done_seen !== 1'b0) begin + errors = errors + 1; + $display(" FAIL harness: old reported done for a zero-chunk request -- " + , "the zero-chunk half of the defect is not reproducing"); + end + if (o_valid_seen !== 1'b1) begin + errors = errors + 1; + $display(" FAIL harness: old never asserted valid on the zero-chunk path"); + end + // Pin the mechanism, not just the symptom. With num_chunks=0 the CHUNK + // compare is the one that can never match, so chunk_id free-runs and wraps + // its 8 bits while neuron_id never advances at all. Both halves are + // asserted: a rendering that hung with chunk_id frozen, or one that + // advanced neuron_id anyway, would be a different defect. + expect_eq("old chunk_id high-water on the zero-chunk path", o_max_cid, 255); + expect_eq("old neuron_id on the zero-chunk path", o_max_nid, 0); + // HANG_CYCLES posedges in RUN leave chunk_id at HANG_CYCLES mod 256. + expect_eq("old chunk_id after the hang", o_cid, HANG_CYCLES % 256); + if (n_done_seen !== 1'b1) begin + errors = errors + 1; + $display(" FAIL new never reported done for a zero-chunk request"); + end + if (n_valid_seen !== 1'b0) begin + errors = errors + 1; + $display(" FAIL new asserted valid for a request of zero chunks"); + end + + // ------------------------------------------------------------------ + // CASES 3..8 -- six non-zero controls. + // + // These are what stop the guard from being a licence to retire everything: + // a rendering that jumped to DONE_ST unconditionally would pass CASE 1 and + // CASE 2 perfectly. Every observable output is compared on every cycle. + // + // The set spans the shapes where the terminators behave differently: + // num_chunks=1 makes `chunk_id==num_chunks-1` true at chunk_id=0 so the + // chunk loop never iterates; num_neurons=1 exercises the single-neuron + // exit; 8'd255 is the largest value the 8-bit chunk port can carry, which + // is the boundary the widened subtraction would disturb if the guard were + // implemented by narrowing the compare instead. + // ------------------------------------------------------------------ + $display("== CASES 3..8: six non-zero controls, full-vector cycle-by-cycle =="); + run_control(16'd1, 8'd1); + run_control(16'd1, 8'd4); + run_control(16'd4, 8'd1); + run_control(16'd3, 8'd5); + run_control(16'd7, 8'd2); + run_control(16'd2, 8'd255); + + $display(""); + if (errors == 0) $display("RESULT: PASS (0 errors)"); + else $display("RESULT: FAIL (%0d errors)", errors); + $finish; + end + +endmodule From 12de8b08db0fc668e1b47e53a53d4df372ece45d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 09:02:59 +0700 Subject: [PATCH 2/4] test(sim): differential harness for the stale prefetch_done level (Refs #1985) `weight_prefetch_ctrl` documented `prefetch_done` as a one-cycle pulse but cleared it only inside the start guard, leaving it asserted for the whole idle gap. A requester sampling it in the cycle it raises `start_prefetch` reads the previous transaction's completion. Elaborates the PR #2340 pre-fix and post-fix renderings in one simulation. Reproduces the published numbers: t2 sampled_done OLD=1 / NEW=0, done_rises 2/2, we_count 4/4. A second case varies the idle gap to separate level from pulse: old's high-time grows with the gap, new's does not. Reporting, not a gate: `vvp` exits 0 on FAIL as well as PASS. Refs #2348 --- sim/tb_bitnet_prefetch_done_pulse.v | 332 ++++++++++++++++++++++++++++ 1 file changed, 332 insertions(+) create mode 100644 sim/tb_bitnet_prefetch_done_pulse.v diff --git a/sim/tb_bitnet_prefetch_done_pulse.v b/sim/tb_bitnet_prefetch_done_pulse.v new file mode 100644 index 000000000..f86359cf6 --- /dev/null +++ b/sim/tb_bitnet_prefetch_done_pulse.v @@ -0,0 +1,332 @@ +`timescale 1ns/1ps +// =========================================================================== +// tb_bitnet_prefetch_done_pulse -- differential harness for issue #1985 +// =========================================================================== +// `weight_prefetch_ctrl` documents `prefetch_done` as a one-cycle pulse raised +// in DONE_ST. It was not one. The pre-fix emitter cleared the flag only INSIDE +// the start guard: +// +// IDLE: if (start_prefetch) begin +// state <= FETCH; prefetch_active <= 1'b1; prefetch_done <= 1'b0; +// ... +// end +// +// DONE_ST raises `prefetch_done` and drops straight back to IDLE, and nothing +// in IDLE lowers it again until the NEXT request arrives. So the flag is not a +// pulse at all -- it is a level that stays asserted for the whole idle gap, +// however long that gap happens to be. +// +// That matters because of who reads it. A requester that samples +// `prefetch_done` in the same cycle it raises `start_prefetch` -- which is +// exactly what the `multilayer_sequencer` WAIT_PF state does -- reads the +// PREVIOUS transaction's completion and concludes its own prefetch is already +// finished, skipping it. +// +// The fix retires the flag on entry to IDLE, unconditionally, ahead of the +// guard: +// +// IDLE: begin +// prefetch_done <= 1'b0; +// if (start_prefetch) begin ... end +// end +// +// This harness elaborates BOTH the pre-fix and the post-fix emitter output in +// one simulation, drives them from identical stimulus, and compares them. The +// two variants differ only in the `module_name` passed to the emitter, so the +// comparison is between two renderings of the same design, not two designs. +// +// Build (see sim/README.md for the emit step): +// iverilog -g2005 -o tb.vvp sim/tb_bitnet_prefetch_done_pulse.v \ +// pf_old.v pf_new.v +// vvp tb.vvp +// +// The measured property has two halves, and BOTH are asserted: +// +// * `prefetch_done` must be observable as a pulse, not a level -- measured +// by holding the idle gap at two different lengths and checking that the +// number of cycles the flag stays high does not track the gap. +// * NOTHING ELSE may change. Every other output is compared cycle by cycle +// across the whole run and must be identical. Without that half, a +// rendering that fixed the flag by breaking the fetch would pass. +// =========================================================================== + +module tb_bitnet_prefetch_done_pulse; + + localparam integer WORDS = 2; // BRAM writes per transaction + + reg clk = 0, rst_n = 0; + always #5 clk = ~clk; + + integer errors = 0; + + reg d_start = 0; + reg [31:0] d_src = 32'h1000; + reg [15:0] d_words = WORDS; + + wire o_active, n_active, o_done, n_done; + wire [31:0] o_araddr, n_araddr; + wire o_arvalid, n_arvalid, o_rready, n_rready; + wire [11:0] o_baddr, n_baddr; + wire [53:0] o_bdata, n_bdata; + wire o_bwe, n_bwe; + + // AXI read slave: address always accepted, data always available. Beat K + // carries the value K, so a payload comparison is meaningful. + reg [63:0] rbeat = 0; + always @(posedge clk) if (!rst_n) rbeat <= 0; + else if (o_rready) rbeat <= rbeat + 1; + + pf_old u_old ( + .clk(clk), .rst_n(rst_n), .start_prefetch(d_start), + .src_addr(d_src), .num_words(d_words), + .prefetch_active(o_active), .prefetch_done(o_done), + .axi_araddr(o_araddr), .axi_arvalid(o_arvalid), .axi_arready(1'b1), + .axi_rdata(rbeat), .axi_rvalid(1'b1), .axi_rready(o_rready), + .bram_addr(o_baddr), .bram_data(o_bdata), .bram_we(o_bwe) + ); + + pf_new u_new ( + .clk(clk), .rst_n(rst_n), .start_prefetch(d_start), + .src_addr(d_src), .num_words(d_words), + .prefetch_active(n_active), .prefetch_done(n_done), + .axi_araddr(n_araddr), .axi_arvalid(n_arvalid), .axi_arready(1'b1), + .axi_rdata(rbeat), .axi_rvalid(1'b1), .axi_rready(n_rready), + .bram_addr(n_baddr), .bram_data(n_bdata), .bram_we(n_bwe) + ); + + // ----------------------------------------------------------------------- + // Observers, sampled on the NEGEDGE. + // + // Every output of this FSM is registered, so a posedge observer reads the + // value from before that edge's non-blocking update and trails the design by + // a cycle. The negedge reads the settled value of the cycle the design is + // actually in. (A sibling harness in this directory reported a count one + // short for exactly this reason.) + // ----------------------------------------------------------------------- + integer o_we_count = 0, n_we_count = 0; + integer o_done_rises = 0, n_done_rises = 0; + integer o_done_high = 0, n_done_high = 0; + reg o_done_prev = 0, n_done_prev = 0; + + // What a requester sees when it samples the flag in the same cycle it raises + // `start_prefetch`. Indexed by which request it is: t1 is the first (both + // renderings must read 0 -- nothing has completed yet), t2 the second. + // + // These are captured inside `pulse_start`, in the stimulus process itself, + // NOT by the negedge observer below. `d_start` is asserted on the negedge, + // so an observer that also triggers on the negedge races the assignment and + // may sample the cycle before the request. The first draft did exactly that + // and reported t2=0 for both renderings -- an ordering artefact, not a + // property of either. Sampling from the process that drives the signal + // removes the race by construction. + integer start_idx = 0; + reg o_t1 = 1'bx, n_t1 = 1'bx, o_t2 = 1'bx, n_t2 = 1'bx; + + // Comparator over EVERY output except `prefetch_done`. `other_mismatch` must + // stay 0: the fix is meant to be surgical. `done_differs` counts the cycles + // on which the flag itself diverges and must be NON-zero, or the harness is + // observing nothing. + integer other_mismatch = 0, done_differs = 0, cmp_cycles = 0; + + always @(negedge clk) if (!rst_n) begin + o_we_count = 0; n_we_count = 0; + o_done_rises = 0; n_done_rises = 0; + o_done_high = 0; n_done_high = 0; + o_done_prev = 0; n_done_prev = 0; + other_mismatch = 0; done_differs = 0; cmp_cycles = 0; + end else begin + if (o_bwe) o_we_count = o_we_count + 1; + if (n_bwe) n_we_count = n_we_count + 1; + if (o_done && !o_done_prev) o_done_rises = o_done_rises + 1; + if (n_done && !n_done_prev) n_done_rises = n_done_rises + 1; + if (o_done) o_done_high = o_done_high + 1; + if (n_done) n_done_high = n_done_high + 1; + o_done_prev = o_done; + n_done_prev = n_done; + + cmp_cycles = cmp_cycles + 1; + if (o_active !== n_active || o_araddr !== n_araddr || + o_arvalid !== n_arvalid || o_rready !== n_rready || + o_baddr !== n_baddr || o_bdata !== n_bdata || + o_bwe !== n_bwe) begin + if (other_mismatch == 0) + $display(" FAIL a non-prefetch_done output diverged at compared cycle %0d: active %b/%b araddr %0h/%0h arvalid %b/%b rready %b/%b baddr %0d/%0d bdata %0h/%0h bwe %b/%b", + cmp_cycles, o_active, n_active, o_araddr, n_araddr, + o_arvalid, n_arvalid, o_rready, n_rready, + o_baddr, n_baddr, o_bdata, n_bdata, o_bwe, n_bwe); + other_mismatch = other_mismatch + 1; + end + if (o_done !== n_done) done_differs = done_differs + 1; + end + + task expect_eq(input [511:0] what, input integer got, input integer want); + begin + if (got !== want) begin + errors = errors + 1; + $display(" FAIL %0s: got %0d, want %0d", what, got, want); + end + end + endtask + + // Stimulus changes on the NEGEDGE so `start_prefetch` is stable across + // exactly one posedge whatever phase the caller is in. Driven from the + // posedge it races the DUT's own sampling and the transaction can silently + // never begin. + // The flag is sampled here, one delta past the negedge, BEFORE `d_start` is + // raised. That is precisely the requester's view: the value it reads on the + // posedge at which the DUT first sees `start_prefetch` high is the value + // settled from the previous posedge, which is what this negedge holds. + task pulse_start; + begin + @(negedge clk); + #1; + start_idx = start_idx + 1; + if (start_idx == 1) begin o_t1 = o_done; n_t1 = n_done; end + if (start_idx == 2) begin o_t2 = o_done; n_t2 = n_done; end + d_start = 1; + @(negedge clk); d_start = 0; + end + endtask + + task hard_reset; + begin + @(negedge clk); rst_n = 0; + repeat (4) @(posedge clk); + @(negedge clk); rst_n = 1; + @(posedge clk); + end + endtask + + // ----------------------------------------------------------------------- + // Two back-to-back transactions separated by `gap` idle cycles, with the + // requester sampling `prefetch_done` in the same cycle it raises + // `start_prefetch`. + // ----------------------------------------------------------------------- + task two_transactions(input integer gap); + begin + hard_reset; + start_idx = 0; + o_t1 = 1'bx; n_t1 = 1'bx; o_t2 = 1'bx; n_t2 = 1'bx; + pulse_start; + repeat (WORDS + 8) @(posedge clk); // let transaction 1 retire + repeat (gap) @(posedge clk); // idle gap + pulse_start; + repeat (WORDS + 8) @(posedge clk); // let transaction 2 retire + @(negedge clk); #1; // step past the observer's own edge + end + endtask + + // ----------------------------------------------------------------------- + // Stimulus + // ----------------------------------------------------------------------- + initial begin + + // ------------------------------------------------------------------ + // CASE 1 -- the reported defect, with a short idle gap. + // ------------------------------------------------------------------ + two_transactions(8); + + $display("== CASE 1: two %0d-word transactions, 8-cycle idle gap ==", WORDS); + $display(" old: t1 sampled_done=%b t2 sampled_done=%b done_rises=%0d done_high_cycles=%0d we_count=%0d", + o_t1, o_t2, o_done_rises, o_done_high, o_we_count); + $display(" new: t1 sampled_done=%b t2 sampled_done=%b done_rises=%0d done_high_cycles=%0d we_count=%0d", + n_t1, n_t2, n_done_rises, n_done_high, n_we_count); + $display(" non-prefetch_done outputs: %0d mismatch(es) over %0d cycles; prefetch_done differed on %0d cycle(s)", + other_mismatch, cmp_cycles, done_differs); + + // Liveness first. Every claim below is vacuous if the transactions did not + // happen: both renderings must have completed both of them and written the + // same number of BRAM words. + expect_eq("old done rises", o_done_rises, 2); + expect_eq("new done rises", n_done_rises, 2); + expect_eq("old BRAM writes", o_we_count, 2 * WORDS); + expect_eq("new BRAM writes", n_we_count, 2 * WORDS); + + // At the FIRST request nothing has completed yet, so both renderings must + // read the flag low. This is what makes the t2 reading below a difference + // in staleness rather than a constant offset between the two renderings. + if (o_t1 !== 1'b0 || n_t1 !== 1'b0) begin + errors = errors + 1; + $display(" FAIL at the first request the flag should read low in both, got old=%b new=%b", + o_t1, n_t1); + end + + // The defect must still be present in the OLD rendering, or this harness + // is not measuring what it claims to measure. + if (o_t2 !== 1'b1) begin + errors = errors + 1; + $display(" FAIL harness: old read prefetch_done=%b at the second request, expected the stale 1 -- the defect this harness exists to demonstrate is not reproducing", + o_t2); + end + + // The fix: the second requester sees its own state, not the previous + // transaction's completion. + if (n_t2 !== 1'b0) begin + errors = errors + 1; + $display(" FAIL new read prefetch_done=%b at the second request, expected 0", + n_t2); + end + + // The fix must be surgical: the flag is the ONLY thing that may differ. + expect_eq("non-prefetch_done outputs that diverged", other_mismatch, 0); + // ...and it must actually differ, or the comparison above is measuring an + // absence of change that includes the change under test. + if (done_differs == 0) begin + errors = errors + 1; + $display(" FAIL prefetch_done never differed between the renderings -- the harness observed nothing"); + end + + // ------------------------------------------------------------------ + // CASE 2 -- pulse or level? Same stimulus, a much longer idle gap. + // + // This is the case that distinguishes the two possible readings of CASE 1. + // A one-cycle pulse holds its high-time constant when the gap grows; a + // level stretches with it. The old rendering's high-time must track the + // gap and the new one's must not. + // ------------------------------------------------------------------ + begin : case2 + integer o_high_short, n_high_short; + o_high_short = o_done_high; + n_high_short = n_done_high; + + two_transactions(40); + + $display("== CASE 2: same, 40-cycle idle gap (was 8) =="); + $display(" old: done_high_cycles=%0d (8-cycle gap gave %0d) done_rises=%0d we_count=%0d", + o_done_high, o_high_short, o_done_rises, o_we_count); + $display(" new: done_high_cycles=%0d (8-cycle gap gave %0d) done_rises=%0d we_count=%0d", + n_done_high, n_high_short, n_done_rises, n_we_count); + $display(" non-prefetch_done outputs: %0d mismatch(es) over %0d cycles; prefetch_done differed on %0d cycle(s)", + other_mismatch, cmp_cycles, done_differs); + + expect_eq("old done rises (long gap)", o_done_rises, 2); + expect_eq("new done rises (long gap)", n_done_rises, 2); + expect_eq("old BRAM writes (long gap)", o_we_count, 2 * WORDS); + expect_eq("new BRAM writes (long gap)", n_we_count, 2 * WORDS); + + // The old flag is a level: 32 more idle cycles, 32 more cycles high. + expect_eq("old prefetch_done high-time grew by the extra gap", + o_done_high - o_high_short, 32); + + // The new flag is a pulse: one cycle per completion, gap-independent. + expect_eq("new prefetch_done high-time is gap-independent", + n_done_high - n_high_short, 0); + expect_eq("new prefetch_done high-time is one cycle per completion", + n_done_high, 2); + + expect_eq("non-prefetch_done outputs that diverged (long gap)", + other_mismatch, 0); + if (o_t2 !== 1'b1 || n_t2 !== 1'b0) begin + errors = errors + 1; + $display(" FAIL long-gap sampled_done: old=%b new=%b, expected old=1 new=0", + o_t2, n_t2); + end + end + + $display(""); + if (errors == 0) $display("RESULT: PASS (0 errors)"); + else $display("RESULT: FAIL (%0d errors)", errors); + $finish; + end + +endmodule From 610f916fa76ccbfcd3bc80f08ee07cd2caf92cd0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 09:03:17 +0700 Subject: [PATCH 3/4] test(sim): three-way harness for the latent local_we default (Refs #2006) #2006 defaults `local_we` low ahead of the case. The pre-fix and post-fix renderings are observationally IDENTICAL: every reachable path already drove the strobe, and the states that never mention it are never entered with it high, because READ_DATA's only exit is DONE_ST. A harness that passes by finding no difference proves nothing, so this one is three-way: A = pre-#2006 (PR #2344 base), B = #2006 (PR #2344 head), C = #2006 + #2003 (PR #2345 head). B and C are consecutive revisions -- #2344's head rendering is byte-identical to #2345's base -- so one comparator sees all three. A vs B must be identical; B vs C must differ. Same comparator, same stimulus, same run, same 293-bit vector of every output port. Putting B in the C slot makes the run fail rather than certify its null result. Measured: A vs B 0 mismatching cycles, B vs C 266, over 373 cycles and seven phases. Deleting READ_DATA's `end else local_we <= 1'b0;` from both renderings makes A emit 22 local writes against B's 18 -- latent today, a real backstop the moment an arm stops clearing the strobe. Reporting, not a gate: `vvp` exits 0 on FAIL as well as PASS. Refs #2348 --- sim/tb_bitnet_dma_we_default.v | 398 +++++++++++++++++++++++++++++++++ 1 file changed, 398 insertions(+) create mode 100644 sim/tb_bitnet_dma_we_default.v diff --git a/sim/tb_bitnet_dma_we_default.v b/sim/tb_bitnet_dma_we_default.v new file mode 100644 index 000000000..6e8434433 --- /dev/null +++ b/sim/tb_bitnet_dma_we_default.v @@ -0,0 +1,398 @@ +`timescale 1ns/1ps +// =========================================================================== +// tb_bitnet_dma_we_default -- THREE-WAY differential harness for issue #2006 +// =========================================================================== +// `local_we` is a one-cycle write strobe, but the pre-fix `dma_controller` +// drove it only from the arms that happened to think about it. The fix +// defaults it low ahead of the case, so a state that never mentions it leaves +// it low: +// +// end else begin +// local_we <= 1'b0; // <-- added +// case (state) +// ... +// +// THE CLAIM THIS HARNESS MAKES IS A NULL RESULT, AND THAT NEEDS SAYING OUT +// LOUD: the pre-fix and post-fix renderings are observationally IDENTICAL. +// The fix is behaviourally latent. Reading the pre-fix emitter output, every +// reachable path already drives the strobe -- +// +// reset : local_we <= 1'b0; +// READ_DATA : local_we <= 1'b1; ... end else local_we <= 1'b0; +// DONE_ST : local_we <= 1'b0; +// +// -- and the states that do NOT drive it (IDLE, READ_ADDR, WRITE_ADDR, +// WRITE_DATA, default) are never entered with it high, because READ_DATA's +// only exit is DONE_ST and DONE_ST clears the strobe before returning to IDLE. +// The default-low is defence in depth against a future arm, not a repair of an +// observable defect. +// +// A harness that "passes" by finding no difference proves nothing on its own: +// a harness wired to the wrong ports, or one whose comparator never ran, finds +// no difference either. So this harness is THREE-WAY. It elaborates three +// renderings of the same design and runs one comparator over two pairs: +// +// A = pre-#2006 (PR #2344 base) +// B = #2006 applied (PR #2344 head) +// C = #2006 + #2003 write-address fix (PR #2345 head) +// +// B and C are consecutive revisions of the same file: PR #2344's head +// rendering is byte-identical to PR #2345's base rendering, so A, B and C are +// a linear chain and the same comparator sees all three. +// +// A vs B must be IDENTICAL -- the null result under test +// B vs C must DIFFER -- the anti-vacuity control +// +// The control is not decoration and it is not a separate test. It is the SAME +// comparator, on the SAME stimulus, in the SAME simulation, over the same +// signal set. If the comparator is blind -- misconnected ports, a disabled +// enable, a vector that omits `local_we`, a stimulus that never starts a +// transfer -- then B vs C reports "identical" too, and the run FAILS. Only +// when the instrument has demonstrated on C that it can see a difference does +// "A equals B" carry any information. +// +// Build (see sim/README.md for the emit step): +// iverilog -g2005 -o tb.vvp sim/tb_bitnet_dma_we_default.v \ +// dma_a.v dma_b.v dma_c.v +// vvp tb.vvp +// =========================================================================== + +module tb_bitnet_dma_we_default; + + localparam integer CAP_WORDS = 4096; + + reg clk = 0, rst_n = 0; + always #5 clk = ~clk; + + integer errors = 0; + + reg d_start = 0; + reg d_dir = 0; + reg [31:0] d_length = 0; + reg d_arready = 1; // held low to stretch READ_ADDR + reg d_rvalid = 0; + reg d_rlast = 0; + reg d_wready = 1; + + // ------------------------------------------------------------------------ + // Three renderings, identical stimulus. + // ------------------------------------------------------------------------ + wire a_busy, b_busy, c_busy, a_done, b_done, c_done; + wire [63:0] a_araddr, b_araddr, c_araddr; + wire [7:0] a_arlen, b_arlen, c_arlen; + wire a_arvalid, b_arvalid, c_arvalid; + wire a_rready, b_rready, c_rready; + wire [63:0] a_awaddr, b_awaddr, c_awaddr; + wire [7:0] a_awlen, b_awlen, c_awlen; + wire a_awvalid, b_awvalid, c_awvalid; + wire [63:0] a_wdata_axi, b_wdata_axi, c_wdata_axi; + wire a_wlast, b_wlast, c_wlast, a_wvalid, b_wvalid, c_wvalid; + wire a_bready, b_bready, c_bready; + wire [11:0] a_addr, b_addr, c_addr; + wire [63:0] a_wdata, b_wdata, c_wdata; + wire a_we, b_we, c_we; + + // Each DUT gets its own beat counter driven by its own `rready`, so the + // payload a rendering captures is its own beat index and the three never + // share a counter. Beat K carries the value K. + reg beats_clr = 0; + reg [63:0] a_beats = 0, b_beats = 0, c_beats = 0; + always @(posedge clk) if (!rst_n || beats_clr) a_beats <= 0; else if (a_rready && d_rvalid) a_beats <= a_beats + 1; + always @(posedge clk) if (!rst_n || beats_clr) b_beats <= 0; else if (b_rready && d_rvalid) b_beats <= b_beats + 1; + always @(posedge clk) if (!rst_n || beats_clr) c_beats <= 0; else if (c_rready && d_rvalid) c_beats <= c_beats + 1; + + dma_a u_a ( + .clk(clk), .rst_n(rst_n), + .start(d_start), .src_addr(64'h2000), .dst_addr(64'h3000), + .length(d_length), .direction(d_dir), .busy(a_busy), .done(a_done), + .m_axi_araddr(a_araddr), .m_axi_arlen(a_arlen), .m_axi_arvalid(a_arvalid), + .m_axi_arready(d_arready), + .m_axi_rdata(a_beats), .m_axi_rlast(d_rlast), .m_axi_rvalid(d_rvalid), + .m_axi_rready(a_rready), + .m_axi_awaddr(a_awaddr), .m_axi_awlen(a_awlen), .m_axi_awvalid(a_awvalid), + .m_axi_awready(1'b1), + .m_axi_wdata(a_wdata_axi), .m_axi_wlast(a_wlast), .m_axi_wvalid(a_wvalid), + .m_axi_wready(d_wready), .m_axi_bvalid(1'b1), .m_axi_bready(a_bready), + .local_addr(a_addr), .local_wdata(a_wdata), .local_we(a_we), + .local_rdata(64'hA5A5_0000_0000_5A5A) + ); + + dma_b u_b ( + .clk(clk), .rst_n(rst_n), + .start(d_start), .src_addr(64'h2000), .dst_addr(64'h3000), + .length(d_length), .direction(d_dir), .busy(b_busy), .done(b_done), + .m_axi_araddr(b_araddr), .m_axi_arlen(b_arlen), .m_axi_arvalid(b_arvalid), + .m_axi_arready(d_arready), + .m_axi_rdata(b_beats), .m_axi_rlast(d_rlast), .m_axi_rvalid(d_rvalid), + .m_axi_rready(b_rready), + .m_axi_awaddr(b_awaddr), .m_axi_awlen(b_awlen), .m_axi_awvalid(b_awvalid), + .m_axi_awready(1'b1), + .m_axi_wdata(b_wdata_axi), .m_axi_wlast(b_wlast), .m_axi_wvalid(b_wvalid), + .m_axi_wready(d_wready), .m_axi_bvalid(1'b1), .m_axi_bready(b_bready), + .local_addr(b_addr), .local_wdata(b_wdata), .local_we(b_we), + .local_rdata(64'hA5A5_0000_0000_5A5A) + ); + + dma_c u_c ( + .clk(clk), .rst_n(rst_n), + .start(d_start), .src_addr(64'h2000), .dst_addr(64'h3000), + .length(d_length), .direction(d_dir), .busy(c_busy), .done(c_done), + .m_axi_araddr(c_araddr), .m_axi_arlen(c_arlen), .m_axi_arvalid(c_arvalid), + .m_axi_arready(d_arready), + .m_axi_rdata(c_beats), .m_axi_rlast(d_rlast), .m_axi_rvalid(d_rvalid), + .m_axi_rready(c_rready), + .m_axi_awaddr(c_awaddr), .m_axi_awlen(c_awlen), .m_axi_awvalid(c_awvalid), + .m_axi_awready(1'b1), + .m_axi_wdata(c_wdata_axi), .m_axi_wlast(c_wlast), .m_axi_wvalid(c_wvalid), + .m_axi_wready(d_wready), .m_axi_bvalid(1'b1), .m_axi_bready(c_bready), + .local_addr(c_addr), .local_wdata(c_wdata), .local_we(c_we), + .local_rdata(64'hA5A5_0000_0000_5A5A) + ); + + // ------------------------------------------------------------------------ + // The observation vector. EVERY output port of the module is in it -- if a + // signal is not here it is not being compared, and the null result would be + // correspondingly weaker. + // ------------------------------------------------------------------------ + wire [292:0] a_vec = {a_busy, a_done, a_araddr, a_arlen, a_arvalid, a_rready, + a_awaddr, a_awlen, a_awvalid, a_wdata_axi, a_wlast, + a_wvalid, a_bready, a_addr, a_wdata, a_we}; + wire [292:0] b_vec = {b_busy, b_done, b_araddr, b_arlen, b_arvalid, b_rready, + b_awaddr, b_awlen, b_awvalid, b_wdata_axi, b_wlast, + b_wvalid, b_bready, b_addr, b_wdata, b_we}; + wire [292:0] c_vec = {c_busy, c_done, c_araddr, c_arlen, c_arvalid, c_rready, + c_awaddr, c_awlen, c_awvalid, c_wdata_axi, c_wlast, + c_wvalid, c_bready, c_addr, c_wdata, c_we}; + + // ------------------------------------------------------------------------ + // Observers and the shared comparator, sampled on the NEGEDGE. + // + // Every output of this module is registered (or a function of registered + // state), so a posedge observer reads values from before that edge's + // non-blocking update. The negedge holds the settled value of the cycle the + // design is actually in. + // ------------------------------------------------------------------------ + integer cmp_cycles = 0; + integer ab_mismatch = 0, bc_mismatch = 0; + integer ab_first = -1, bc_first = -1; + integer a_we_high = 0, b_we_high = 0, c_we_high = 0; + integer a_writes = 0, b_writes = 0, c_writes = 0; + integer a_wbeats = 0, b_wbeats = 0, c_wbeats = 0; + integer a_dones = 0, b_dones = 0, c_dones = 0; + reg a_done_prev = 0, b_done_prev = 0, c_done_prev = 0; + + always @(negedge clk) if (!rst_n) begin + a_done_prev = 0; b_done_prev = 0; c_done_prev = 0; + end else begin + cmp_cycles = cmp_cycles + 1; + + if (a_vec !== b_vec) begin + if (ab_mismatch == 0) begin + ab_first = cmp_cycles; + $display(" FAIL A/B diverged at compared cycle %0d", cmp_cycles); + $display(" A: busy=%b done=%b arvalid=%b rready=%b wvalid=%b local_addr=%0d local_wdata=%0h local_we=%b", + a_busy, a_done, a_arvalid, a_rready, a_wvalid, a_addr, a_wdata, a_we); + $display(" B: busy=%b done=%b arvalid=%b rready=%b wvalid=%b local_addr=%0d local_wdata=%0h local_we=%b", + b_busy, b_done, b_arvalid, b_rready, b_wvalid, b_addr, b_wdata, b_we); + end + ab_mismatch = ab_mismatch + 1; + end + + if (b_vec !== c_vec) begin + if (bc_mismatch == 0) bc_first = cmp_cycles; + bc_mismatch = bc_mismatch + 1; + end + + // Liveness accounting. `local_we` high-cycles and write counts must match + // between A and B for the null result to mean "both did the same work" + // rather than "neither did any". + if (a_we) begin a_we_high = a_we_high + 1; a_writes = a_writes + 1; end + if (b_we) begin b_we_high = b_we_high + 1; b_writes = b_writes + 1; end + if (c_we) begin c_we_high = c_we_high + 1; c_writes = c_writes + 1; end + if (a_wvalid) a_wbeats = a_wbeats + 1; + if (b_wvalid) b_wbeats = b_wbeats + 1; + if (c_wvalid) c_wbeats = c_wbeats + 1; + if (a_done && !a_done_prev) a_dones = a_dones + 1; + if (b_done && !b_done_prev) b_dones = b_dones + 1; + if (c_done && !c_done_prev) c_dones = c_dones + 1; + a_done_prev = a_done; b_done_prev = b_done; c_done_prev = c_done; + end + + task expect_eq(input [511:0] what, input integer got, input integer want); + begin + if (got !== want) begin + errors = errors + 1; + $display(" FAIL %0s: got %0d, want %0d", what, got, want); + end + end + endtask + + // Stimulus changes on the NEGEDGE so `start` is stable across exactly one + // posedge whatever phase the caller is in. Driven from the posedge it races + // the DUTs' own sampling and the transfer can silently never begin. + task pulse_start; + begin + @(negedge clk); d_start = 1; + @(negedge clk); d_start = 0; + end + endtask + + task hard_reset; + begin + @(negedge clk); rst_n = 0; + repeat (4) @(posedge clk); + @(negedge clk); rst_n = 1; + @(posedge clk); + end + endtask + + // ----------------------------------------------------------------------- + // Stimulus. Every phase runs against all three renderings at once and the + // comparator is live throughout -- there is no per-phase enable that could + // be left off. + // ----------------------------------------------------------------------- + initial begin + hard_reset; + + // PHASE 1 -- a plain 4-beat read. This is the phase that makes B and C + // differ, because #2003 changed the READ_DATA address arm. + d_rvalid = 1; d_rlast = 0; d_arready = 1; d_dir = 0; + d_length = 32; + pulse_start; + repeat (40) @(posedge clk); + + // PHASE 2 -- a second read with no reset between, exercising the IDLE + // re-arm path. + @(negedge clk); beats_clr = 1; @(negedge clk); beats_clr = 0; + d_length = 32; + pulse_start; + repeat (40) @(posedge clk); + + // PHASE 3 -- READ_ADDR stretched. `arready` is held low for 6 cycles, so + // the FSM sits in READ_ADDR -- one of the states the pre-fix rendering + // never drives `local_we` from. If a stale strobe could survive into it, + // this is where it would show. + d_arready = 0; + d_length = 32; + pulse_start; + repeat (6) @(posedge clk); + @(negedge clk); d_arready = 1; + repeat (40) @(posedge clk); + + // PHASE 4 -- write direction. IDLE -> WRITE_ADDR -> WRITE_DATA -> DONE_ST, + // and `local_we` is never driven by any of them in the pre-fix rendering. + d_dir = 1; d_length = 32; + pulse_start; + repeat (40) @(posedge clk); + + // PHASE 5 -- write with `wready` throttled, stretching WRITE_DATA. + d_wready = 0; + d_dir = 1; d_length = 32; + pulse_start; + repeat (5) @(posedge clk); + @(negedge clk); d_wready = 1; + repeat (40) @(posedge clk); + + // PHASE 6 -- READ_DATA throttled. `rvalid` is toggled so the FSM sits in + // READ_DATA on cycles where no beat arrives. That is the ONLY place the + // pre-fix rendering's `end else local_we <= 1'b0;` does any work, and + // therefore the only place the #2006 default-low has a live competitor. + // Without this phase the strobe would never be seen falling inside a + // transfer and the null result would be resting on untested ground. + d_dir = 0; d_length = 32; + d_rvalid = 0; + pulse_start; + repeat (3) @(posedge clk); + @(negedge clk); d_rvalid = 1; + repeat (2) @(posedge clk); + @(negedge clk); d_rvalid = 0; + repeat (4) @(posedge clk); + @(negedge clk); d_rvalid = 1; + repeat (40) @(posedge clk); + + // PHASE 7 -- back to a plain read, then a long idle tail with no request + // at all, so IDLE is held for many cycles with nothing driving the strobe. + @(negedge clk); beats_clr = 1; @(negedge clk); beats_clr = 0; + d_dir = 0; d_length = 16; + pulse_start; + repeat (40) @(posedge clk); + repeat (60) @(posedge clk); + + @(negedge clk); #1; + + // ------------------------------------------------------------------ + // Report + // ------------------------------------------------------------------ + $display("== THREE-WAY: A=pre-#2006 B=#2006 C=#2006+#2003 =="); + $display(" compared %0d cycles across 7 stimulus phases", cmp_cycles); + $display(" A: local_we_high=%0d local_writes=%0d axi_write_beats=%0d done_pulses=%0d", + a_we_high, a_writes, a_wbeats, a_dones); + $display(" B: local_we_high=%0d local_writes=%0d axi_write_beats=%0d done_pulses=%0d", + b_we_high, b_writes, b_wbeats, b_dones); + $display(" C: local_we_high=%0d local_writes=%0d axi_write_beats=%0d done_pulses=%0d", + c_we_high, c_writes, c_wbeats, c_dones); + $display(" A vs B: %0d mismatching cycle(s)%0s", ab_mismatch, + ab_mismatch ? "" : " <- the null result under test"); + $display(" B vs C: %0d mismatching cycle(s), first at %0d <- anti-vacuity control", + bc_mismatch, bc_first); + + // ------------------------------------------------------------------ + // LIVENESS. Everything below is vacuous if the stimulus did nothing. + // ------------------------------------------------------------------ + if (cmp_cycles < 200) begin + errors = errors + 1; + $display(" FAIL harness: only %0d cycles compared -- the stimulus did not run", + cmp_cycles); + end + if (a_writes == 0 || b_writes == 0 || c_writes == 0) begin + errors = errors + 1; + $display(" FAIL harness: a rendering produced no local writes (A=%0d B=%0d C=%0d) -- the read path was never exercised", + a_writes, b_writes, c_writes); + end + if (a_wbeats == 0 || b_wbeats == 0 || c_wbeats == 0) begin + errors = errors + 1; + $display(" FAIL harness: a rendering produced no AXI write beats (A=%0d B=%0d C=%0d) -- the write path was never exercised", + a_wbeats, b_wbeats, c_wbeats); + end + if (a_dones < 7 || b_dones < 7 || c_dones < 7) begin + errors = errors + 1; + $display(" FAIL harness: not every phase completed (done pulses A=%0d B=%0d C=%0d, want 7 each)", + a_dones, b_dones, c_dones); + end + + // ------------------------------------------------------------------ + // THE ANTI-VACUITY CONTROL, asserted BEFORE the null result. + // + // The comparator must be shown capable of reporting a difference on this + // very stimulus, over this very signal set, in this very run. Until it + // has, "A equals B" is not evidence of anything. + // ------------------------------------------------------------------ + if (bc_mismatch == 0) begin + errors = errors + 1; + $display(" FAIL anti-vacuity: B and C compared equal. #2003 changes the READ_DATA address arm, so a working comparator MUST see a difference here. Since it does not, the A/B null result below is meaningless and this run proves nothing."); + end + + // ------------------------------------------------------------------ + // THE NULL RESULT. The #2006 default-low is behaviourally latent: every + // reachable path already drove the strobe, so A and B are indistinguishable + // at the ports. + // ------------------------------------------------------------------ + if (ab_mismatch !== 0) begin + errors = errors + 1; + $display(" FAIL A and B differ on %0d cycle(s), first at %0d -- #2006 was expected to be behaviourally latent", + ab_mismatch, ab_first); + end + + // Same work, not merely the same silence. + expect_eq("A vs B local_we high-cycles", b_we_high, a_we_high); + expect_eq("A vs B local writes", b_writes, a_writes); + expect_eq("A vs B AXI write beats", b_wbeats, a_wbeats); + expect_eq("A vs B done pulses", b_dones, a_dones); + + $display(""); + if (errors == 0) $display("RESULT: PASS (0 errors)"); + else $display("RESULT: FAIL (%0d errors)", errors); + $finish; + end + +endmodule From 0f9295eb785941be1acfbeec2890018196e6c27a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 09:03:17 +0700 Subject: [PATCH 4/4] docs(sim): document the three new harnesses and the standalone emit recipe (Refs #2348) Adds a README section per harness in the style #2379 established, plus the shared emit recipe: every BitNet emitter compiles standalone under `rustc` with a four-line driver, no cargo and no target directory, because the only `use` in any of them is `use super::*` inside `#[cfg(test)]`. Records the base/head shas each harness was rendered from, and the three instrument faults found while reproducing the published claims -- a posedge-sampled observer trailing the design by a cycle, an observer racing the stimulus that drove `start_prefetch`, and `first_chunk`/`last_chunk` having no reset in either rendering. Each was a fault in the instrument; every published number reproduced once the instrument was corrected. Refs #1977, #1985, #2006 --- ...348-harnesses-land-one-is-a-null-result.md | 95 +++++++++++ sim/README.md | 160 ++++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 docs/now/2026-08-22-the-three-remaining-2348-harnesses-land-one-is-a-null-result.md diff --git a/docs/now/2026-08-22-the-three-remaining-2348-harnesses-land-one-is-a-null-result.md b/docs/now/2026-08-22-the-three-remaining-2348-harnesses-land-one-is-a-null-result.md new file mode 100644 index 000000000..54637dc36 --- /dev/null +++ b/docs/now/2026-08-22-the-three-remaining-2348-harnesses-land-one-is-a-null-result.md @@ -0,0 +1,95 @@ +# NOW — the three remaining #2348 harnesses land, and one of them is a null result + +Last updated: 2026-08-22 + +## Differential harnesses for #1977, #1985 and #2006 (Refs #2348) + +- Branch: `sim/2348-harnesses` +- Issues: #2348, #1977 (PR #2337), #1985 (PR #2340), #2006 (PR #2344) + +### What landed + +Three testbenches in `sim/`, following the shape #2379 set for +`tb_bitnet_dma_write_address.v`, plus the README sections that document them. + +- `tb_bitnet_sequencer_zero_count.v` — the `layer_sequencer` that never left + RUN for a zero count. Reproduces the published numbers exactly: 200,000 + cycles with no `done`, `neuron_id` reaching **50,000**, and six non-zero + controls byte-identical old vs new. +- `tb_bitnet_prefetch_done_pulse.v` — `prefetch_done` as a level rather than a + one-cycle pulse. Reproduces `t2 sampled_done` OLD=1 / NEW=0, `done_rises` + 2/2, `we_count` 4/4. +- `tb_bitnet_dma_we_default.v` — **three-way**, because its claim is that the + #2006 fix changes nothing observable. + +Every emitter compiles standalone under `rustc` with a four-line driver — no +`cargo`, no target directory. The only `use` in any of them is `use super::*` +inside `#[cfg(test)]`. The recipe is in `sim/README.md`. + +### The null result, and why it needed a third rendering + +#2006 defaults `local_we` low ahead of the `case`. Pre-fix and post-fix are +observationally identical: every reachable path already drove the strobe, and +the states that never mention it are never entered with it high. A harness that +passes by finding no difference proves nothing — one wired to the wrong ports +finds no difference too. + +So the harness elaborates three renderings: A (pre-#2006), B (#2006), C +(#2006 + #2003). B and C are consecutive revisions — PR #2344's head rendering +is byte-identical to PR #2345's base — so one comparator sees all three. **A vs +B must be identical; B vs C must differ.** Same comparator, same stimulus, same +run, same 293-bit vector. Substituting B into the C slot makes the run fail, +explicitly refusing to certify its own null result. + +Measured: A vs B `0` mismatching cycles, B vs C `266`, over 373 cycles and +seven stimulus phases. + +### Biting + +One mutant per guard, each failing with a quoted message: + +- Guard on `num_neurons==0` only → case 1 passes, case 2 fails: the two zero + compares are independently guarded. +- Guard forced to `1'b1` → cases 1 and 2 pass, **all six** non-zero controls + fail. The controls are what stop the fix being a licence to retire every + request. +- `prefetch_done` clear moved back inside the start guard while keeping the new + `IDLE: begin` syntax → fails. The harness measures ports, not emitter text. +- BRAM address stride broken while the flag stays correct → fails the + surgical-ness control. +- `READ_DATA`'s `end else local_we <= 1'b0;` deleted from both DMA renderings → + A emits **22** local writes against B's **18**. #2006 is latent today and a + real backstop the moment an arm stops clearing the strobe. + +Anti-vacuity in every harness: the fixed rendering placed in the *old* slot +makes each one fail, so none can pass against a non-defective "before". + +### Honesty bounds (BINDING) + +- These are **reporting** instruments, **not gates**. `vvp` exits 0 on + `RESULT: FAIL` as well as `RESULT: PASS`, and no workflow runs them — `cargo + test -p t27c` is invoked by none (#2292) and `fpga-build.yml` never calls + `vvp` (#2241). Wiring `sim/` into CI is tracked separately. No claim is made + that anything here defends `master`. +- Three published numbers initially failed to reproduce. **All three were + faults in my instrument, not in the claims**, and each was diagnosed before + any assertion was adjusted: + - A posedge-sampled observer read registered outputs before their + non-blocking update and reported `max_nid=49999` while the port plainly + read 50000. `200000/4` confirms 50000 independently. Observers moved to the + negedge. + - A negedge observer read `d_start` on the same negedge the stimulus assigned + it — undefined order — and reported `t2 sampled_done=0` for both + renderings. The sample moved into the driving process. + - `first_chunk`/`last_chunk` are **absent from the reset block of both + renderings**, so both sit at X until the first RUN cycle and `X !== X`. + Those two are now compared only while `valid` is high, with a + `qual_cycles` counter asserted equal to `num_neurons*num_chunks` so the + qualification cannot void the check. +- A fourth bug was found only by *running a mutant*: `$display` two-string + continuation together with format args printed `"expe"` as `1702391909`. + Failure messages that are never exercised are not known to work. +- The zero-chunk half of #1977 is asserted separately from the zero-neuron + half. The published claim covered only the zero-neuron case; the second is + additional, and its numbers (`chunk_id` high-water 255, final 64 = 200000 mod + 256) are measured here, not quoted from the PR. diff --git a/sim/README.md b/sim/README.md index 43a89672c..da020f59e 100644 --- a/sim/README.md +++ b/sim/README.md @@ -143,3 +143,163 @@ test -p t27c` is invoked by no workflow (removed by #2292) and `fpga-build.yml` never calls `vvp` (#2241). It prints `RESULT: PASS`/`RESULT: FAIL` and exits 0 either way, matching `tb_bitnet_request_overflow.v`. Wiring `sim/` into CI is tracked separately. + +## Emitting the pre-fix and post-fix Verilog + +All four BitNet emitters compile **standalone under `rustc`** — no `cargo`, no +target directory, seconds per revision. The only `use` in any of them is +`use super::*` inside a `#[cfg(test)]` module, which `rustc` never reaches +without `--test`. + +``` +# $1 = bootstrap/src file, $2 = commit sha, $3 = emitter fn, $4 = module name +gh api "repos/gHashTag/t27/contents/bootstrap/src/$1?ref=$2" --jq .content \ + | base64 -d > src_$2.rs +printf '#[path = "src_%s.rs"]\nmod e;\nfn main(){let a:Vec=std::env::args().collect();print!("{}",e::%s(&a[1]));}\n' "$2" "$3" > drv_$2.rs +rustc -O --edition 2021 -o drv_$2 drv_$2.rs +./drv_$2 "$4" > "$4.v" +``` + +Base and head shas come from `gh api repos/gHashTag/t27/pulls/N --jq +'.base.sha, .head.sha'`. The per-harness tables below give the ones used. + +## `tb_bitnet_sequencer_zero_count.v` — issue #1977 + +Differential harness for the `layer_sequencer` that never terminated when +asked for zero work. Both terminators are `index == count-1` compares against +an unsigned port, and the bare literal `1` widens each subtraction to 32 bits, +so a zero count borrows to `32'hFFFFFFFF` while the index zero-extends. No +value a 16-bit `neuron_id` or an 8-bit `chunk_id` can hold ever matches, the +FSM never reaches `DONE_ST`, and `valid` is asserted forever for work nobody +requested. + +| emitter | `bootstrap/src/bitnet_pipeline.rs`, `build_layer_sequencer` | +|---|---| +| pre-fix | `4ea72c322fa5572fc0c33fb5deedb739b7ad6c6a` (PR #2337 base) | +| post-fix | `e3c2d655fbcae69dd41c4a050d3feb801ac2d129` (PR #2337 head) | + +``` +iverilog -g2005 -o tb.vvp sim/tb_bitnet_sequencer_zero_count.v seq_old.v seq_new.v +vvp tb.vvp +``` + +### What it asserts + +| case | stimulus | expectation | +|---|---|---| +| 1 | `num_neurons=0`, `num_chunks=4`, 200,000 cycles | old never pulses `done` and is *still counting* — `neuron_id` reaches exactly 50,000 (200000/4); new retires with `valid` never raised | +| 2 | `num_neurons=8`, `num_chunks=0`, 200,000 cycles | the other borrow: old's `chunk_id` free-runs and wraps (high-water 255, final 64 = 200000 mod 256) while `neuron_id` never advances | +| 3–8 | six non-zero requests | old and new identical on **every output, every cycle** | + +Case 1 asserts the old rendering still hangs *and* still counts: a rendering +frozen with all outputs static would also show `done=0`, and that is a +different defect. The six non-zero controls are what stop the guard being a +licence to retire everything — a mutant guarded on `1'b1` passes cases 1 and 2 +and fails all six. + +`first_chunk`/`last_chunk` are compared only while `valid` is high. **Neither +rendering resets them** — the emitted reset block is `state<=IDLE; +neuron_id<=0; chunk_id<=0; valid<=0; done<=0;` — so both sit at X until the +first RUN cycle and `X !== X`. That is a property of the design, identical +either side of the fix. `qual_cycles` counts how often the qualified +comparison actually ran and is asserted equal to `num_neurons*num_chunks`, so +the qualification cannot silently void that half of the vector. + +## `tb_bitnet_prefetch_done_pulse.v` — issue #1985 + +Differential harness for `weight_prefetch_ctrl`'s `prefetch_done`, documented +as a one-cycle pulse but implemented as a level. The pre-fix emitter cleared +the flag only *inside* the start guard, so it stayed asserted for the whole +idle gap. A requester that samples it in the same cycle it raises +`start_prefetch` — exactly what `multilayer_sequencer`'s `WAIT_PF` state does — +reads the previous transaction's completion and skips its own prefetch. + +| emitter | `bootstrap/src/bitnet_buffers.rs`, `build_weight_prefetch_ctrl` | +|---|---| +| pre-fix | `e058a03ea20397ae4f066a57ad12ad25e01d78df` (PR #2340 base) | +| post-fix | `851dc6d99ed6f88b6bb5fd02cbf93a89ec71900a` (PR #2340 head) | + +``` +iverilog -g2005 -o tb.vvp sim/tb_bitnet_prefetch_done_pulse.v pf_old.v pf_new.v +vvp tb.vvp +``` + +### What it asserts + +| case | stimulus | expectation | +|---|---|---| +| 1 | two 2-word transactions, 8-cycle gap | at request 2 old reads the stale `prefetch_done=1`, new reads `0`; both show `done_rises=2` and `we_count=4` | +| 2 | the same with a 40-cycle gap | old's high-time grows by exactly the extra 32 cycles — it is a **level**; new's stays at 2, one cycle per completion — it is a **pulse** | + +Case 2 is what distinguishes the two readings of case 1: a single sample can +be explained by phase, but only a level stretches with the gap. Both cases +also assert that at the *first* request both renderings read the flag low, +which is what makes the request-2 reading a difference in staleness rather +than a constant offset. + +Every output **except** `prefetch_done` is compared cycle by cycle and must be +identical (the fix is surgical), while `prefetch_done` itself must differ on at +least one cycle (the harness is observing something). Those two assertions are +anti-vacuity controls pointing in opposite directions: a mutant that fixes the +flag but breaks the BRAM address stride fails the first, and a mutant that +changes nothing fails the second. + +## `tb_bitnet_dma_we_default.v` — issue #2006 + +**Three-way** differential harness for the `local_we` default-low. Its result +is a **null** one, and that is the whole difficulty: the pre-fix and post-fix +renderings are observationally *identical*. Reading the pre-fix output, every +reachable path already drives the strobe — reset clears it, `READ_DATA` sets +it and has an explicit `else` clear, `DONE_ST` clears it — and the states that +never mention it are never entered with it high, because `READ_DATA`'s only +exit is `DONE_ST`. The fix is defence in depth against a future arm, not a +repair of an observable defect. + +A harness that "passes" by finding no difference proves nothing on its own: +one wired to the wrong ports finds no difference either. So this harness +elaborates **three** renderings and runs one comparator over two pairs: + +| slot | rendering | sha | +|---|---|---| +| A | pre-#2006 | `bd2d25df4b2e5bcc4cca61eb3da3b3505c73df7d` (PR #2344 base) | +| B | #2006 applied | `4f07aa84acdac656977ea45b284288f2b6d2ba69` (PR #2344 head) | +| C | #2006 + #2003 | `4db5729b1817a2d0f0d453e34c707ab425956934` (PR #2345 head) | + +B and C are consecutive revisions of the same file — PR #2344's head rendering +is byte-identical to PR #2345's base rendering — so A, B and C form a linear +chain and one comparator sees all three. + +``` +iverilog -g2005 -o tb.vvp sim/tb_bitnet_dma_we_default.v dma_a.v dma_b.v dma_c.v +vvp tb.vvp +``` + +### What it asserts + +* **A vs B must be identical** — the null result under test. +* **B vs C must differ** — the anti-vacuity control. #2003 changed the + `READ_DATA` address arm, so a working comparator has to see it. + +The control is not a separate test. It is the same comparator, on the same +stimulus, in the same simulation, over the same 293-bit vector of every output +port. If the comparator is blind — misconnected ports, an enable left off, a +vector omitting `local_we` — then B vs C reports "identical" too and the run +**fails**, explicitly refusing to certify the A/B null result. Only once the +instrument has shown on C that it *can* see a difference does "A equals B" +carry information. + +Seven stimulus phases cover read, back-to-back read, stretched `READ_ADDR` +(`arready` held low), write, throttled `WRITE_DATA`, throttled `READ_DATA` +(`rvalid` toggled), and a long idle tail. Phase 6 is load-bearing: throttling +`rvalid` is the only condition under which the pre-fix `end else local_we <= +1'b0;` does any work, and hence the only place the #2006 default has a live +competitor. Deleting that `else` clear from both renderings makes A emit **22** +local writes against B's 18 — four spurious strobes — with the first +divergence inside phase 6. The fix is latent today and a real backstop the +moment an arm stops clearing the strobe. + +### Status + +Like the harnesses above, all three are **reporting** instruments, not gates. +`vvp` exits 0 on `RESULT: FAIL` as well as `RESULT: PASS`, and no workflow runs +them.