From 6a46dc857fa3a5bf0894ed9f8955ef2f4f68642c Mon Sep 17 00:00:00 2001 From: manvalan Date: Tue, 15 Sep 2026 22:39:00 +0000 Subject: [PATCH] fix: propagate runtime-indexed-crossbar fix to slot_mem_arbiter(_wide) + dataflow_core mux DEC-0043: slot_mem_arbiter.v/slot_mem_arbiter_wide.v both still had the exact runtime-variable-indexed part-select anti-pattern (pending_addr[grant_idx*ADDR_WIDTH +: ADDR_WIDTH], grant_idx a runtime register) that neural_director.v had already found and fixed once before -- ADDR_WIDTH=26 not being a power of 2 means this synthesizes as a real multiplier + wide crossbar, sitting right on the arbiter<->backend boundary this project's own N=8 congestion diagnosis names, growing with N_PORTS=N_SLOTS(+1). Also fixed the cheaper but same-class dir_job_out_slot*16 mux in nms_dataflow_core_sdram.v, feeding directly into dependency_manager -- this exact signal was DEC-0042's own diagnosed N=4 critical path. Fix: N_PORTS/N_SLOTS parallel constant-indexed comparisons (unrolled for-loop) instead of a runtime-indexed read -- same technique already proven in neural_director.v. Purely an internal-implementation change. Verified bit-exact via Verilator: tb_fpga_neural_v2_top_smoke.v 11/11 PASS; tb_nms_dstress_sdram_unified.v (256-neuron stress) at both N_SLOTS_CFG=4 and =8, 256/256 bit-exact vs golden, total_cycles IDENTICAL to pre-fix historical values (49927/49909, exact match to DEC-0042's own recorded numbers). Bonus finding from the same D-Stress run (not this commit's main point, logged for Phase 3/4): sdram_busy_cycles ~81.6% and useful-MAC-cycle fraction HALVING from N=4 to N=8 (2.04%->1.02%) -- real existing evidence the system is memory-bound on a single SDRAM bank well before N=8, independent of Fmax. Re-synthesis (8-seed sweep, N=4/N=8) in progress to measure the actual Fmax delta from this fix -- committed separately once complete. Co-Authored-By: Claude Sonnet 5 --- hardware/v2/logs/decisions.log | 75 +++++++++++++++++++ hardware/v2/logs/simulation.log | 24 ++++++ hardware/v2/nms/rtl/nms_dataflow_core_sdram.v | 25 ++++++- hardware/v2/rtl/slot_mem_arbiter.v | 47 ++++++++++-- hardware/v2/rtl/slot_mem_arbiter_wide.v | 47 ++++++++++-- 5 files changed, 204 insertions(+), 14 deletions(-) diff --git a/hardware/v2/logs/decisions.log b/hardware/v2/logs/decisions.log index d37e3fa..1a7a1ad 100644 --- a/hardware/v2/logs/decisions.log +++ b/hardware/v2/logs/decisions.log @@ -2462,3 +2462,78 @@ N_SLOTS=8 explicitly deferred by user request, not attempted further this pass. Flash #1 removed; its RTL (flash_mem_adapter.v, tb_flash_integration_smoke.v, the OP_FLASH_CMD opcode) remains in git history (revertible commit `59901a4`) if ever needed again. + +DEC-0043 -- Propagated the neural_director.v runtime-indexed-crossbar +fix to slot_mem_arbiter.v/slot_mem_arbiter_wide.v and to +nms_dataflow_core_sdram.v's own director->dependency_manager mux + +DATE: 2026-09-15 +CONTEXT: new brief (N=8 timing closure, LFE5U-85F retarget, 4/8/16 x +1/2-bank SDRAM sweep). Phase 0 established N_SLOTS=4/8 both already +PASS at 64MHz on this session's toolchain (EXP-0049/EXP-0050) with +real margin, but RTL analysis of the arbiter<->backend boundary (the +congestion hub the brief itself names) found a real, concrete, +previously-missed instance of an anti-pattern this project had ALREADY +found and fixed once before, just not everywhere it occurs. +ROOT CAUSE (real, found by direct RTL inspection, not guessed): +neural_director.v's own header comment documents a real historical +bug -- a runtime-computed part-select (`slot_x_base[free_slot_idx* +ADDR_WIDTH +: ADDR_WIDTH] <= ...`) synthesized as an actual +MULT18X18D hard multiplier feeding a wide demux/crossbar (ADDR_WIDTH=26 +is not a power of 2), collapsing worst-seed Fmax from 68.51MHz to +40-47MHz -- fixed there via N_SLOTS parallel CONSTANT-indexed +comparisons instead. The SAME exact pattern was found, unfixed, in: + - rtl/slot_mem_arbiter.v line ~162-163 and rtl/slot_mem_arbiter_ + wide.v line ~163-164: `m_addr <= pending_addr[grant_idx* + ADDR_WIDTH +: ADDR_WIDTH]` where `grant_idx` is a REGISTER (the + output of the arbiter's own priority-encoder), not a constant -- + sitting exactly on the arbiter<->backend boundary, whose N_PORTS + (=N_SLOTS+1 / N_SLOTS) scales with N_SLOTS. + - nms/rtl/nms_dataflow_core_sdram.v line 189: `dir_slot_node_id[ + dir_job_out_slot*16 +: 16]`, feeding directly into + dependency_manager -- the multiplier here is by a power of 2 (16), + so cheaper (no real hardware multiplier), but the resulting + N_SLOTS-way runtime mux still grows with N_SLOTS, and this EXACT + signal was DEC-0042's own diagnosed N=4 critical path (neural_ + director.job_out_slot -> dependency_manager.node_resolved/ + node_state) -- confirmed again in this session's own N=4 seed3 + critical path report (EXP-0049). +FIX: replaced all three runtime-indexed reads with N_PORTS/N_SLOTS +parallel CONSTANT-indexed comparisons (`for` loop, unrolled at +elaboration, each iteration's index is a compile-time constant) -- +same technique neural_director.v itself already established. +Functionally IDENTICAL: exactly one iteration's condition is ever true +(grant_idx/dir_job_out_slot is always a single valid index when the +mux fires), so this is a pure internal-implementation change, zero +external behavior change. Files changed: rtl/slot_mem_arbiter.v, +rtl/slot_mem_arbiter_wide.v, nms/rtl/nms_dataflow_core_sdram.v. +VERIFICATION (bit-exact, real): tb_fpga_neural_v2_top_smoke.v (the +real board-level top's own smoke test, run via Verilator 5.053 with +-DSIM for the PLL behavioral bypass): 11/11 PASS, unchanged. tb_nms_ +dstress_sdram_unified.v (256-neuron stress regression, the project's +main bit-exact oracle) at both N_SLOTS_CFG=4 and N_SLOTS_CFG=8: PASS, +all 256 neurons bit-exact vs golden, total_cycles IDENTICAL to the +pre-fix historical values (N=4: 49927 cycles; N=8: 49909 cycles, +matching DEC-0042's own recorded numbers exactly) -- confirms zero +behavioral/cycle-count change, as expected for an internal- +implementation-only fix. +BONUS FINDING (not this decision's main point, but real and worth +flagging for Phase 3/4): tb_nms_dstress_sdram_unified.v's own +benchmark report already shows sdram_busy_cycles ~81.6% and weight_ +stall_cycles 89-94% of total_cycles*N_SLOTS at BOTH N=4 and N=8, with +the useful-MAC-cycle fraction actually HALVING from N=4 to N=8 +(2.04%->1.02% of slot-cycles) -- strong existing evidence, from a +testbench this project already had, that the system is memory-bound +(single shared SDRAM channel) well before N=8, independent of Fmax. +Relevant to the brief's own Phase 4 question; see experiments.log. +STATUS: fix applied and bit-exact verified. Re-synthesis (fresh 8-seed +nextpnr-ecp5 sweep, N=4/N=8, real fpga_neural_v2_top + +v2_board_top.lpf) in progress to measure the actual Fmax delta -- see +timing.log for the result once available. + +TOOLCHAIN NOTE: this session's environment had no C/C++ toolchain +(make/gcc/g++) or working Perl FindBin module, both required to build/ +run Verilator-based simulations -- installed via `dnf install -y make +gcc-c++ perl-FindBin libatomic` (system packages, not project files) +to make bit-exact verification possible at all. Recorded here since it +was a real, if minor, environment gap. diff --git a/hardware/v2/logs/simulation.log b/hardware/v2/logs/simulation.log index fa12b0e..09369cf 100644 --- a/hardware/v2/logs/simulation.log +++ b/hardware/v2/logs/simulation.log @@ -180,3 +180,27 @@ errors: 2 real bugs found and fixed during implementation (errors.log a new instance in the activation-cache side of memory_manager.v), and a repeat of ERR-0009's N_SLOTS=1 zero-width replication bug (this time in activation_cache.v itself). + +[2026-09-15] DEC-0043 verification -- Verilator 5.053 (-DSIM for PLL +behavioral bypass on tb_fpga_neural_v2_top_smoke.v; plain build for +tb_nms_dstress_sdram_unified.v, -GN_SLOTS_CFG override) +test=tb_fpga_neural_v2_top_smoke, config=fpga_neural_v2_top N_SLOTS=4 + (default): 11/11 PASS, unchanged. +test=tb_nms_dstress_sdram_unified (D-Stress, 256 neurons, 128 inputs + each), config=N_SLOTS_CFG=4: PASS, 256/256 bit-exact vs golden, + total_cycles=49927 (IDENTICAL to pre-fix), sdram_busy=81.56%, + weight_stall=89.37% of total_cycles*N_SLOTS, useful_mac_cycles=2.04% + of slot-cycles. +test=tb_nms_dstress_sdram_unified (D-Stress), config=N_SLOTS_CFG=8: + PASS, 256/256 bit-exact vs golden, total_cycles=49909 (IDENTICAL to + pre-fix, matches DEC-0042's own recorded N=8 cycle count exactly), + sdram_busy=81.62%, weight_stall=94.54% of total_cycles*N_SLOTS, + useful_mac_cycles=1.02% of slot-cycles (HALF of N=4's fraction -- + more slots sharing one SDRAM channel does not proportionally + increase useful work). +Result: zero behavioral/cycle-count regression from the slot_mem_ +arbiter.v/slot_mem_arbiter_wide.v/nms_dataflow_core_sdram.v runtime- +index fix (DEC-0043). Real, disclosed evidence the system is +memory-bound at both N=4 and N=8 on a single SDRAM bank, independent +of the arbiter fix or Fmax -- relevant to Phase 3/4 of the current +brief. diff --git a/hardware/v2/nms/rtl/nms_dataflow_core_sdram.v b/hardware/v2/nms/rtl/nms_dataflow_core_sdram.v index 157bc85..558cc39 100644 --- a/hardware/v2/nms/rtl/nms_dataflow_core_sdram.v +++ b/hardware/v2/nms/rtl/nms_dataflow_core_sdram.v @@ -186,9 +186,30 @@ module nms_dataflow_core_sdram #( end assign data_ready = data_ready_reg; - wire [15:0] completed_node_id_16 = dir_slot_node_id[dir_job_out_slot*16 +: 16]; + // completed_node_id_16: which slot's node_id feeds dependency_manager + // this cycle, selected via N_SLOTS parallel CONSTANT-indexed reads + // (`cni` is the for-loop's own unrolled constant) instead of a + // runtime-indexed part-select (`dir_job_out_slot*16`) -- same fix + // class as neural_director.v's own write-side fix (see that file's + // header) and slot_mem_arbiter.v/slot_mem_arbiter_wide.v's grant_idx + // fix: this exact signal was DEC-0042's own diagnosed N=4 critical + // path (neural_director.job_out_slot -> dependency_manager. + // node_resolved/node_state). The multiplier here is by a power of 2 + // (16), so no real hardware multiplier was involved, but the + // resulting N_SLOTS-way runtime mux still grows with N_SLOTS. + // Functionally IDENTICAL (exactly one cni matches dir_job_out_slot). + reg [15:0] completed_node_id_16_c; + integer cni; + always @(*) begin + completed_node_id_16_c = 16'h0000; + for (cni = 0; cni < N_SLOTS; cni = cni + 1) begin + if (dir_job_out_slot == cni[$clog2(N_SLOTS)-1:0]) begin + completed_node_id_16_c = dir_slot_node_id[cni*16 +: 16]; + end + end + end assign dm_producer_done_valid = dir_job_out_done; - assign dm_producer_done_node_id = completed_node_id_16[NODE_IDW-1:0]; + assign dm_producer_done_node_id = completed_node_id_16_c[NODE_IDW-1:0]; // ---- NMS memory: shared Activation SRAM (replicated) + private // Weight SRAM (packed), per DEC-0019/DEC-0020 ---- diff --git a/hardware/v2/rtl/slot_mem_arbiter.v b/hardware/v2/rtl/slot_mem_arbiter.v index cbdc3d2..020570f 100644 --- a/hardware/v2/rtl/slot_mem_arbiter.v +++ b/hardware/v2/rtl/slot_mem_arbiter.v @@ -110,6 +110,39 @@ module slot_mem_arbiter #( end end + // grant_idx-selected pending fields, read out via N_PORTS parallel + // CONSTANT-indexed comparisons (`gi` is the for-loop's own unrolled + // constant, not a runtime value) instead of a runtime-indexed part- + // select of a wide packed array -- same fix class already applied + // in neural_director.v (see that file's own header comment): a + // variable-indexed read/write of a wide packed array synthesizes as + // a real multiplier (index * ADDR_WIDTH, ADDR_WIDTH=26 not a power + // of 2) feeding a wide demux/crossbar, measurably worse as + // ADDR_WIDTH/N_PORTS grow -- exactly the arbiter<->backend boundary + // this project's own N=8 congestion diagnosis names. Functionally + // IDENTICAL to the old `pending_*[grant_idx]` reads (exactly one gi + // matches grant_idx whenever any_pending is set). + reg grant_wr_c, grant_lb_n_c, grant_ub_n_c; + reg [ADDR_WIDTH-1:0] grant_addr_c; + reg [15:0] grant_wdata_c; + integer gi; + always @(*) begin + grant_wr_c = 1'b0; + grant_lb_n_c = 1'b1; + grant_ub_n_c = 1'b1; + grant_addr_c = {ADDR_WIDTH{1'b0}}; + grant_wdata_c = 16'h0000; + for (gi = 0; gi < N_PORTS; gi = gi + 1) begin + if (grant_idx == gi[PIDXW-1:0]) begin + grant_wr_c = pending_wr[gi]; + grant_lb_n_c = pending_lb_n[gi]; + grant_ub_n_c = pending_ub_n[gi]; + grant_addr_c = pending_addr[gi*ADDR_WIDTH +: ADDR_WIDTH]; + grant_wdata_c = pending_wdata[gi*16 +: 16]; + end + end + end + integer pi; always @(posedge clk) begin @@ -156,12 +189,14 @@ module slot_mem_arbiter #( if (any_pending) begin owner <= grant_idx + 1'b1; m_req <= 1'b1; - m_wr <= pending_wr[grant_idx]; - m_lb_n <= pending_lb_n[grant_idx]; - m_ub_n <= pending_ub_n[grant_idx]; - m_addr <= pending_addr[grant_idx*ADDR_WIDTH +: ADDR_WIDTH]; - m_wdata <= pending_wdata[grant_idx*16 +: 16]; - pending[grant_idx] <= 1'b0; + m_wr <= grant_wr_c; + m_lb_n <= grant_lb_n_c; + m_ub_n <= grant_ub_n_c; + m_addr <= grant_addr_c; + m_wdata <= grant_wdata_c; + for (pi = 0; pi < N_PORTS; pi = pi + 1) begin + if (grant_idx == pi[PIDXW-1:0]) pending[pi] <= 1'b0; + end end end else begin if (m_ready) begin diff --git a/hardware/v2/rtl/slot_mem_arbiter_wide.v b/hardware/v2/rtl/slot_mem_arbiter_wide.v index 925b9bd..f5b0afd 100644 --- a/hardware/v2/rtl/slot_mem_arbiter_wide.v +++ b/hardware/v2/rtl/slot_mem_arbiter_wide.v @@ -111,6 +111,39 @@ module slot_mem_arbiter_wide #( end end + // grant_idx-selected pending fields, read out via N_PORTS parallel + // CONSTANT-indexed comparisons (`gi` is the for-loop's own unrolled + // constant, not a runtime value) instead of a runtime-indexed part- + // select of a wide packed array -- same fix class already applied + // in neural_director.v (see that file's own header comment): a + // variable-indexed read/write of a wide packed array synthesizes as + // a real multiplier (index * ADDR_WIDTH, ADDR_WIDTH=26 not a power + // of 2) feeding a wide demux/crossbar, measurably worse as + // ADDR_WIDTH/N_PORTS grow -- exactly the arbiter<->backend boundary + // this project's own N=8 congestion diagnosis names. Functionally + // IDENTICAL to the old `pending_*[grant_idx]` reads (exactly one gi + // matches grant_idx whenever any_pending is set). + reg grant_wr_c, grant_lb_n_c, grant_ub_n_c; + reg [ADDR_WIDTH-1:0] grant_addr_c; + reg [DATA_WIDTH-1:0] grant_wdata_c; + integer gi; + always @(*) begin + grant_wr_c = 1'b0; + grant_lb_n_c = 1'b1; + grant_ub_n_c = 1'b1; + grant_addr_c = {ADDR_WIDTH{1'b0}}; + grant_wdata_c = {DATA_WIDTH{1'b0}}; + for (gi = 0; gi < N_PORTS; gi = gi + 1) begin + if (grant_idx == gi[PIDXW-1:0]) begin + grant_wr_c = pending_wr[gi]; + grant_lb_n_c = pending_lb_n[gi]; + grant_ub_n_c = pending_ub_n[gi]; + grant_addr_c = pending_addr[gi*ADDR_WIDTH +: ADDR_WIDTH]; + grant_wdata_c = pending_wdata[gi*DATA_WIDTH +: DATA_WIDTH]; + end + end + end + integer pi; always @(posedge clk) begin @@ -157,12 +190,14 @@ module slot_mem_arbiter_wide #( if (any_pending) begin owner <= grant_idx + 1'b1; m_req <= 1'b1; - m_wr <= pending_wr[grant_idx]; - m_lb_n <= pending_lb_n[grant_idx]; - m_ub_n <= pending_ub_n[grant_idx]; - m_addr <= pending_addr[grant_idx*ADDR_WIDTH +: ADDR_WIDTH]; - m_wdata <= pending_wdata[grant_idx*DATA_WIDTH +: DATA_WIDTH]; - pending[grant_idx] <= 1'b0; + m_wr <= grant_wr_c; + m_lb_n <= grant_lb_n_c; + m_ub_n <= grant_ub_n_c; + m_addr <= grant_addr_c; + m_wdata <= grant_wdata_c; + for (pi = 0; pi < N_PORTS; pi = pi + 1) begin + if (grant_idx == pi[PIDXW-1:0]) pending[pi] <= 1'b0; + end end end else begin if (m_ready) begin