From ccaf3ee059c4e8fa1d51a928c9e1a229fd1bf431 Mon Sep 17 00:00:00 2001 From: manvalan Date: Sun, 20 Sep 2026 19:18:57 +0200 Subject: [PATCH] feat: real result-writeback engine, removes the last hard N-scaling pin blocker (EXP-0088) Adds result_writeback.v, one instance per packed_slot.v, writing each completed job's result directly into DDR3 at the job's own result_addr_a/b instead of driving literal top-level pins -- the same architectural shape as the weight-fetch path, in reverse. job_done now means "durably in DDR3", not "captured in a register only a pin could see". n2_system_ddr3_top.v's own s0_result_data_a/b, s1_result_data_a/b top-level package pins are removed (and the now-dangling XDC constraint for them), closing the real, hard scaling blocker docs/ARCHITECTURE_ ANALYSIS.md flagged since EXP-0074/0079 (8 bits x 2 lanes x N cores -> 256 pins at N=16). Addressing reuses the exact same JOB_ADDR_WIDTH->ctrl-bus-word truncation x_base_a/w_base already use (verified against act_tile_ fetch.v's/layer_prefetch_ctrl.v's own real code, not guessed). The host reads results back via the already-existing READ_MEM (0x02) SPI opcode -- no new protocol. A real EXP-0066-class bug (issuing ctrl_req before mem_grant) was caught and fixed before ever compiling, by re-deriving the design against act_tile_fetch.v's own proven S_MEMWAIT/S_GAP sequencing. Verified two ways: tb_packed_slot.v extended with a real DDR3 read-after-write check (9/9 PASS, confirms the write actually landed, not just that job_done pulsed); tb_n2_system_ddr3.v re-run via real xsim to confirm correct behavior under real 2-slot shared-bus arbitration (8/8 PASS, 0 errors, consistent timing with EXP-0087's own baseline for this workload). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MUG92aM9m68TRc4rG55BcC --- docs/ARCHITECTURE_ANALYSIS.md | 86 ++++++-- docs/PHYSICAL_REALIZATION.md | 18 +- hardware/v2/logs/experiments.log | 105 ++++++++++ .../v3/constraints/n2_system_ddr3_top.xdc | 19 +- hardware/v3/rtl/n2_system_ddr3_top.v | 24 ++- hardware/v3/rtl/packed_slot.v | 88 ++++++-- hardware/v3/rtl/result_writeback.v | 192 ++++++++++++++++++ hardware/v3/sim/tb_packed_slot.v | 53 +++++ 8 files changed, 534 insertions(+), 51 deletions(-) create mode 100644 hardware/v3/rtl/result_writeback.v diff --git a/docs/ARCHITECTURE_ANALYSIS.md b/docs/ARCHITECTURE_ANALYSIS.md index d8be91b..9d8dd52 100644 --- a/docs/ARCHITECTURE_ANALYSIS.md +++ b/docs/ARCHITECTURE_ANALYSIS.md @@ -267,16 +267,23 @@ Real, verified, low resource cost, not on any critical performance path (host commands are inherently much slower than the internal compute/memory loop). No bottleneck here. Not a scaling concern. -### 4.6 Missing: result-writeback engine +### 4.6 [DONE, EXP-0088] Result-writeback engine -Still genuinely absent (disclosed since `packed_slot.v`'s own original -header, unchanged through EXP-0079). Currently `result_data_a/b` are literal -top-level pins — functional at N=2 (32 pins), but this is the **exact same -class of mistake already caught once** for activation data (EXP-0074: ~360 -pins nearly exceeded the whole package's I/O budget). At N=16 this port -alone would need 8 bits × 2 lanes × 16 cores = 256 pins — **a real, hard -blocker for any scaling beyond a handful of cores**, independent of the -memory-bandwidth ceiling in §3. Recommended fix in §5.3. +**No longer absent.** `result_writeback.v` (new module) is instantiated +inside each `packed_slot.v`, sharing that slot's own ctrl port with +`layer_prefetch_ctrl.v`/`ddr_prefetch_mgr.v` exactly the way those two +already share it with each other (mutually exclusive in time by FSM +construction — writeback only starts in `S_RESULT`, strictly after the +tile loop has finished). Each completed job's result is written directly +into DDR3 at the job's own `result_addr_a`/`result_addr_b` — the SAME +architectural shape as the weight-fetch path, in reverse, exactly as this +section used to recommend before it was built. `job_done` now means "the +result is durably in DDR3", not merely "captured in a register only a +literal top-level pin could see". `n2_system_ddr3_top.v`'s own +`s0_result_data_a/b`/`s1_result_data_a/b` top-level PACKAGE PINS are +**removed** — the real, hard N-scaling blocker this section used to flag +(8 bits × 2 lanes × N cores → 256 pins at N=16) no longer exists at any N. +See §5.3 for the full real design/verification story. --- @@ -433,13 +440,62 @@ exists in this codebase; kept for when it's revisited): reorders/never dispatches that queued job) also wastes real bandwidth — needs a real cancellation/staleness mechanism, not assumed away. -### 5.3 [Blocking for any real scaling] Result-writeback engine +### 5.3 [DONE, EXP-0088] Result-writeback engine -Must exist before N>2 is even attemptable (§4.6) — result data needs to go -into DDR3 (or through the SPI status/register path for small result sets), -never as N-scaled literal top-level pins again. Same architectural shape as -the weight-fetch path, in reverse (write instead of read) — a reasonable, -bounded scope, and a real prerequisite, not optional polish. +**Built and real-verified.** New module `result_writeback.v`, one instance +per `packed_slot.v` (matching how `layer_prefetch_ctrl.v`/`act_tile_ +fetch.v`/`ddr_prefetch_mgr.v` are already one-per-slot, not a new +arbiter-requester count as N scales). On job completion (`S_RESULT`), it +writes BOTH lanes' results into DDR3 at the job's own `result_addr_a`/ +`result_addr_b` and only THEN asserts `job_done` — real, correctness-first +sequencing (`job_done` now means "durably in DDR3", not "captured in a +register" the way it used to). + +**Real addressing** (verified against `act_tile_fetch.v`'s/`layer_ +prefetch_ctrl.v`'s own real address-computation code, not guessed): +`result_addr_a/b` arrive in `packed_slot.v`'s own `JOB_ADDR_WIDTH=26`-bit +convention; the low `ADDR_WIDTH=25` bits (dropping the unused top/MSB +headroom bit) are used directly as a ctrl-bus-native 32-bit-word address +— the exact same address space `x_base_a`/`w_base` already live in. One +full 32-bit ctrl-word is written per lane: `{node_id[15:0], 8'h00, +result_data[7:0]}`. The host reads results back via the ALREADY-EXISTING +`READ_MEM` (0x02) SPI opcode — no new protocol. Real, disclosed +host-firmware implication (not yet built, same class of gap as this +project's other disclosed firmware work, e.g. JTAG bit-banging): reading +a result back needs `mem_addr = result_addr[24:0]*2` for the value and +`+1` for node_id (2 host reads per lane), since `READ_MEM`'s own +`mem_addr` is 16-bit-word-granular while this engine writes a native +32-bit ctrl-word — the same real halving `host_mem_bridge.v`'s own header +already discloses for the debug raw-access path. + +**Shared-bus discipline** (mirrors `act_tile_fetch.v`'s own real, +proven pattern, not reinvented): waits for `mem_grant` before ever +issuing `ctrl_req` (EXP-0066's own established rule — an early/blind +request on a shared bus can lose the request permanently); a real +`S_GAP` state waits for `!ctrl_busy` between lane A's write and lane +B's own, since `mig_native_adapter.v`'s own `busy` stays asserted one +cycle past `ctrl_ready`. `wmask` polarity matches `host_mem_bridge.v`'s +own real, already-working convention exactly (0 = write this byte, 1 = +masked, the same DQM-style polarity this project's whole memory stack +already uses end to end). + +**Real, hard scaling blocker removed**: `n2_system_ddr3_top.v`'s own +`s0_result_data_a/b`/`s1_result_data_a/b` top-level PACKAGE PINS are +gone — each slot's `packed_slot.v` still exposes `result_data_a/b` etc. +as plain output ports (for debug/testbench visibility, unchanged), but +these are no longer wired to literal FPGA package pins at any N. + +**Verification**: `tb_packed_slot.v` extended with a new real +read-after-write check (`verify_writeback` task) — after each job's +`job_done`, the testbench independently reads back the exact DDR3 +location `result_writeback.v` should have written and confirms both the +result value AND node_id match, closing the loop (not just checking +`job_done` eventually pulses). **9/9 PASS, 0 errors**, real Icarus xsim +against `burst_mem_model32.v`. Also re-verified at the full N=2 system +level (`tb_n2_system_ddr3.v`, real xsim against the real, closed +32-bit/3225ps DDR3 model) to confirm the writeback engine behaves +correctly under real shared-bus arbitration contention between 2 slots +— see `hardware/v2/logs/experiments.log` EXP-0088 for the real result. ### 5.4 [DONE] Widening the physical DDR3 channel: 32-bit single channel vs. a second independent 16-bit channel — **functionally complete AND real timing closed (EXP-0084 → EXP-0086)** diff --git a/docs/PHYSICAL_REALIZATION.md b/docs/PHYSICAL_REALIZATION.md index 0264cbf..fe70f67 100644 --- a/docs/PHYSICAL_REALIZATION.md +++ b/docs/PHYSICAL_REALIZATION.md @@ -363,10 +363,11 @@ when it actually fires, instead of polling every loop iteration. ## 7. Known-open items (honestly disclosed, not hidden) - Scaling past N=2 compute cores (silicon budget allows up to ~30 per the - DSP48E1 count) is not yet built or timing-verified. A result-writeback - engine (currently `result_data_a/b` are literal top-level pins, fine at - N=2 but a real pin-budget blocker beyond that) must land first — see - `docs/ARCHITECTURE_ANALYSIS.md` §5.3. + DSP48E1 count) is not yet built or timing-verified. The real pin-budget + blocker that used to require this (`result_data_a/b` as literal + top-level pins) is now REMOVED (EXP-0088, `result_writeback.v` — see + `docs/ARCHITECTURE_ANALYSIS.md` §4.6/§5.3) — real N=2/4/8/16 P&R + scaling tests are the next real milestone, no longer blocked. - `sys_rst` has only a *tentative* real pin (G13, bank 15, EXP-0084 — chosen just to unblock real P&R, not a final board decision) — assign its real, permanent location once the rest of the board layout (reset @@ -388,7 +389,8 @@ when it actually fires, instead of polling every loop iteration. `tb_spi_host_bridge_v3.v`), AND real P&R-verified at the closed EXP-0086 signoff (D14/LVCMOS33, confirmed via a direct query on the routed checkpoint) — see §6. -- Scaling past N=2 real core count and the result-writeback engine (both - listed above) remain the real next milestones now that the 32-bit - channel's own timing is closed — see - `hardware/v2/logs/experiments.log` EXP-0086's `next_action`. +- `result_writeback.v` (EXP-0088) is real, built, and verified (9/9 PASS + including a new real DDR3 read-after-write check, `tb_packed_slot.v`). + Real P&R re-verification for this specific addition is the next real + step, together with the N=2/4/8/16 core-count scaling tests it + unblocks — see `docs/ARCHITECTURE_ANALYSIS.md` §5.3/§5.5. diff --git a/hardware/v2/logs/experiments.log b/hardware/v2/logs/experiments.log index 518263e..2b44804 100644 --- a/hardware/v2/logs/experiments.log +++ b/hardware/v2/logs/experiments.log @@ -5813,3 +5813,108 @@ now-diminished DDR3-latency lever this experiment just showed has little room left to give at N=2 -- worth real-measuring its own benefit carefully before investing further RTL effort, rather than assuming EXP-0083's original optimistic framing still applies. + +EXP-0088 -- real result-writeback engine: the last hard N-scaling +blocker removed (2026-09-20, user's own explicit reprioritization: +"riordiniamo le priorita ... BRAM ci pensiamo dopo. Fai la parte +realmente mancante prima, il RESULT-WRITEBACK e poi implementa la 4x4 +sistolica") + +CONTEXT: docs/ARCHITECTURE_ANALYSIS.md S4.6/S5.3 flagged this since +packed_slot.v's own original header (unchanged through EXP-0079): +result_data_a/b/result_node_id_a/b were literal top-level PACKAGE PINS +on n2_system_ddr3_top.v (s0_result_data_a/b, s1_result_data_a/b) -- +fine at N=2 (4 pins), the exact same class of scaling mistake already +caught once for activation data (EXP-0074: ~360 pins nearly exceeded +the whole package's I/O budget) -- at N=16 this port alone would need +8 bits x 2 lanes x 16 cores = 256 pins, a real, hard blocker. + +DESIGN: new module `result_writeback.v`, one instance per +`packed_slot.v` (matching how layer_prefetch_ctrl.v/act_tile_fetch.v/ +ddr_prefetch_mgr.v are already one-per-slot, NOT a new arbiter- +requester count as N scales -- each slot still contributes exactly one +ctrl_req to the shared arbiter, now locally 3-way-muxed instead of 2). +On job completion (S_RESULT), packed_slot.v pulses `wb_start`; the new +S_WRITEBACK state holds `job_done` back until `wb_done` fires -- job_done +now means "the result is durably in DDR3", not "captured in a register +only a literal top-level pin could see". + +REAL ADDRESSING (verified against act_tile_fetch.v's/layer_prefetch_ +ctrl.v's own real address-computation code, not guessed, since getting +this wrong would be a silent correctness bug, not just a performance +one): result_addr_a/b arrive in packed_slot.v's own JOB_ADDR_WIDTH=26-bit +convention. The low ADDR_WIDTH=25 bits (dropping the unused top/MSB +headroom bit) are used DIRECTLY as a ctrl-bus-native 32-bit-word +address -- the exact same address space x_base_a/w_base already live +in (confirmed: `x_base_a_lat[ADDR_WIDTH-2:0]` feeds act_tile_fetch.v's +own ctrl_addr computation directly, same truncation). ONE full 32-bit +ctrl-word is written per lane: {node_id[15:0], 8'h00, result_data[7:0]} +(low 16 bits = zero-extended 8-bit result value, high 16 bits = +node_id). The host reads results back via the ALREADY-EXISTING +READ_MEM (0x02) SPI opcode -- no new protocol needed. Real, disclosed +host-firmware implication (not yet built, same class of gap as this +project's other disclosed firmware work, e.g. JTAG bit-banging): +reading a result back needs `mem_addr = result_addr[24:0]*2` for the +value and `mem_addr = result_addr[24:0]*2 + 1` for node_id (2 host +reads per lane), since READ_MEM's own mem_addr is 16-bit-word-granular +while this engine writes a native 32-bit ctrl-word -- the same real +halving host_mem_bridge.v's own header already discloses for the debug +raw-access path (EXP-0084). + +SHARED-BUS DISCIPLINE (mirrors act_tile_fetch.v's own real, proven +pattern, not reinvented -- a REAL bug was caught and fixed during +design, not just asserted correct): the first draft omitted `mem_grant` +entirely and issued ctrl_req unconditionally, exactly the class of bug +EXP-0066 already documented (an early/blind request on a shared, +arbitrated bus can lose the request permanently) -- caught by re- +deriving the design against act_tile_fetch.v's own real S_MEMWAIT +sequencing before ever compiling it, not found by simulation. Fixed: +real S_MEMWAIT (wait for mem_grant before issuing ctrl_req) and a real +S_GAP state (wait for !ctrl_busy between lane A's write and lane B's +own, since mig_native_adapter.v's own busy stays asserted one cycle +past ctrl_ready -- act_tile_fetch.v's own header already established +this). wmask polarity matches host_mem_bridge.v's own real, already- +working convention exactly (0 = write this byte, 1 = masked). + +REAL SCALING FIX: n2_system_ddr3_top.v's own s0_result_data_a/b, +s1_result_data_a/b top-level PACKAGE PINS are REMOVED (and the +now-dangling XDC IOSTANDARD constraint for them removed too) -- +packed_slot.v still exposes result_data_a/b/etc. as plain output ports +(unchanged, for debug/testbench visibility), but nothing wires them to +literal FPGA pins any more, at any N. + +VERIFICATION (two levels, same discipline as every other real change +in this project): + 1. tb_packed_slot.v -- extended with a new real read-after-write + check (`verify_writeback` task): after each job's job_done, the + testbench independently reads back the EXACT DDR3 location + result_writeback.v should have written (via burst_mem_model32.v) + and confirms BOTH the result value AND node_id match -- not just + that job_done eventually pulsed. **9/9 PASS, 0 errors**, real + Icarus xsim (iverilog -g2012). + 2. tb_n2_system_ddr3.v -- real xsim (Vivado, real ddr3_model.sv, + real mig_7series_0_mig, real 2-slot sdram_arbiter_n.v contention) + confirms the writeback engine behaves correctly under REAL shared- + bus arbitration between 2 slots, not just in isolation. **8/8 + PASS, 0 errors, 8/8 positions completed**, $finish at + 101204.9335 ns -- consistent with EXP-0087's own real ~100.6-100.7us + baseline for this same N=2/8-position workload (writeback adds a + small, real, expected overhead, not a regression). + Two real setup bugs found and fixed getting this run to compile: + result_writeback.v (a brand-new file) needed to be added to the + Vivado project's own `sources_1` fileset as a direct reference + (not just the xsim-only sim_1 fileset) -- confirmed via TCL query + it landed as a direct reference, not an imported copy, avoiding + the stale-import class of bug from the start. + +DECISION: keep, real, verified, closes the last hard N-scaling blocker +this project's own docs had flagged since EXP-0074/0079. Real P&R +re-verification for this specific addition is the next real step +(deferred, together with the N=2/4/8/16 scaling tests it directly +unblocks, per the user's own next directive). + +next_action: real P&R signoff for this change (confirm it doesn't +disturb the closed EXP-0086 timing), then real N=2/4/8/16 core-count +scaling tests (each with its own real P&R signoff, per the user's own +standing directive), then the 4x4 hybrid systolic architecture +(docs/ARCHITECTURE_ANALYSIS.md S5.6, currently exploratory/not built). diff --git a/hardware/v3/constraints/n2_system_ddr3_top.xdc b/hardware/v3/constraints/n2_system_ddr3_top.xdc index 3fd13c8..82ac8b0 100644 --- a/hardware/v3/constraints/n2_system_ddr3_top.xdc +++ b/hardware/v3/constraints/n2_system_ddr3_top.xdc @@ -52,17 +52,20 @@ set_property IOSTANDARD LVCMOS33 [get_ports flash_cs_n] set_property PROHIBIT true [get_sites -of_objects [get_package_pins {L16 R16 V15}]] # ---- EXP-0084: remaining top-level ports with no fixed board LOC yet -# (result-data debug pins, status signals) default to LVCMOS18 with no -# explicit IOSTANDARD set -- real place_design found this real, -# concrete: banks 14/15/34/35 are ALL already committed to other real -# voltages (2.5V/3.3V/1.5V/1.5V), leaving only bank 16's spare pins as -# LVCMOS18-compatible, and there aren't enough of them (40 ports vs 10 -# pins). Assign these explicitly to LVCMOS33 so they place in bank -# 15's own real spare capacity (46 free pins) instead -- a real, +# (status signals) default to LVCMOS18 with no explicit IOSTANDARD set +# -- real place_design found this real, concrete: banks 14/15/34/35 are +# ALL already committed to other real voltages (2.5V/3.3V/1.5V/1.5V), +# leaving only bank 16's spare pins as LVCMOS18-compatible, and there +# aren't enough of them. Assign these explicitly to LVCMOS33 so they +# place in bank 15's own real spare capacity instead -- a real, # necessary fix, not a workaround; a permanent board LOC for each # should still be assigned once the rest of the board layout is # decided (S7 of docs/PHYSICAL_REALIZATION.md). -set_property IOSTANDARD LVCMOS33 [get_ports {s0_result_data_a[*] s0_result_data_b[*] s1_result_data_a[*] s1_result_data_b[*]}] +# EXP-0088: the s0_result_data_a/b, s1_result_data_a/b ports this +# constraint used to also cover no longer exist as top-level ports -- +# each slot now writes its own result directly into DDR3 via its own +# internal result_writeback.v (see packed_slot.v's own header) instead +# of exposing literal, N-scaled result pins. set_property IOSTANDARD LVCMOS33 [get_ports {job_out_slot[*] job_out_done init_calib_complete ui_clk_o}] # ---- neural-processor management SPI (-> spi_host_bridge_v3.v): diff --git a/hardware/v3/rtl/n2_system_ddr3_top.v b/hardware/v3/rtl/n2_system_ddr3_top.v index 3b1f670..3c6c286 100644 --- a/hardware/v3/rtl/n2_system_ddr3_top.v +++ b/hardware/v3/rtl/n2_system_ddr3_top.v @@ -98,13 +98,17 @@ module n2_system_ddr3_top #( output wire flash_mosi, input wire flash_miso, - // ---- results (small enough to keep as real top-level pins for - // observation; NOT part of the activation-interface pin-count - // problem described below) ---- - output wire signed [DATA_WIDTH-1:0] s0_result_data_a, - output wire signed [DATA_WIDTH-1:0] s0_result_data_b, - output wire signed [DATA_WIDTH-1:0] s1_result_data_a, - output wire signed [DATA_WIDTH-1:0] s1_result_data_b, + // ---- results: EXP-0088 REMOVES the literal per-slot top-level + // result pins that used to live here (s0_result_data_a/b, + // s1_result_data_a/b) -- they were flagged (docs/ARCHITECTURE_ + // ANALYSIS.md S4.6/S5.3) as the exact same class of scaling mistake + // already caught once for activation data (EXP-0074): fine at N=2 + // (4 pins), a hard blocker at N=16 (8 bits x 2 lanes x 16 cores = + // 256 pins on this port alone). Each packed_slot.v instance now + // writes its own result directly into DDR3 via its own internal + // result_writeback.v (see packed_slot.v's own header) -- the host + // reads results back via the already-existing READ_MEM (0x02) SPI + // opcode, no new top-level port needed at any N. // ---- status ---- output wire ui_clk_o, @@ -283,6 +287,12 @@ module n2_system_ddr3_top #( wire [15:0] s0_nid_a, s0_nid_b, s1_nid_a, s1_nid_b; wire [JOB_ADDR_WIDTH-1:0] s0_raddr_a, s0_raddr_b, s1_raddr_a, s1_raddr_b; + // EXP-0088: plain internal debug wires now (no longer top-level + // pins) -- the real result is written to DDR3 by each slot's own + // internal result_writeback.v; these remain wired from packed_ + // slot.v's own output ports purely for internal observability. + wire signed [DATA_WIDTH-1:0] s0_result_data_a, s0_result_data_b; + wire signed [DATA_WIDTH-1:0] s1_result_data_a, s1_result_data_b; // ---- activation fetch: REAL now (EXP-0079) -- each packed_slot // instance owns its own act_tile_fetch.v internally, sharing that diff --git a/hardware/v3/rtl/packed_slot.v b/hardware/v3/rtl/packed_slot.v index 8155af6..e6a0b2e 100644 --- a/hardware/v3/rtl/packed_slot.v +++ b/hardware/v3/rtl/packed_slot.v @@ -40,9 +40,16 @@ // word burst (even index low 64 bits, odd index high 64 bits) -- see // act_tile_fetch.v's own header and docs/PHYSICAL_REALIZATION.md S4. // -// Also disclosed: no result-writeback engine exists yet either -- -// result_addr_a/b are passed through unused, for a future writeback -// stage to consume. +// RESULT WRITEBACK (EXP-0088, real, closes the gap this header used to +// disclose as deferred): result_writeback.v (u_wb) writes each +// completed job's result INTO DDR3 at result_addr_a/b, sharing this +// slot's own ctrl port the same way u_pf/u_ddrpf already do (mutually +// exclusive in time -- writeback only ever starts in S_RESULT, after +// the whole tile loop has finished). This was the last real blocker +// for scaling past a handful of slots: result_data_a/b/etc. remain +// real output ports here for debug/testbench visibility, but the +// TOP-LEVEL n2_system_ddr3_top.v no longer exposes them as literal, +// N-scaled package pins -- see result_writeback.v's own header. // // EVERY job re-fetches its layer from SDRAM (no resident-weight-skip // optimization) -- correctness first; EXP-0057's own measured @@ -122,7 +129,8 @@ module packed_slot #( S_TILEWAIT = 4'd6, S_OPERAND = 4'd7, S_RESULT = 4'd8, - S_DONE = 4'd9; + S_DONE = 4'd9, + S_WRITEBACK = 4'd10; reg [3:0] state; reg [ADDR_WIDTH-1:0] w_base_lat, x_base_a_lat, x_base_b_lat; @@ -185,16 +193,50 @@ module packed_slot #( .ctrl_rdata(ctrl_rdata), .ctrl_ready(ctrl_ready), .ctrl_busy(ctrl_busy) ); + // ---- result_writeback.v (EXP-0088): real result-writeback engine, + // shares this slot's own ctrl port with u_pf/u_ddrpf above + // (mutually exclusive in time by FSM construction -- writeback only + // ever starts in S_RESULT, strictly after the whole tile loop that + // drives u_ddrpf has already finished). Writes both lanes' results + // into DDR3 at their own job-supplied result_addr_a/b -- see + // result_writeback.v's own header for the real addressing/format. + reg wb_start; + wire wb_busy, wb_done; + wire wb_mem_active; + + wire wb_ctrl_req, wb_ctrl_wr; + wire [ADDR_WIDTH-2:0] wb_ctrl_addr; + wire [32*BURST_LEN-1:0] wb_ctrl_wdata; + wire [4*BURST_LEN-1:0] wb_ctrl_wmask; + + result_writeback #( + .BURST_LEN(BURST_LEN), .DATA_WIDTH(DATA_WIDTH), + .JOB_ADDR_WIDTH(ADDR_WIDTH), .ADDR_WIDTH(ADDR_WIDTH-1) + ) u_wb ( + .clk(clk), .rst(rst), + .start(wb_start), + .result_addr_a(result_addr_a_lat), .result_addr_b(result_addr_b_lat), + .result_data_a(result_data_a), .result_data_b(result_data_b), + .result_node_id_a(result_node_id_a), .result_node_id_b(result_node_id_b), + .busy(wb_busy), .done(wb_done), + .mem_active(wb_mem_active), .mem_grant(mem_grant), + .ctrl_req(wb_ctrl_req), .ctrl_wr(wb_ctrl_wr), .ctrl_addr(wb_ctrl_addr), + .ctrl_wdata(wb_ctrl_wdata), .ctrl_wmask(wb_ctrl_wmask), + .ctrl_rdata(ctrl_rdata), .ctrl_ready(ctrl_ready), .ctrl_busy(ctrl_busy) + ); + // mutually exclusive by FSM construction (weight prefetch always // fully completes, incl. consume_done, before the tile loop that - // triggers ddrpf_job_start ever fires) -- safe to select on act_mem_active alone. - assign ctrl_req = act_mem_active ? act_ctrl_req : pf_ctrl_req; - assign ctrl_wr = act_mem_active ? act_ctrl_wr : pf_ctrl_wr; - assign ctrl_addr = act_mem_active ? act_ctrl_addr : pf_ctrl_addr; - assign ctrl_wdata = act_mem_active ? act_ctrl_wdata : pf_ctrl_wdata; - assign ctrl_wmask = act_mem_active ? act_ctrl_wmask : pf_ctrl_wmask; + // triggers ddrpf_job_start ever fires; writeback only ever starts + // in S_RESULT, strictly after that same tile loop has already + // finished) -- safe to select on act_mem_active/wb_mem_active alone. + assign ctrl_req = act_mem_active ? act_ctrl_req : (wb_mem_active ? wb_ctrl_req : pf_ctrl_req); + assign ctrl_wr = act_mem_active ? act_ctrl_wr : (wb_mem_active ? wb_ctrl_wr : pf_ctrl_wr); + assign ctrl_addr = act_mem_active ? act_ctrl_addr : (wb_mem_active ? wb_ctrl_addr : pf_ctrl_addr); + assign ctrl_wdata = act_mem_active ? act_ctrl_wdata : (wb_mem_active ? wb_ctrl_wdata : pf_ctrl_wdata); + assign ctrl_wmask = act_mem_active ? act_ctrl_wmask : (wb_mem_active ? wb_ctrl_wmask : pf_ctrl_wmask); - assign mem_active = (state == S_MEMWAIT) || (state == S_PREFETCH) || act_mem_active; + assign mem_active = (state == S_MEMWAIT) || (state == S_PREFETCH) || act_mem_active || wb_mem_active; // ---- layer_weight_buffer.v ---- wire [BUFADDRW-1:0] lwb_rd_addr; @@ -277,6 +319,7 @@ module packed_slot #( operand_valid<= 1'b0; tile_last <= 1'b0; result_ready <= 1'b0; + wb_start <= 1'b0; job_bias <= {DATA_WIDTH{1'b0}}; job_activation <= ACT_RELU; tcnt <= 16'd0; @@ -287,6 +330,7 @@ module packed_slot #( tile_req <= 1'b0; ddrpf_job_start <= 1'b0; ddrpf_tile_consume <= 1'b0; + wb_start <= 1'b0; case (state) S_IDLE: begin @@ -384,6 +428,17 @@ module packed_slot #( end end + // EXP-0088: result_data_a/b/result_node_id_a/b/ + // result_addr_a_out/b_out remain real output ports + // (unchanged, still updated exactly as before -- kept + // for debug/testbench visibility), but job_done is now + // held back until the real DDR3 writeback itself + // completes (S_WRITEBACK), not just the moment + // result_valid_np fires -- the whole point of this + // module's own EXP-0088 upgrade is that job_done means + // "the result is durably in DDR3", not merely "captured + // in a register only this slot's own top-level pins + // could see". S_RESULT: begin if (result_valid_np) begin result_data_a <= result_data_a_np; @@ -393,8 +448,15 @@ module packed_slot #( result_addr_a_out <= result_addr_a_lat; result_addr_b_out <= result_addr_b_lat; result_ready <= 1'b0; - job_done <= 1'b1; - state <= S_IDLE; + wb_start <= 1'b1; + state <= S_WRITEBACK; + end + end + + S_WRITEBACK: begin + if (wb_done) begin + job_done <= 1'b1; + state <= S_IDLE; end end diff --git a/hardware/v3/rtl/result_writeback.v b/hardware/v3/rtl/result_writeback.v new file mode 100644 index 0000000..c30f902 --- /dev/null +++ b/hardware/v3/rtl/result_writeback.v @@ -0,0 +1,192 @@ +`timescale 1ns/1ps + +// ============================================================ +// V3 -- result_writeback.v: real result-writeback engine, closing the +// gap disclosed since packed_slot.v's own original header ("no +// result-writeback engine exists yet either -- result_addr_a/b are +// passed through unused") and flagged as a hard scaling blocker +// (docs/ARCHITECTURE_ANALYSIS.md S4.6/S5.3): literal top-level +// result_data_a/b pins do not scale past a handful of cores (8 bits x +// 2 lanes x N cores -- at N=16 that's 256 pins on this port alone). +// +// REAL FIX: write each completed job's result INTO DDR3 at the job's +// own result_addr_a/result_addr_b (already carried through packed_ +// slot.v's own interface, previously unused), reusing the SAME shared +// ctrl port packed_slot.v already time-multiplexes among its other +// sub-engines (layer_prefetch_ctrl.v / ddr_prefetch_mgr.v) -- same +// architectural shape as the weight-fetch path, in reverse. The host +// reads results back via the ALREADY-EXISTING READ_MEM (0x02) opcode +// -- no new SPI protocol needed. +// +// REAL ADDRESSING (verified against act_tile_fetch.v's/layer_ +// prefetch_ctrl.v's own real address-computation code, not guessed): +// result_addr_a/b arrive in packed_slot.v's own JOB_ADDR_WIDTH=26-bit +// convention. Exactly like x_base_a/w_base already do, the LOW +// ADDR_WIDTH=25 bits (dropping the unused top/MSB headroom bit) are +// used DIRECTLY as a ctrl-bus-native 32-bit-word address -- the SAME +// address space act_tile_fetch.v's own ctrl_addr already lives in. +// ONE full 32-bit ctrl-word is written per lane: +// {node_id[15:0], 8'h00, result_data[7:0]} (low 16 bits = +// zero-extended 8-bit result value, high 16 bits = node_id). +// +// REAL, DISCLOSED HOST-FIRMWARE IMPLICATION (not yet built, same as +// this project's other disclosed host-firmware gaps, e.g. JTAG +// bit-banging): reading a written result back via the EXISTING +// READ_MEM (16-bit-word-addressed) opcode needs +// `mem_addr = result_addr[24:0]*2` for the value and +// `mem_addr = result_addr[24:0]*2 + 1` for node_id (2 host reads per +// lane, since READ_MEM's own mem_addr is 16-bit-word-granular while +// this engine writes a native 32-bit ctrl-word -- see host_mem_ +// bridge.v's own header for the real reason that halving exists). +// +// WMASK CONVENTION (matches host_mem_bridge.v's own real, already- +// working pattern exactly, not reinvented): 0 = write this byte, 1 = +// masked -- the same DQM-style polarity this project's whole memory +// stack already uses end to end. +// +// TWO LANES, ONE TRANSACTION EACH, SEQUENTIAL: lane A's write +// completes fully (through its own ctrl_ready) before lane B's own +// starts -- mirrors act_tile_fetch.v's own "lane A then lane B" +// sequencing for its two burst reads, the same discipline already +// proven safe on this shared bus. +// ============================================================ +module result_writeback #( + parameter BURST_LEN = 8, + parameter DATA_WIDTH = 8, + parameter JOB_ADDR_WIDTH = 26, + parameter ADDR_WIDTH = 25 // ctrl-bus-native word address, matches sdram_arbiter_n.v's own convention +)( + input wire clk, + input wire rst, + + // one-shot request: pulse `start` with all fields valid the same + // cycle (matches this project's own established one-shot-pulse- + // requester discipline, EXP-0066). + input wire start, + input wire [JOB_ADDR_WIDTH-1:0] result_addr_a, + input wire [JOB_ADDR_WIDTH-1:0] result_addr_b, + input wire signed [DATA_WIDTH-1:0] result_data_a, + input wire signed [DATA_WIDTH-1:0] result_data_b, + input wire [15:0] result_node_id_a, + input wire [15:0] result_node_id_b, + output wire busy, + output reg done, // one-cycle pulse + + // ---- shared ctrl port (packed_slot.v's own local mux gates this + // the same way it already gates pf_ctrl_*/act_ctrl_*) ---- + output wire mem_active, + input wire mem_grant, + + output reg ctrl_req, + output reg ctrl_wr, + output reg [ADDR_WIDTH-1:0] ctrl_addr, + output reg [32*BURST_LEN-1:0] ctrl_wdata, + output reg [4*BURST_LEN-1:0] ctrl_wmask, + input wire [32*BURST_LEN-1:0] ctrl_rdata, + input wire ctrl_ready, + input wire ctrl_busy +); + localparam ALIGN_BITS = $clog2(BURST_LEN); // 3: which of the BURST_LEN 32-bit words in the burst + + localparam S_IDLE = 3'd0, + S_MEMWAIT = 3'd1, + S_XFER_A = 3'd2, + S_GAP = 3'd3, // wait for ctrl_busy to clear before firing lane B's request + S_XFER_B = 3'd4, + S_DONE = 3'd5; + + reg [2:0] state; + reg [DATA_WIDTH-1:0] data_a_lat, data_b_lat; + reg [15:0] nid_a_lat, nid_b_lat; + reg [ADDR_WIDTH-1:0] word_addr_a_lat, word_addr_b_lat; + + assign busy = (state != S_IDLE); + // real, established discipline (EXP-0066): mem_active must be + // visible to the arbiter the SAME cycle this module first wants + // the bus, i.e. as soon as it leaves S_IDLE -- not only once a + // transaction is actually in flight. + assign mem_active = (state != S_IDLE); + + // real ctrl-bus-native word address: low ADDR_WIDTH bits of the + // JOB_ADDR_WIDTH job address -- the exact same truncation act_ + // tile_fetch.v/layer_prefetch_ctrl.v already apply to x_base_a/ + // w_base (verified against their own real code, not guessed). + wire [ADDR_WIDTH-1:0] word_addr_a = result_addr_a[ADDR_WIDTH-1:0]; + wire [ADDR_WIDTH-1:0] word_addr_b = result_addr_b[ADDR_WIDTH-1:0]; + + always @(posedge clk) begin + if (rst) begin + state <= S_IDLE; + ctrl_req <= 1'b0; + ctrl_wr <= 1'b0; + done <= 1'b0; + end else begin + ctrl_req <= 1'b0; + done <= 1'b0; + + case (state) + S_IDLE: begin + if (start) begin + data_a_lat <= result_data_a; + data_b_lat <= result_data_b; + nid_a_lat <= result_node_id_a; + nid_b_lat <= result_node_id_b; + word_addr_a_lat <= word_addr_a; + word_addr_b_lat <= word_addr_b; + state <= S_MEMWAIT; + end + end + + // real, established discipline (EXP-0066): never issue + // ctrl_req before mem_grant is actually observed -- a + // blind/early ctrl_req on a shared, arbitrated bus can + // lose the request permanently. + S_MEMWAIT: begin + if (mem_grant) begin + ctrl_req <= 1'b1; + ctrl_wr <= 1'b1; + ctrl_addr <= {word_addr_a_lat[ADDR_WIDTH-1:ALIGN_BITS], {ALIGN_BITS{1'b0}}}; + ctrl_wdata <= {(BURST_LEN){nid_a_lat, 8'h00, data_a_lat}}; + ctrl_wmask <= ~({{(4*BURST_LEN-4){1'b0}}, 4'hF} << (word_addr_a_lat[ALIGN_BITS-1:0] * 4)); + state <= S_XFER_A; + end + end + + S_XFER_A: begin + if (ctrl_ready) begin + state <= S_GAP; + end + end + + // mig_native_adapter.v's own S_DONE state keeps `busy` + // asserted one cycle past ctrl_ready (act_tile_fetch.v's + // own header/code already established this) -- wait for + // !ctrl_busy before firing lane B's write, instead of + // assuming back-to-back is safe. + S_GAP: begin + if (!ctrl_busy) begin + ctrl_req <= 1'b1; + ctrl_wr <= 1'b1; + ctrl_addr <= {word_addr_b_lat[ADDR_WIDTH-1:ALIGN_BITS], {ALIGN_BITS{1'b0}}}; + ctrl_wdata <= {(BURST_LEN){nid_b_lat, 8'h00, data_b_lat}}; + ctrl_wmask <= ~({{(4*BURST_LEN-4){1'b0}}, 4'hF} << (word_addr_b_lat[ALIGN_BITS-1:0] * 4)); + state <= S_XFER_B; + end + end + + S_XFER_B: begin + if (ctrl_ready) begin + state <= S_DONE; + end + end + + S_DONE: begin + done <= 1'b1; + state <= S_IDLE; + end + + default: state <= S_IDLE; + endcase + end + end +endmodule diff --git a/hardware/v3/sim/tb_packed_slot.v b/hardware/v3/sim/tb_packed_slot.v index 822d41a..bf52f7c 100644 --- a/hardware/v3/sim/tb_packed_slot.v +++ b/hardware/v3/sim/tb_packed_slot.v @@ -179,6 +179,55 @@ module tb; reg signed [DATA_WIDTH-1:0] expected_a, expected_b; integer wd; + // EXP-0088: real read-after-write check that result_writeback.v + // (inside the DUT) actually landed the correct value in DDR3 at + // result_addr_a/b -- not just that job_done eventually pulsed. + // Format matches result_writeback.v's own header exactly: one + // 32-bit ctrl-word per lane, {node_id[15:0], 8'h00, + // result_data[7:0]}. + task automatic verify_writeback( + input integer li, input integer pos_a, input integer pos_b, + input [ADDR_WIDTH-1:0] raddr_a, input [ADDR_WIDTH-1:0] raddr_b, + input signed [DATA_WIDTH-1:0] exp_data_a, input signed [DATA_WIDTH-1:0] exp_data_b, + input [15:0] exp_nid_a, input [15:0] exp_nid_b + ); + reg [31:0] word_a, word_b; + reg [SDRAM_ADDR_WIDTH-1:0] burst_addr; + reg [2:0] word_in_block; + begin + pre_active = 1'b1; + + burst_addr = {raddr_a[SDRAM_ADDR_WIDTH-1:3], 3'b0}; + word_in_block = raddr_a[2:0]; + @(posedge clk); while (ctrl_busy) @(posedge clk); + wpre_req = 1'b1; wpre_wr = 1'b0; wpre_addr = burst_addr; + @(posedge clk); wpre_req = 1'b0; + while (!ctrl_ready) @(posedge clk); + word_a = ctrl_rdata[word_in_block*32 +: 32]; + + burst_addr = {raddr_b[SDRAM_ADDR_WIDTH-1:3], 3'b0}; + word_in_block = raddr_b[2:0]; + @(posedge clk); while (ctrl_busy) @(posedge clk); + wpre_req = 1'b1; wpre_wr = 1'b0; wpre_addr = burst_addr; + @(posedge clk); wpre_req = 1'b0; + while (!ctrl_ready) @(posedge clk); + word_b = ctrl_rdata[word_in_block*32 +: 32]; + + pre_active = 1'b0; + + if (word_a[7:0] !== exp_data_a || word_a[31:16] !== exp_nid_a) begin + $display("FAIL li=%0d pos_a=%0d: WRITEBACK readback mismatch lane A: word=%08h (data=%0d nid=%0d) expected data=%0d nid=%0d", + li, pos_a, word_a, $signed(word_a[7:0]), word_a[31:16], $signed(exp_data_a), exp_nid_a); + errors = errors + 1; + end + if (word_b[7:0] !== exp_data_b || word_b[31:16] !== exp_nid_b) begin + $display("FAIL li=%0d pos_b=%0d: WRITEBACK readback mismatch lane B: word=%08h (data=%0d nid=%0d) expected data=%0d nid=%0d", + li, pos_b, word_b, $signed(word_b[7:0]), word_b[31:16], $signed(exp_data_b), exp_nid_b); + errors = errors + 1; + end + end + endtask + task automatic run_one_pair(input integer li, input integer pos_a, input integer pos_b); begin tests = tests + 1; @@ -224,6 +273,10 @@ module tb; end else begin $display("PASS li=%0d pos_a=%0d pos_b=%0d: a=%0d b=%0d (packed_slot.v real sequencer)", li, pos_a, pos_b, $signed(result_data_a), $signed(result_data_b)); + // EXP-0088: real DDR3 read-after-write check -- job_done + // now means "written to DDR3", confirm it actually was. + verify_writeback(li, pos_a, pos_b, result_addr_a, result_addr_b, + expected_a, expected_b, node_id_a, node_id_b); end end endtask