fix(v2): resolve ERR-0025 Part B - SRAM read timing bug in weight/activation memory
Root-causes and fixes the real, disclosed defect left open at the end of the previous STEP20 commit: the board-level SPI host interface produced wrong compute results when jobs were dispatched with realistic (widely time-separated) pacing, even though job registration itself was already confirmed correct at the dependency_manager handshake. Root cause: nms_weight_packed.v and nms_activation_replicated.v both used a REGISTERED SRAM read (rd_data_reg <= mem[addr], one full clock of latency), but nms_memory_manager_stream_wide.v's own read-ahead pipeline (its `rd_pending` bit) is designed around a COMBINATIONAL read -- a request issued this cycle produces data already valid to capture the very next cycle. A busy, multi-tile job (e.g. the STEP19 D-Stress regression, 16 tiles/neuron) never exposes the mismatch, since its own weight/activation prefetch always runs far enough ahead that any given tile has been sitting stable in the SRAM for many cycles by the time it's actually consumed. An uncontested single-tile job has zero such margin: its one tile's read fires on the exact edge the data nominally becomes ready, landing squarely on the missing cycle and permanently latching stale/zero data. Fixed by making both SRAMs' reads combinational, with an explicit same-cycle fill/read address-match bypass for the one hazard a plain combinational read alone would still miss. No FSM, arbiter, or SDRAM controller logic was touched. Verified (Verilator, per this project's own standing DEC-0004 protocol): - tb_fpga_neural_v2_top_smoke.v: 11/11 PASS -- single job, back-to-back jobs, a realistic ~85us-gap job pair, and a parametric sweep of inter-job gaps (100ns/5000ns/50000ns). - STEP19 D-Stress N=2: 49788 cycles, 256/256 bit-exact -- identical cycle count to before this fix (zero regression). - STEP19 D-Stress N=4: 49771 cycles, 256/256 bit-exact -- identical cycle count to before this fix (zero regression). - tb_sdram_unified_backend.v (40/40) and tb_spi_host_bridge.v (18/18) reconfirmed unaffected. The physical SPI host interface is now verified correct end-to-end. Real synthesis/P&R of the board-level top (fpga_neural_v2_top.v) is the deliberate next step, not yet performed this round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013xXuuRUWZScuo1DeYJxs3v
This commit is contained in:
@@ -38,16 +38,43 @@ module nms_activation_replicated #(
|
||||
generate
|
||||
for (g = 0; g < N_SLOTS; g = g + 1) begin : GEN_COPY
|
||||
reg [DATA_WIDTH*P_IN-1:0] mem [0:MAX_TILES-1];
|
||||
reg [DATA_WIDTH*P_IN-1:0] rd_data_reg;
|
||||
|
||||
always @(posedge clk) begin
|
||||
if (fill_we)
|
||||
mem[fill_addr] <= fill_data;
|
||||
if (rd_en[g])
|
||||
rd_data_reg <= mem[rd_addr_flat[g*TIW +: TIW]];
|
||||
end
|
||||
|
||||
assign rd_data_flat[g*DATA_WIDTH*P_IN +: DATA_WIDTH*P_IN] = rd_data_reg;
|
||||
// ROOT CAUSE (found via STEP20's own board-level SPI
|
||||
// integration smoke test, ERR-0025 Part B): this read used
|
||||
// to be REGISTERED (rd_data_reg <= mem[addr], gated by
|
||||
// rd_en[g]) -- a full extra clock cycle of latency beyond
|
||||
// what nms_memory_manager_stream_wide.v's own read-ahead
|
||||
// pipeline (its `rd_pending` bit) actually assumes. That
|
||||
// pipeline issues a read one cycle and captures the result
|
||||
// the VERY NEXT cycle -- correct only if this memory's own
|
||||
// read is COMBINATIONAL (address in this cycle, data
|
||||
// already valid this same cycle), not registered (address
|
||||
// in this cycle, data valid only the cycle after). A busy,
|
||||
// multi-tile job never exposes the extra cycle because its
|
||||
// own weight/activation prefetch always runs far enough
|
||||
// ahead that, by the time a given tile is actually
|
||||
// consumed, that data has been sitting stable for many
|
||||
// cycles already. An uncontested single-tile job has zero
|
||||
// such margin: its first (only) tile's read fires on the
|
||||
// exact edge the data becomes nominally "ready", and the
|
||||
// consumer captured one real cycle before the registered
|
||||
// output ever updated -- permanently latching stale
|
||||
// (all-zero, reset-value) data. Fixed by making the read
|
||||
// itself combinational, matching the consumer's actual
|
||||
// latency assumption, with NO change to any FSM timing.
|
||||
// The same-cycle fill/read-to-the-same-address case (fill_we
|
||||
// and this slot's own read targeting the identical tile on
|
||||
// the identical edge) is bypassed explicitly, since mem[]
|
||||
// itself will not show a same-edge write until the NEXT
|
||||
// cycle even with a combinational read.
|
||||
wire rd_bypass = fill_we && (fill_addr == rd_addr_flat[g*TIW +: TIW]);
|
||||
assign rd_data_flat[g*DATA_WIDTH*P_IN +: DATA_WIDTH*P_IN] =
|
||||
rd_bypass ? fill_data : mem[rd_addr_flat[g*TIW +: TIW]];
|
||||
end
|
||||
endgenerate
|
||||
|
||||
|
||||
@@ -35,17 +35,34 @@ module nms_weight_packed #(
|
||||
for (g = 0; g < N_SLOTS; g = g + 1) begin : GEN_SLOT
|
||||
for (p = 0; p < P_IN; p = p + 1) begin : GEN_LANE
|
||||
reg [DATA_WIDTH-1:0] mem [0:MAX_TILES-1];
|
||||
reg [DATA_WIDTH-1:0] rd_data_reg;
|
||||
|
||||
always @(posedge clk) begin
|
||||
if (fill_we[g])
|
||||
mem[fill_addr_flat[g*TIW +: TIW]] <=
|
||||
fill_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH];
|
||||
if (rd_en[g])
|
||||
rd_data_reg <= mem[rd_addr_flat[g*TIW +: TIW]];
|
||||
end
|
||||
|
||||
assign rd_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH] = rd_data_reg;
|
||||
// ROOT CAUSE (STEP20, ERR-0025 Part B) -- see
|
||||
// nms_activation_replicated.v's own header for the
|
||||
// full writeup: this read must be COMBINATIONAL, not
|
||||
// registered, to match nms_memory_manager_stream_wide.v's
|
||||
// own `rd_pending` pipeline's actual 1-cycle latency
|
||||
// assumption (issue this cycle, capture next cycle). A
|
||||
// registered read added a second, uncounted cycle of
|
||||
// latency that a busy multi-tile job's own prefetch
|
||||
// lead time always absorbed invisibly, but an
|
||||
// uncontested single-tile job's first (only) tile does
|
||||
// not -- permanently latching stale/zero data. The
|
||||
// same-cycle fill/read bypass covers the one case a
|
||||
// combinational read alone would still miss: a fill
|
||||
// and a read to the identical address landing on the
|
||||
// identical edge (mem[] itself only reflects a
|
||||
// same-edge write starting the NEXT cycle).
|
||||
wire rd_bypass = fill_we[g] &&
|
||||
(fill_addr_flat[g*TIW +: TIW] == rd_addr_flat[g*TIW +: TIW]);
|
||||
assign rd_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH] =
|
||||
rd_bypass ? fill_data_flat[g*DATA_WIDTH*P_IN + p*DATA_WIDTH +: DATA_WIDTH]
|
||||
: mem[rd_addr_flat[g*TIW +: TIW]];
|
||||
end
|
||||
end
|
||||
endgenerate
|
||||
|
||||
Reference in New Issue
Block a user