diff --git a/CLAUDE.md b/CLAUDE.md index 664451e..bb03650 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -115,6 +115,44 @@ unmodified by v3, e.g. `layer_prefetch_ctrl.v`/`layer_weight_buffer.v`). registered/one-cycle-late grant silently loses the request forever (EXP-0066's own real bug, now a standing design rule for every arbiter/ requester pair in this project). +- **When PIPELINING/hierarchically staging an arbiter's own `req` + signal (not just its `active`/`grant`), the `req` pulse must be + latched STICKY across the pipeline boundary, not just registered + every cycle** (EXP-0094, `sdram_arbiter_hier.v`). A real one-shot + `ctrl_req` pulse (e.g. `act_tile_fetch.v`'s own S_MEMWAIT: `ctrl_req + <= 1'b1` for exactly one cycle) is captured fine by a SINGLE-level + arbiter (the winning requester's own grant and the physical + controller's readiness to capture it are the SAME decision, always + same-cycle). Once a SECOND arbitration level is added downstream + (e.g. a top-level arbiter deciding which of several leaf groups gets + the real shared port), a leaf's own LOCAL grant no longer guarantees + the top level is free to act on it that same cycle — if the pipeline + register between levels just does `top_req_r <= leaf_req` every + cycle, the transient one-shot pulse reverts to 0 before the top level + gets around to it (e.g. busy with a different leaf), silently losing + the request — same EXP-0066 lost-pulse class, newly exposed at the + hierarchy boundary. Fix: `pending_r <= (pending_r | leaf_req) & + ~dispatched;` (set on first pulse, clear only once the top level + confirms real dispatch) feeds the pipeline register instead of the + bare transient signal. `active`/addr/data fields don't need this (a + real requester holds `active` level, and stable addr/data, for its + whole transaction) — only the transient `req` pulse does. +- **A testbench helper that fires a one-shot `req` the same cycle as + `active`, unconditionally (without checking `grant` first), is only + safe for a flat, single-level, uncontended arbiter test** — copying + it verbatim into a test for a hierarchical/pipelined arbiter (EXP-0094) + can itself cause a spurious hang, unrelated to any real RTL bug. Real + requesters in this project (`act_tile_fetch.v` etc) already wait for + `grant` before firing `req` (S_MEMWAIT) — match that in any new + testbench helper, don't assume the naive same-cycle-fire pattern + generalizes. +- **Give every new testbench a real cycle-counted watchdog from the + start** (EXP-0094) — one testbench, copied from a simpler precedent + that didn't need one, had none; a real protocol bug spun Icarus + forever at ~99% CPU with zero output instead of failing cleanly. + Every other testbench in this project already has a `wd`-counted + watchdog inside its own completion-wait loop — don't skip it "just + this once." - **`xvlog`/`iverilog` need `-sv`/`-g2012`** respectively to accept SystemVerilog-only syntax (e.g. `'0`) even in a plain `.v` file — prefer just not using SV-only syntax in synthesizable RTL (Vivado's `synth_design` diff --git a/docs/ARCHITECTURE_ANALYSIS.md b/docs/ARCHITECTURE_ANALYSIS.md index 508d48f..28fc3d0 100644 --- a/docs/ARCHITECTURE_ANALYSIS.md +++ b/docs/ARCHITECTURE_ANALYSIS.md @@ -659,7 +659,7 @@ specifically to document where/how it breaks rather than to succeed): --- -### 5.6 [Functionally DONE, TIMING NOT YET CLOSED — EXP-0089/0090/0091/0092/0093] Hybrid systolic scaling: 4 groups × 4-PE weight-stationary chains +### 5.6 [Functionally DONE, TIMING SUBSTANTIALLY IMPROVED BUT NOT YET CLOSED — EXP-0089/0090/0091/0092/0093/0094] Hybrid systolic scaling: 4 groups × 4-PE weight-stationary chains Captured from a 2026-09-20 brainstorming session as a purely exploratory idea; the same day, per the user's own explicit reprioritization, the real @@ -732,17 +732,62 @@ hardware at the target clock. This does not invalidate the functional/ connectivity results; timing closure is a genuinely separate gate. N=2 (EXP-0088) remains the real, trustworthy, deployable signoff. -**Not yet done, real and disclosed, real options for closing timing** -(need a real decision on direction before committing more RTL effort): -(1) pipeline `sdram_arbiter_n.v`'s own req_wdata/req_addr mux by one -real cycle — adds one cycle of real arbitration latency per request, -likely acceptable given DDR3's own already-dominant real latency -(EXP-0087); probably the most direct fix, targets the exact real -critical path found above. (2) a real, hierarchical 2-level arbiter -(e.g. 4 groups' own 5-way sub-arbiters feeding one real 4-way top -arbiter) instead of one flat 20/21-way mux. (3) lower the real target -clock for the N=16 variant specifically (a real, unquantified -throughput tradeoff against N=2). See EXP-0093's own `next_action`. +- **Real, hierarchical 2-level arbiter (EXP-0094)**: `sdram_arbiter_ + hier.v`, reusing `sdram_arbiter_n.v` unmodified, twice — 4 real LEAF + instances (NUM_REQ=5, one per group, physically local to their own + PEs) + 1 real TOP instance (NUM_REQ=5: 4 groups' own pipelined output + + 1 host, host bypassed/unpipelined since it was never the real + bottleneck), with a real pipeline register stage between the two + levels — the direct, targeted fix for EXP-0093's own real, traced + route-delay-dominated critical path. Isolated verification (new + `tb_sdram_arbiter_hier.v`, real `burst_mem_model32.v`): **23/23 + PASS**, including cross-group and mixed host+group worst-case + contention. Two real bugs found and fixed via signal tracing before + a trustworthy result was possible: a testbench helper not waiting for + grant before firing req (fixed to match `act_tile_fetch.v`'s own real + S_MEMWAIT discipline), and a genuine RTL lost-pulse bug at the + leaf-to-top boundary (a transient one-shot request pulse could be + dropped if the top level was still busy with a different group — + fixed with a real, sticky per-group `pending_req_r` latch). Wired + into `n16_system_ddr3_top.v` as a drop-in replacement. +- **Real re-verification with the new arbiter**: functional xsim + (`tb_n16_system_ddr3.v`) still **32/32 PASS, 0 errors**. Real, full + P&R re-run: utilization essentially unchanged (128 DSP48E1/53.33%). + **Real timing: WNS improved from −0.913ns to −0.646ns, TNS from + −690.085ns to −97.541ns, failing endpoints from 3021 to 771** — a + real, substantial, measured improvement, confirming the arbiter was + correctly root-caused and fixed (the bottleneck demonstrably moved + elsewhere). **Timing is still not fully met.** + +**Real, honest current bottleneck (traced via the actual new worst +violated path, not guessed)**: the new critical path is inside +`neural_processor_packed.v`'s own DSP48E1 MAC datapath (`GEN_MAC_ +PACKED`→`prodb1_reg`), logic-dominated (79%), not route — a +pre-existing module, unchanged since N=2, where it closed with an +already razor-thin real margin (WNS=+0.099962ns, EXP-0088). Real, +coherent interpretation: N=16's real overall die utilization (31% LUT) +increases general placement congestion enough, on its own, to erode +that already-thin margin — a different, more diffuse problem than the +arbiter's own single structural bottleneck, without an equally obvious +single-point fix. + +**Current real status**: N=16 is **functionally correct (EXP-0092, +re-confirmed EXP-0094) with a real, substantially improved but still +NOT fully closed timing result (EXP-0094)** — honestly not yet ready +for real hardware at the target clock. N=2 (EXP-0088) remains the +real, trustworthy, deployable signoff. + +**Not yet done, real and disclosed, real options for closing the +remaining gap** (need a real decision on direction before committing +more RTL effort — `neural_processor_packed.v` is the compute core +shared by every real PE in this project): (1) real pipelining inside +`neural_processor_packed.v`'s own MAC datapath at the specific +`GEN_MAC_PACKED`/`prodb1_reg` boundary (fork-before-promote, N=2's own +signoff must stay protected). (2) a real, measured lower target clock +for the N=16 variant specifically (unquantified throughput trade-off +against N=2). (3) real Vivado placement/timing directives (e.g. a +`PBLOCK` per group to reduce congestion) as a lower-RTL-risk first +attempt. See EXP-0094's own `next_action`. **The problem it targets**: plain N=16 independent cores (§5.5's own "documentary, expected to break" framing) means 16 independent DDR3 diff --git a/hardware/v2/logs/experiments.log b/hardware/v2/logs/experiments.log index c09ae9d..93301d4 100644 --- a/hardware/v2/logs/experiments.log +++ b/hardware/v2/logs/experiments.log @@ -6397,3 +6397,193 @@ N=16 variant specifically (a real, measured tradeoff against N=2's own real throughput, not yet quantified). Real signal-tracing already done above narrows the fix to the arbiter's own write-data mux specifically -- future work should start there, not guess elsewhere in the design. + +EXP-0094 -- real, hierarchical 2-level arbiter (sdram_arbiter_hier.v): +real fix for EXP-0093's own real timing failure, isolated verification +23/23 PASS (2026-09-21, user's own explicit direction: "sarei tentato +per un arbiter a due livelli... secondo te funziona meglio?", confirmed +"ok ora cerca di terminare il lavoro") + +CONTEXT: EXP-0093's own real, traced root cause: sdram_arbiter_n.v's +own flat req_wdata/req_addr mux grew from 3-way (N=2) to real 20/21-way +(N=16) over a 256-bit bus, dominated by ROUTE delay (73% of the real +critical path) -- a real PHYSICAL fan-in/placement problem (20 +separately-placed sources converging on one central mux), not primarily +a logic-depth one. + +METHOD: `sdram_arbiter_hier.v` (NEW), reusing `sdram_arbiter_n.v` +UNMODIFIED, twice, hierarchically -- 4 real LEAF instances (NUM_REQ=5: +1 weight-fetch + 4 PEs per group, physically local to their own real +systolic_group.v) + 1 real TOP instance (NUM_REQ=5: 4 groups' own +pipelined output + 1 host_mem_bridge.v, deliberately UNPIPELINED/ +bypassed since it was never the reported critical path -- see the +file's own header for the full real design rationale). ONE real +pipeline register stage, both directions, between the two levels -- +the actual real fix for the route-delay-dominated critical path, not +just a logic restructuring. Fork-before-promote: `sdram_arbiter_n.v` +itself untouched, N=2's own real EXP-0088 signoff unaffected. + +REAL CORRECTNESS ARGUMENT (derived, not asserted): every real requester +in this project (`act_tile_fetch.v`, `layer_prefetch_ctrl.v`, +`host_mem_bridge.v`) already keeps its own `mem_active` asserted for +the FULL duration of its own outstanding transaction -- the same real +invariant `sdram_arbiter_n.v`'s own `locked` state already relies on +for transactions spanning many real DDR3-latency cycles today. The +added real pipeline latency (GROUP-sourced traffic only; host bypasses +it entirely) is indistinguishable, from any requester's own point of +view, from "DDR3 was slightly slower this time" -- confirmed, not +assumed, via real isolated verification below. EXP-0066's own real +"own grant same cycle as own active" requirement is preserved EXACTLY +for all 21 real requesters (weight-fetch/PE via the leaf's own +unmodified combinational grant; host via the top's own direct +combinational grant) -- only the underlying ctrl_req/wdata/etc +reaching mig_native_adapter.v is pipelined, never the grant signal a +real requester actually polls. + +REAL BUGS FOUND AND FIXED VIA SIGNAL TRACING BEFORE A TRUSTWORTHY +RESULT WAS POSSIBLE (per this project's own "root-cause every anomaly, +never guess" discipline): + 1. New isolated testbench `tb_sdram_arbiter_hier.v` (21-way, N_GROUPS=4/ + PES_PER_GROUP=4/+1 host, real `burst_mem_model32.v` mock controller, + same real methodology as `tb_sdram_arbiter_n.v`) had NO real + watchdog -- a real protocol bug spun the real Icarus sim forever + (99% CPU, zero output) instead of failing cleanly. Added a real + 500000ns watchdog (a NEW, generalizable gap: every OTHER testbench + in this project already has a `wd`-counted watchdog inside its own + completion-wait loop; this one, copied from a simpler precedent, + didn't). Also hit, mid-debugging, a real false alarm from the + user's own accidental Ctrl-C of a *different*, harmless foreground + `vvp` run -- confirmed via `git status`/`ps -ef` that nothing was + actually lost before continuing. + 2. Testbench bug (NOT RTL): `one_shot_txn`'s own original version, + copied verbatim from `tb_sdram_arbiter_n.v`, fired `req_req` the + SAME cycle as `req_active`, unconditionally, without first + checking `req_grant` -- real, traced failure mode: this module's + own real extra lock-release lag (1-2 cycles, after a PRIOR + transaction on a DIFFERENT slot completes) can race a same-cycle + blind fire, losing the pulse. Confirmed, via `act_tile_fetch.v`'s + own real S_MEMWAIT state ("ctrl_req is only issued after mem_grant + is observed, never blind"), that REAL requesters in this project + already wait for grant before firing req -- fixed the testbench + helper to match real behavior (and this file's own already-correct + `concurrent_contention` task), not the RTL. + 3. REAL RTL BUG (the actual fix this experiment exists to deliver): + `leaf_ctrl_req` is a TRANSIENT one-shot pulse (mirrors a real + requester's own one-cycle `ctrl_req`) -- if the TOP level is still + busy servicing a DIFFERENT group at the exact cycle it fires, a + bare "register leaf_ctrl_req every cycle" pipeline (the first, + broken version of this file) lets the pulse revert to 0 and be + silently lost before TOP ever gets to it -- the EXACT EXP-0066 + lost-pulse class, newly exposed at the leaf-to-top boundary + because a leaf's own LOCAL grant does not guarantee the top level + is free to act on it the same cycle (unlike the flat, single-level + arbiter, where the winning requester's own grant and the physical + controller's own readiness to capture it are the SAME decision). + Root-caused via real $monitor signal tracing of TEST 3 (cross- + group contention: all 4 groups' own weight-fetch simultaneously) -- + traced `u_top.locked`/`grant_idx_r` staying locked onto group 0 + for ~125000fs (~8 real cycles, DDR3-model latency) while groups + 1-3's own already-captured `top_req_req_r` bits had already + reverted to 0. Fixed with a real, sticky per-group `pending_req_r` + latch: SET the cycle `leaf_ctrl_req` first pulses, CLEARED only + once `top_req_grant && top_req_req` both confirm real dispatch for + that group -- `(pending_req_r | leaf_ctrl_req)` feeds the outgoing + pipeline register instead of the bare transient signal. addr/wr/ + wdata/wmask do NOT need the same treatment (the leaf stays locked + onto the SAME real requester for its whole transaction, so those + fields are already stable throughout). + +REAL RESULT (isolated, `tb_sdram_arbiter_hier.v`, real Icarus xsim): +**23/23 PASS, 0 errors** -- sequential single-requester across weight- +fetch/PE/host slots in all 4 groups, WITHIN-group contention (leaf- +level, group 1's weight-fetch + all 4 PEs simultaneously), CROSS-group +contention (top-level, all 4 groups' own weight-fetch simultaneously), +and full worst-case mixed contention (one PE from each of the 4 groups ++ host, all simultaneously) -- every real response verified bit-exact +back to its own correct requester, no misrouting through either +arbiter level. + +Wired into `n16_system_ddr3_top.v` (drop-in replacement, same real +external port shape -- only the module name/parameters at the +instantiation site changed) and into `tb_n16_system_ddr3.v` (host slot +tied off inactive, matching the real topology exactly). + +DECISION: the real, isolated arbiter fix is verified correct. Re- +running EXP-0092's own real functional DDR3 xsim (whole-system, +`tb_n16_system_ddr3.v`) next, to confirm the fix holds wired into the +full system before re-attempting real P&R. + +next_action: (1) real functional xsim of the whole N=16 system with +the new hierarchical arbiter (re-run of EXP-0092's own test). (2) if +that passes, real, full P&R (place_design+route_design) to check +whether WNS actually closes now. + +EXP-0094 (continued) -- real functional re-verification + real, full +P&R with the hierarchical arbiter wired in: SUBSTANTIAL real +improvement, root cause demonstrably shifted, timing STILL not fully +closed (2026-09-21) + +METHOD: (1) real functional xsim, `tb_n16_system_ddr3.v` updated to +instantiate `sdram_arbiter_hier.v` (host slot tied off inactive, +matching the real topology) instead of the flat `sdram_arbiter_n.v`, +same real DDR3-model methodology as EXP-0092. (2) real, full P&R +(synth_design+opt_design+place_design+route_design) against the real +XC7A100T-CSG324-2 part, `n16_system_ddr3_top.v` updated to instantiate +`sdram_arbiter_hier.v` in place of the flat arbiter (drop-in, same +external port shape). `sdram_arbiter_hier.v` added to the real Vivado +project's own `sources_1` fileset as a direct reference (confirmed via +a real stale-import check before trusting the result: empty, no +`*/imports/*` copies of anything in the real dependency chain). + +REAL RESULT (1), functional: **32/32 PASS, 0 errors**, `$finish` at +197735.6335ns -- the hierarchical arbiter fix does not break real +functional correctness at full N=16 scale. + +REAL RESULT (2), P&R: real post-route utilization essentially +unchanged from EXP-0093 (19902 LUTs/31.39%, 35395 regs/27.91%, 128 +DSP48E1/53.33%). **Real timing: WNS=-0.646ns (up from EXP-0093's real +-0.913ns), TNS=-97.541ns (up from -690.085ns, ~7x fewer), 771 failing +setup endpoints (down from 3021, ~4x fewer)** -- a real, substantial, +measured improvement in the RIGHT direction, but timing constraints +are STILL NOT MET. + +REAL, HONEST NEW FINDING (traced via the actual new worst violated +path, not guessed): the bottleneck has DEMONSTRABLY SHIFTED away from +the arbiter entirely. The new worst path runs from `neural_processor_ +packed.v`'s own `GEN_MAC_PACKED[4].product` (a real DSP48E1 MAC output, +inside group 3 / PE 2's own compute core) to its own `prodb1_reg[4][13]` +-- a path dominated by LOGIC delay (79%, CARRY4=4/LUT2=1), NOT route, +unlike EXP-0093's own arbiter-driven, route-dominated (73%) violation. +This is a PRE-EXISTING module, byte-for-byte unchanged since N=2 (EXP- +0088), where it closed with an already razor-thin real margin +(WNS=+0.099962ns). Real, coherent interpretation (not asserted without +the evidence above): N=16's real overall die utilization (31% LUT vs +N=2's much lower level) increases general placement/routing congestion +enough, on its own, to erode that already-thin pre-existing margin -- +a DIFFERENT, more diffuse, congestion-driven problem than the +arbiter's own single, structural fan-in bottleneck, without an +equally obvious single-point fix. + +DECISION: the hierarchical arbiter (EXP-0094) is a real, verified, +substantial improvement, kept and committed regardless of whether full +N=16 timing closure is reached -- it fixed the exact problem it was +built for (confirmed by the bottleneck moving elsewhere entirely). +Full N=16 timing closure needs a SEPARATE, further real intervention +(likely touching `neural_processor_packed.v` itself, the compute core +shared by every real PE in this project, or accepting a lower target +clock for the N=16 variant specifically) -- a real engineering decision +better made with the user's own direction than guessed at further +unprompted, given `neural_processor_packed.v`'s own central, shared +role across the whole project. + +next_action: real options for closing the REMAINING gap, not yet +attempted, to discuss before committing more RTL effort: (1) real +pipelining inside `neural_processor_packed.v`'s own MAC datapath at the +specific `GEN_MAC_PACKED`/`prodb1_reg` boundary (fork-before-promote, +same discipline as the arbiter fix -- N=2's own real signoff must stay +protected). (2) a real, measured lower target clock for the N=16 +variant specifically (quantify the real throughput trade-off against +N=2, not yet done). (3) real Vivado placement/timing directives +(e.g. `-directive` on `place_design`/`route_design`, or a `PBLOCK` +constraint per group to reduce congestion) as a lower-RTL-risk first +attempt before touching `neural_processor_packed.v` itself. diff --git a/hardware/v3/rtl/n16_system_ddr3_top.v b/hardware/v3/rtl/n16_system_ddr3_top.v index 9989845..e4f0510 100644 --- a/hardware/v3/rtl/n16_system_ddr3_top.v +++ b/hardware/v3/rtl/n16_system_ddr3_top.v @@ -142,8 +142,14 @@ module n16_system_ddr3_top #( wire [NUM_REQ*4*BURST_LEN-1:0] req_wmask; wire [NUM_REQ*32*BURST_LEN-1:0] req_rdata; - sdram_arbiter_n #( - .NUM_REQ(NUM_REQ), .ADDR_WIDTH(MEM_ADDR_WIDTH), .BURST_LEN(BURST_LEN) + // EXP-0094: real, hierarchical 2-level arbiter (fixes EXP-0093's own + // real, measured P&R timing failure on the flat 21-way version -- + // see sdram_arbiter_hier.v's own header). Drop-in replacement: same + // real external port shape/NUM_REQ/slot-index convention this + // module's own surrounding req_active/req_grant/etc wiring already + // uses, no other change needed here. + sdram_arbiter_hier #( + .N_GROUPS(N_GROUPS), .PES_PER_GROUP(4), .ADDR_WIDTH(MEM_ADDR_WIDTH), .BURST_LEN(BURST_LEN) ) u_arb ( .clk(ui_clk), .rst(ui_clk_sync_rst), .req_active(req_active), .req_grant(req_grant), diff --git a/hardware/v3/rtl/sdram_arbiter_hier.v b/hardware/v3/rtl/sdram_arbiter_hier.v new file mode 100644 index 0000000..6688a3d --- /dev/null +++ b/hardware/v3/rtl/sdram_arbiter_hier.v @@ -0,0 +1,285 @@ +`timescale 1ns/1ps + +// ============================================================ +// V3 -- real, hierarchical 2-level arbiter for n16_system_ddr3_top.v +// (EXP-0094), built to fix EXP-0093's own real, measured P&R timing +// failure (WNS=-0.913ns on the real 155.039MHz clk_pll_i domain). +// +// REAL ROOT CAUSE THIS FIXES (traced via EXP-0093's own worst violated +// path, not guessed): sdram_arbiter_n.v's own flat req_wdata/req_addr/ +// etc mux grew from a 3-way select at N=2 to a real 20/21-way select at +// N=16 over a 256-bit-wide bus -- EXP-0093's own real post-route report +// showed the delay dominated by ROUTE (73%), not logic (27%): this is a +// real PHYSICAL problem (20 separately-placed PE/weight-fetch sources +// all converging on one central mux at mig_native_adapter.v), not just +// a logic-depth one. +// +// REAL FIX: reuse `sdram_arbiter_n.v` UNMODIFIED, twice, hierarchically +// -- NOT a new arbitration algorithm, the SAME already-proven +// combinational-first-grant/locked semantics, just applied at two +// levels instead of one: +// - LEAF level (4x, one per systolic_group.v): a real +// `sdram_arbiter_n` instance with NUM_REQ=5 (1 weight-fetch + 4 PE +// activation/writeback requesters of THAT group), physically local +// to its own group's own real PEs -- short wires, small mux. +// - TOP level (1x): a real `sdram_arbiter_n` instance with +// NUM_REQ=5 (4 groups' own pipelined output + 1 host_mem_bridge.v, +// unpipelined -- see below), deciding which GROUP gets the real +// shared ctrl_* port this cycle. +// - ONE real pipeline register stage, BOTH directions, between the +// two levels (leaf's own ctrl_req/wr/addr/wdata/wmask -> top's own +// req_*, AND top's own req_ready/req_busy/req_rdata -> leaf's own +// ctrl_ready/ctrl_busy/ctrl_rdata) -- this is the real fix for the +// route-delay-dominated critical path: it lets the placer/router +// spread the two mux stages across two separate clock periods' +// worth of physical budget instead of forcing all 20 sources to +// settle into one central mux within a single cycle. +// +// REAL CORRECTNESS ARGUMENT (not asserted without derivation): EVERY +// requester in this project (`act_tile_fetch.v`, `layer_prefetch_ +// ctrl.v`, `host_mem_bridge.v`) already keeps its own `mem_active` +// asserted for the FULL duration of its own outstanding transaction, +// only dropping it once it has genuinely seen its own completion -- +// this is the real invariant `sdram_arbiter_n.v`'s own `locked` state +// already relies on for transactions that already span many real DDR3- +// latency cycles today. The 2 extra real cycles of round-trip latency +// this hierarchy adds (1 cycle each direction, GROUP-sourced traffic +// only) are indistinguishable, from any requester's own point of view, +// from "DDR3 was slightly slower this time" -- no new race is +// introduced, because the LEAF's own `locked` state depends only on +// its own local requesters' real `mem_active` signals, never on +// response timing from the top level. +// +// REAL, DELIBERATE ASYMMETRY: `host_mem_bridge.v`'s own single real +// requester slot BYPASSES the leaf/pipeline stage entirely, wired +// directly into the top-level arbiter's own 5th slot -- it was never +// the reported critical path (only one physical source, not 20), and +// this keeps its own real round-trip latency completely unchanged from +// today's flat design (no reason to add latency where there is no real +// fan-in problem to fix). +// +// PRESERVES EXP-0066's real "own grant same cycle as own active" +// requirement EXACTLY for all 21 real requesters: weight-fetch/PE +// requesters see the LEAF's own combinational grant (unchanged +// `sdram_arbiter_n.v` internals, just NUM_REQ=5 instead of 21); host +// sees the TOP's own combinational grant directly. Only the underlying +// ctrl_req/wdata/etc reaching mig_native_adapter.v is pipelined -- the +// grant signal a requester actually polls is untouched. +// +// EXTERNAL PORT LIST: byte-for-byte identical shape to +// `sdram_arbiter_n.v` (same NUM_REQ-wide req_active/req_grant/etc +// arrays, same single ctrl_* port) -- a real drop-in replacement at +// the `n16_system_ddr3_top.v` instantiation site, no change needed to +// the surrounding req_active/req_grant/etc bus-slicing code there. +// Real, fixed slot-index convention matched to n16_system_ddr3_top.v's +// own real `GEN_GROUP` layout: slot g (g=0..N_GROUPS-1) = group g's +// own weight-fetch; slot N_GROUPS+g*PES_PER_GROUP+p (p=0..PES_PER_ +// GROUP-1) = group g's own PE p; slot NUM_REQ-1 = host_mem_bridge.v. +// ============================================================ +module sdram_arbiter_hier #( + parameter N_GROUPS = 4, + parameter PES_PER_GROUP = 4, + parameter ADDR_WIDTH = 25, + parameter BURST_LEN = 8, + parameter NUM_REQ = N_GROUPS*(1+PES_PER_GROUP) + 1 // 21, this project's real N=16 topology +)( + input wire clk, + input wire rst, + + input wire [NUM_REQ-1:0] req_active, + output wire [NUM_REQ-1:0] req_grant, + input wire [NUM_REQ-1:0] req_req, + input wire [NUM_REQ-1:0] req_wr, + input wire [NUM_REQ*ADDR_WIDTH-1:0] req_addr, + input wire [NUM_REQ*32*BURST_LEN-1:0] req_wdata, + input wire [NUM_REQ*4*BURST_LEN-1:0] req_wmask, + output wire [NUM_REQ*32*BURST_LEN-1:0] req_rdata, + output wire [NUM_REQ-1:0] req_ready, + output wire [NUM_REQ-1:0] req_busy, + + output wire ctrl_req, + output wire ctrl_wr, + output wire [ADDR_WIDTH-1:0] ctrl_addr, + output wire [32*BURST_LEN-1:0] ctrl_wdata, + output wire [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 GROUP_SIZE = 1 + PES_PER_GROUP; // 5: weight-fetch (local idx 0) + PES_PER_GROUP PEs (local idx 1..4) + + // ---- per-group leaf outputs (outgoing, pre-pipeline) ---- + wire [N_GROUPS-1:0] leaf_active_any; + wire [N_GROUPS-1:0] leaf_ctrl_req, leaf_ctrl_wr; + wire [N_GROUPS*ADDR_WIDTH-1:0] leaf_ctrl_addr; + wire [N_GROUPS*32*BURST_LEN-1:0] leaf_ctrl_wdata; + wire [N_GROUPS*4*BURST_LEN-1:0] leaf_ctrl_wmask; + + // ---- per-group leaf inputs (return path, post-pipeline) ---- + reg [N_GROUPS-1:0] leaf_ctrl_ready_reg, leaf_ctrl_busy_reg; + reg [N_GROUPS*32*BURST_LEN-1:0] leaf_ctrl_rdata_reg; + + genvar lg; + generate + for (lg = 0; lg < N_GROUPS; lg = lg + 1) begin : GEN_LEAF + // real, fixed slot map (matches n16_system_ddr3_top.v's own + // GEN_GROUP layout exactly): local idx 0 = weight-fetch + // (external slot lg, same relative priority it already has + // today -- lower index than its own group's PEs), local idx + // 1..PES_PER_GROUP = PEs 0..PES_PER_GROUP-1 (external slots + // N_GROUPS+lg*PES_PER_GROUP .. +PES_PER_GROUP-1). + wire [GROUP_SIZE-1:0] g_req_active = {req_active[N_GROUPS+lg*PES_PER_GROUP +: PES_PER_GROUP], req_active[lg]}; + wire [GROUP_SIZE-1:0] g_req_req = {req_req[N_GROUPS+lg*PES_PER_GROUP +: PES_PER_GROUP], req_req[lg]}; + wire [GROUP_SIZE-1:0] g_req_wr = {req_wr[N_GROUPS+lg*PES_PER_GROUP +: PES_PER_GROUP], req_wr[lg]}; + wire [GROUP_SIZE*ADDR_WIDTH-1:0] g_req_addr = + {req_addr[(N_GROUPS+lg*PES_PER_GROUP)*ADDR_WIDTH +: PES_PER_GROUP*ADDR_WIDTH], req_addr[lg*ADDR_WIDTH +: ADDR_WIDTH]}; + wire [GROUP_SIZE*32*BURST_LEN-1:0] g_req_wdata = + {req_wdata[(N_GROUPS+lg*PES_PER_GROUP)*32*BURST_LEN +: PES_PER_GROUP*32*BURST_LEN], req_wdata[lg*32*BURST_LEN +: 32*BURST_LEN]}; + wire [GROUP_SIZE*4*BURST_LEN-1:0] g_req_wmask = + {req_wmask[(N_GROUPS+lg*PES_PER_GROUP)*4*BURST_LEN +: PES_PER_GROUP*4*BURST_LEN], req_wmask[lg*4*BURST_LEN +: 4*BURST_LEN]}; + + wire [GROUP_SIZE-1:0] g_req_grant; + wire [GROUP_SIZE*32*BURST_LEN-1:0] g_req_rdata; + wire [GROUP_SIZE-1:0] g_req_ready, g_req_busy; + + sdram_arbiter_n #(.NUM_REQ(GROUP_SIZE), .ADDR_WIDTH(ADDR_WIDTH), .BURST_LEN(BURST_LEN)) u_leaf ( + .clk(clk), .rst(rst), + .req_active(g_req_active), .req_grant(g_req_grant), + .req_req(g_req_req), .req_wr(g_req_wr), .req_addr(g_req_addr), + .req_wdata(g_req_wdata), .req_wmask(g_req_wmask), + .req_rdata(g_req_rdata), .req_ready(g_req_ready), .req_busy(g_req_busy), + .ctrl_req(leaf_ctrl_req[lg]), .ctrl_wr(leaf_ctrl_wr[lg]), + .ctrl_addr(leaf_ctrl_addr[lg*ADDR_WIDTH +: ADDR_WIDTH]), + .ctrl_wdata(leaf_ctrl_wdata[lg*32*BURST_LEN +: 32*BURST_LEN]), + .ctrl_wmask(leaf_ctrl_wmask[lg*4*BURST_LEN +: 4*BURST_LEN]), + .ctrl_rdata(leaf_ctrl_rdata_reg[lg*32*BURST_LEN +: 32*BURST_LEN]), + .ctrl_ready(leaf_ctrl_ready_reg[lg]), .ctrl_busy(leaf_ctrl_busy_reg[lg]) + ); + + assign leaf_active_any[lg] = |g_req_active; + + // un-concatenate the leaf's own real per-slot responses back + // to their real external (global) slot positions -- these are + // the LEAF's own combinational grant/ready/busy/rdata, so the + // real EXP-0066 "own grant same cycle as own active" + // requirement is preserved EXACTLY for every weight-fetch/PE + // requester (see this file's own header). + assign req_grant[lg] = g_req_grant[0]; + assign req_grant[N_GROUPS+lg*PES_PER_GROUP +: PES_PER_GROUP] = g_req_grant[GROUP_SIZE-1:1]; + assign req_ready[lg] = g_req_ready[0]; + assign req_ready[N_GROUPS+lg*PES_PER_GROUP +: PES_PER_GROUP] = g_req_ready[GROUP_SIZE-1:1]; + assign req_busy[lg] = g_req_busy[0]; + assign req_busy[N_GROUPS+lg*PES_PER_GROUP +: PES_PER_GROUP] = g_req_busy[GROUP_SIZE-1:1]; + assign req_rdata[lg*32*BURST_LEN +: 32*BURST_LEN] = g_req_rdata[0 +: 32*BURST_LEN]; + assign req_rdata[(N_GROUPS+lg*PES_PER_GROUP)*32*BURST_LEN +: PES_PER_GROUP*32*BURST_LEN] = + g_req_rdata[32*BURST_LEN +: PES_PER_GROUP*32*BURST_LEN]; + end + endgenerate + + // ---- real pipeline register, outgoing direction (leaf -> top) ---- + // REAL FIX (found via signal tracing, EXP-0094): `leaf_ctrl_req` is + // a TRANSIENT one-shot pulse -- it mirrors the real underlying + // requester's own one-shot ctrl_req (e.g. act_tile_fetch.v's own + // S_MEMWAIT: `ctrl_req <= 1'b1` for exactly one real cycle). A bare + // "register leaf_ctrl_req every cycle" pipeline (the first, broken + // version of this file) loses that pulse whenever the TOP level is + // still busy with a DIFFERENT group at the exact cycle it fires -- + // by the time TOP gets around to this group, the transient pulse + // has already reverted to 0. Real, generalizable lost-pulse bug, + // same EXP-0066 class, now hit at the leaf-to-top boundary because + // (unlike the flat single-level arbiter, where the winning + // requester's own grant and the physical controller's own + // readiness to capture it are ALWAYS the same decision) a leaf's + // own LOCAL grant (which a real requester correctly waits for + // before firing req, confirmed via act_tile_fetch.v) does NOT + // guarantee the TOP level is free to act on it the same cycle. + // FIX: latch `pending_req_r` STICKY per group, from the first real + // cycle `leaf_ctrl_req` pulses until the top level has genuinely + // dispatched it (see the `pending_req_r` update below, placed after + // `u_top` so it can reference `top_req_grant`/`top_req_req`) -- addr/ + // wr/wdata/wmask do NOT need the same treatment: the leaf stays + // locked onto the SAME real requester for its entire transaction + // (real requesters hold `mem_active` until truly done), so those + // fields are already stable for as long as `pending_req_r` matters. + reg [N_GROUPS-1:0] top_req_active_r, top_req_req_r, top_req_wr_r; + reg [N_GROUPS*ADDR_WIDTH-1:0] top_req_addr_r; + reg [N_GROUPS*32*BURST_LEN-1:0] top_req_wdata_r; + reg [N_GROUPS*4*BURST_LEN-1:0] top_req_wmask_r; + reg [N_GROUPS-1:0] pending_req_r; + + always @(posedge clk) begin + if (rst) begin + top_req_active_r <= {N_GROUPS{1'b0}}; + top_req_req_r <= {N_GROUPS{1'b0}}; + top_req_wr_r <= {N_GROUPS{1'b0}}; + top_req_addr_r <= {(N_GROUPS*ADDR_WIDTH){1'b0}}; + top_req_wdata_r <= {(N_GROUPS*32*BURST_LEN){1'b0}}; + top_req_wmask_r <= {(N_GROUPS*4*BURST_LEN){1'b0}}; + end else begin + top_req_active_r <= leaf_active_any; + top_req_req_r <= pending_req_r | leaf_ctrl_req; + top_req_wr_r <= leaf_ctrl_wr; + top_req_addr_r <= leaf_ctrl_addr; + top_req_wdata_r <= leaf_ctrl_wdata; + top_req_wmask_r <= leaf_ctrl_wmask; + end + end + + // ---- top-level arbiter: N_GROUPS (pipelined) + 1 host (direct, + // unpipelined -- see this file's own header for why) ---- + localparam TOP_NUM_REQ = N_GROUPS + 1; + + wire [TOP_NUM_REQ-1:0] top_req_active = {req_active[NUM_REQ-1], top_req_active_r}; + wire [TOP_NUM_REQ-1:0] top_req_req = {req_req[NUM_REQ-1], top_req_req_r}; + wire [TOP_NUM_REQ-1:0] top_req_wr = {req_wr[NUM_REQ-1], top_req_wr_r}; + wire [TOP_NUM_REQ*ADDR_WIDTH-1:0] top_req_addr = {req_addr[(NUM_REQ-1)*ADDR_WIDTH +: ADDR_WIDTH], top_req_addr_r}; + wire [TOP_NUM_REQ*32*BURST_LEN-1:0] top_req_wdata = {req_wdata[(NUM_REQ-1)*32*BURST_LEN +: 32*BURST_LEN], top_req_wdata_r}; + wire [TOP_NUM_REQ*4*BURST_LEN-1:0] top_req_wmask = {req_wmask[(NUM_REQ-1)*4*BURST_LEN +: 4*BURST_LEN], top_req_wmask_r}; + + wire [TOP_NUM_REQ-1:0] top_req_grant; + wire [TOP_NUM_REQ*32*BURST_LEN-1:0] top_req_rdata; + wire [TOP_NUM_REQ-1:0] top_req_ready, top_req_busy; + + sdram_arbiter_n #(.NUM_REQ(TOP_NUM_REQ), .ADDR_WIDTH(ADDR_WIDTH), .BURST_LEN(BURST_LEN)) u_top ( + .clk(clk), .rst(rst), + .req_active(top_req_active), .req_grant(top_req_grant), + .req_req(top_req_req), .req_wr(top_req_wr), .req_addr(top_req_addr), + .req_wdata(top_req_wdata), .req_wmask(top_req_wmask), + .req_rdata(top_req_rdata), .req_ready(top_req_ready), .req_busy(top_req_busy), + .ctrl_req(ctrl_req), .ctrl_wr(ctrl_wr), .ctrl_addr(ctrl_addr), + .ctrl_wdata(ctrl_wdata), .ctrl_wmask(ctrl_wmask), + .ctrl_rdata(ctrl_rdata), .ctrl_ready(ctrl_ready), .ctrl_busy(ctrl_busy) + ); + + // pending_req_r update (see its own declaration above for the real + // rationale): set the cycle a group's own leaf_ctrl_req first + // pulses; clear the cycle the top level actually dispatches it + // (its own grant AND req both true for that group simultaneously -- + // this can only happen several cycles after pending_req_r was set, + // via the outgoing pipeline register, so there is no same-cycle + // set/clear race for a single real request). + wire [N_GROUPS-1:0] pending_clear = top_req_grant[N_GROUPS-1:0] & top_req_req[N_GROUPS-1:0]; + always @(posedge clk) begin + if (rst) pending_req_r <= {N_GROUPS{1'b0}}; + else pending_req_r <= (pending_req_r | leaf_ctrl_req) & ~pending_clear; + end + + // host's own external slot: direct, unpipelined (see header) + assign req_grant[NUM_REQ-1] = top_req_grant[N_GROUPS]; + assign req_ready[NUM_REQ-1] = top_req_ready[N_GROUPS]; + assign req_busy[NUM_REQ-1] = top_req_busy[N_GROUPS]; + assign req_rdata[(NUM_REQ-1)*32*BURST_LEN +: 32*BURST_LEN] = top_req_rdata[N_GROUPS*32*BURST_LEN +: 32*BURST_LEN]; + + // ---- real pipeline register, return direction (top -> leaf) ---- + always @(posedge clk) begin + if (rst) begin + leaf_ctrl_ready_reg <= {N_GROUPS{1'b0}}; + leaf_ctrl_busy_reg <= {N_GROUPS{1'b1}}; // safe default: "still busy" during reset/pipeline-fill + leaf_ctrl_rdata_reg <= {(N_GROUPS*32*BURST_LEN){1'b0}}; + end else begin + leaf_ctrl_ready_reg <= top_req_ready[N_GROUPS-1:0]; + leaf_ctrl_busy_reg <= top_req_busy[N_GROUPS-1:0]; + leaf_ctrl_rdata_reg <= top_req_rdata[N_GROUPS*32*BURST_LEN-1:0]; + end + end +endmodule diff --git a/hardware/v3/sim/tb_n16_system_ddr3.v b/hardware/v3/sim/tb_n16_system_ddr3.v index dee036b..4e89f69 100644 --- a/hardware/v3/sim/tb_n16_system_ddr3.v +++ b/hardware/v3/sim/tb_n16_system_ddr3.v @@ -327,9 +327,17 @@ module tb; .dir_state(dir_state), .dir_error(dir_error), .queue_empty(queue_empty) ); - // ---- real 20-way arbiter (4 groups' own weight-fetch + 16 PEs' own - // activation-fetch+writeback) + 4x systolic_group.v ---- - localparam NUM_REQ = N_GROUPS + N_PES; // 4 + 16 = 20 + // ---- EXP-0094: real, hierarchical 2-level arbiter (4 groups' own + // weight-fetch + 16 PEs' own activation-fetch+writeback + 1 host + // slot, matching n16_system_ddr3_top.v's own real, fixed topology + // exactly -- sdram_arbiter_hier.v always includes a host slot, so + // this testbench ties it off inactive (it doesn't instantiate + // host_mem_bridge.v at all, same real precedent as + // tb_n2_system_ddr3.v never instantiating spi_host_bridge_v3.v) -- + // was a real 20-way flat sdram_arbiter_n.v; replaced after EXP-0093 + // found the flat 21-way version's own real P&R timing failure. ---- + localparam NUM_REQ = N_GROUPS + N_PES + 1; // 4 + 16 + 1 = 21 + localparam HOST_SLOT = NUM_REQ - 1; // 20, tied off inactive below wire [NUM_REQ-1:0] req_active, req_grant, req_req, req_wr; wire [NUM_REQ-1:0] req_ready, req_busy; @@ -338,8 +346,15 @@ module tb; wire [NUM_REQ*4*BURST_LEN-1:0] req_wmask; wire [NUM_REQ*32*BURST_LEN-1:0] req_rdata; - sdram_arbiter_n #( - .NUM_REQ(NUM_REQ), .ADDR_WIDTH(MIG_ADDR_WIDTH), .BURST_LEN(BURST_LEN) + assign req_active[HOST_SLOT] = 1'b0; + assign req_req[HOST_SLOT] = 1'b0; + assign req_wr[HOST_SLOT] = 1'b0; + assign req_addr[HOST_SLOT*MIG_ADDR_WIDTH +: MIG_ADDR_WIDTH] = {MIG_ADDR_WIDTH{1'b0}}; + assign req_wdata[HOST_SLOT*32*BURST_LEN +: 32*BURST_LEN] = {(32*BURST_LEN){1'b0}}; + assign req_wmask[HOST_SLOT*4*BURST_LEN +: 4*BURST_LEN] = {(4*BURST_LEN){1'b0}}; + + sdram_arbiter_hier #( + .N_GROUPS(N_GROUPS), .PES_PER_GROUP(4), .ADDR_WIDTH(MIG_ADDR_WIDTH), .BURST_LEN(BURST_LEN) ) u_arb ( .clk(ui_clk), .rst(ui_clk_sync_rst), .req_active(req_active), .req_grant(req_grant), diff --git a/hardware/v3/sim/tb_sdram_arbiter_hier.v b/hardware/v3/sim/tb_sdram_arbiter_hier.v new file mode 100644 index 0000000..86dcbce --- /dev/null +++ b/hardware/v3/sim/tb_sdram_arbiter_hier.v @@ -0,0 +1,242 @@ +`timescale 1ns/1ps + +// ============================================================ +// EXP-0094 -- isolated correctness test for sdram_arbiter_hier.v, +// NUM_REQ=21 (this project's real N=16 topology: N_GROUPS=4, +// PES_PER_GROUP=4, +1 host), same real "one variable at a time" +// discipline every other new module in this project follows -- verify +// the new hierarchical arbiter in isolation BEFORE wiring it into +// n16_system_ddr3_top.v and re-running a real P&R. +// +// Reuses tb_sdram_arbiter_n.v's own proven real methodology (same +// `burst_mem_model32.v` mock controller, same one-shot-pulse requester +// task mirroring packed_slot.v's/act_tile_fetch.v's real risky +// pattern), scaled up and extended to specifically exercise the NEW +// real risk this module introduces: cross-group AND within-group +// simultaneous contention, verifying every response routes back to +// the CORRECT requester -- the same class of bug (misrouted wide-bus +// response) already found and fixed twice this session in similar +// flattened-bus contexts (tb_systolic_group.v's arbiter offset, +// EXP-0089; this module's own leaf/top slot-index un-concatenation). +// ============================================================ +module tb; + localparam BURST_LEN = 8; + localparam ADDR_WIDTH = 25; + localparam CLK_FREQ_MHZ = 64; + localparam CLK_PERIOD_NS = 1000.0/CLK_FREQ_MHZ; + localparam N_GROUPS = 4; + localparam PES_PER_GROUP = 4; + localparam NUM_REQ = N_GROUPS*(1+PES_PER_GROUP) + 1; // 21 + + reg clk = 0; + always #(CLK_PERIOD_NS/2.0) clk = ~clk; + reg rst; + + wire ctrl_req, ctrl_wr; + wire [ADDR_WIDTH-1:0] ctrl_addr; + wire [32*BURST_LEN-1:0] ctrl_wdata, ctrl_rdata; + wire [4*BURST_LEN-1:0] ctrl_wmask; + wire ctrl_ready, ctrl_busy; + + burst_mem_model32 #( + .BURST_LEN(BURST_LEN), .ADDR_WIDTH(ADDR_WIDTH) + ) u_mem ( + .clk(clk), .rst(rst), + .req(ctrl_req), .wr(ctrl_wr), .addr(ctrl_addr), .wdata(ctrl_wdata), .wmask(ctrl_wmask), + .rdata(ctrl_rdata), .ready(ctrl_ready), .busy(ctrl_busy) + ); + + reg [NUM_REQ-1:0] req_active, req_req, req_wr; + wire [NUM_REQ-1:0] req_grant, req_ready, req_busy; + reg [NUM_REQ*ADDR_WIDTH-1:0] req_addr; + reg [NUM_REQ*32*BURST_LEN-1:0] req_wdata; + reg [NUM_REQ*4*BURST_LEN-1:0] req_wmask; + wire [NUM_REQ*32*BURST_LEN-1:0] req_rdata; + + sdram_arbiter_hier #( + .N_GROUPS(N_GROUPS), .PES_PER_GROUP(PES_PER_GROUP), + .ADDR_WIDTH(ADDR_WIDTH), .BURST_LEN(BURST_LEN) + ) u_arb ( + .clk(clk), .rst(rst), + .req_active(req_active), .req_grant(req_grant), + .req_req(req_req), .req_wr(req_wr), .req_addr(req_addr), + .req_wdata(req_wdata), .req_wmask(req_wmask), + .req_rdata(req_rdata), .req_ready(req_ready), .req_busy(req_busy), + .ctrl_req(ctrl_req), .ctrl_wr(ctrl_wr), .ctrl_addr(ctrl_addr), + .ctrl_wdata(ctrl_wdata), .ctrl_wmask(ctrl_wmask), + .ctrl_rdata(ctrl_rdata), .ctrl_ready(ctrl_ready), .ctrl_busy(ctrl_busy) + ); + + integer errors, tests; + + // helper: real global slot index for group g's weight-fetch (local 0) + // or PE p (local 1..PES_PER_GROUP), matching sdram_arbiter_hier.v's + // own fixed slot-map convention exactly. + function automatic integer wf_slot(input integer g); + wf_slot = g; + endfunction + function automatic integer pe_slot(input integer g, input integer p); + pe_slot = N_GROUPS + g*PES_PER_GROUP + p; + endfunction + + // one-shot-pulse requester task -- REAL fix (found via signal + // tracing, EXP-0094): the original version (copied verbatim from + // tb_sdram_arbiter_n.v) fired req_req the SAME cycle as req_active, + // unconditionally, NOT waiting for req_grant first. That happens to + // work for a flat, single-level arbiter's own uncontended sequential + // tests, but is NOT how real requesters in this project actually + // behave -- confirmed via `act_tile_fetch.v`'s own real S_MEMWAIT + // state ("ctrl_req is only issued after mem_grant is observed, + // never blind", act_tile_fetch.v's own header + S_MEMWAIT: if + // (mem_grant) ctrl_req<=1, a REGISTERED assign, taking effect the + // cycle AFTER grant is seen). This module's own hierarchical design + // introduces a real, legitimate extra lock-release lag (1-2 cycles) + // after a prior transaction on a DIFFERENT slot completes, which a + // same-cycle blind fire can race -- exactly the real EXP-0066 risk + // class, just newly exercised by sequential-but-different-slot + // traffic instead of only true simultaneous contention. Fixed to + // match the real S_MEMWAIT sequencing (and this file's own already- + // correct `concurrent_contention` task): wait for req_grant, THEN + // one more posedge, THEN pulse req_req. + task automatic one_shot_txn( + input integer slot, input t_wr, input [ADDR_WIDTH-1:0] t_addr, + input [32*BURST_LEN-1:0] t_wdata, output [32*BURST_LEN-1:0] t_rdata + ); + begin + @(posedge clk); + req_active[slot] = 1'b1; + req_wr[slot] = t_wr; + req_addr[slot*ADDR_WIDTH +: ADDR_WIDTH] = t_addr; + req_wdata[slot*32*BURST_LEN +: 32*BURST_LEN] = t_wdata; + req_wmask[slot*4*BURST_LEN +: 4*BURST_LEN] = {(4*BURST_LEN){1'b0}}; + while (!req_grant[slot]) @(posedge clk); + @(posedge clk); + req_req[slot] = 1'b1; + @(posedge clk); + req_req[slot] = 1'b0; + while (!req_ready[slot]) @(posedge clk); + t_rdata = req_rdata[slot*32*BURST_LEN +: 32*BURST_LEN]; + req_active[slot] = 1'b0; + end + endtask + + reg [32*BURST_LEN-1:0] got, wpat; + + task automatic check_slot(input integer slot, input [ADDR_WIDTH-1:0] a, input [15:0] pattern); + integer i; + begin + for (i = 0; i < BURST_LEN; i = i + 1) + wpat[i*32 +: 32] = {pattern, pattern + i[15:0]}; + one_shot_txn(slot, 1'b1, a, wpat, got); + one_shot_txn(slot, 1'b0, a, {(32*BURST_LEN){1'b0}}, got); + tests = tests + 1; + if (got !== wpat) begin + $display("FAIL slot=%0d addr=%0d: got=%h expected=%h", slot, a, got, wpat); + errors = errors + 1; + end else begin + $display("PASS slot=%0d addr=%0d: bit-exact", slot, a); + end + end + endtask + + // concurrent-contention task: N requesters (given by the module- + // level `csl`/`caddr`/`cpat` arrays, set by the caller just before + // invoking this task -- Verilog-2001 tasks can't take dynamic array + // args cleanly) all raise `active` the SAME cycle, each only + // pulsing its own `req` once ITS OWN grant is observed (real, + // established packed_slot.v-style S_MEMWAIT discipline, not a + // blind simultaneous fire) -- then all read back bit-exact, proving + // no cross-requester corruption/misrouting through either arbiter + // level. Explicit, unrolled per-requester fork branches (n<=8 real + // max used in this test) avoid the classic Verilog for-loop-inside- + // fork variable-capture race entirely. + reg [32*BURST_LEN-1:0] cwpat [0:7]; + integer csl [0:7]; + reg [ADDR_WIDTH-1:0] caddr [0:7]; + reg [15:0] cpat [0:7]; + task automatic concurrent_contention(input integer n); + integer k, kk; + begin + for (k = 0; k < n; k = k + 1) begin + for (kk = 0; kk < BURST_LEN; kk = kk + 1) + cwpat[k][kk*32 +: 32] = {cpat[k], cpat[k] + kk[15:0]}; + req_addr[csl[k]*ADDR_WIDTH +: ADDR_WIDTH] = caddr[k]; + req_wdata[csl[k]*32*BURST_LEN +: 32*BURST_LEN] = cwpat[k]; + req_wr[csl[k]] = 1'b1; + end + @(posedge clk); + for (k = 0; k < n; k = k + 1) req_active[csl[k]] = 1'b1; + + fork + if (n > 0) begin while (!req_grant[csl[0]]) @(posedge clk); @(posedge clk); req_req[csl[0]]=1'b1; @(posedge clk); req_req[csl[0]]=1'b0; while(!req_ready[csl[0]]) @(posedge clk); req_active[csl[0]]=1'b0; end + if (n > 1) begin while (!req_grant[csl[1]]) @(posedge clk); @(posedge clk); req_req[csl[1]]=1'b1; @(posedge clk); req_req[csl[1]]=1'b0; while(!req_ready[csl[1]]) @(posedge clk); req_active[csl[1]]=1'b0; end + if (n > 2) begin while (!req_grant[csl[2]]) @(posedge clk); @(posedge clk); req_req[csl[2]]=1'b1; @(posedge clk); req_req[csl[2]]=1'b0; while(!req_ready[csl[2]]) @(posedge clk); req_active[csl[2]]=1'b0; end + if (n > 3) begin while (!req_grant[csl[3]]) @(posedge clk); @(posedge clk); req_req[csl[3]]=1'b1; @(posedge clk); req_req[csl[3]]=1'b0; while(!req_ready[csl[3]]) @(posedge clk); req_active[csl[3]]=1'b0; end + if (n > 4) begin while (!req_grant[csl[4]]) @(posedge clk); @(posedge clk); req_req[csl[4]]=1'b1; @(posedge clk); req_req[csl[4]]=1'b0; while(!req_ready[csl[4]]) @(posedge clk); req_active[csl[4]]=1'b0; end + if (n > 5) begin while (!req_grant[csl[5]]) @(posedge clk); @(posedge clk); req_req[csl[5]]=1'b1; @(posedge clk); req_req[csl[5]]=1'b0; while(!req_ready[csl[5]]) @(posedge clk); req_active[csl[5]]=1'b0; end + if (n > 6) begin while (!req_grant[csl[6]]) @(posedge clk); @(posedge clk); req_req[csl[6]]=1'b1; @(posedge clk); req_req[csl[6]]=1'b0; while(!req_ready[csl[6]]) @(posedge clk); req_active[csl[6]]=1'b0; end + if (n > 7) begin while (!req_grant[csl[7]]) @(posedge clk); @(posedge clk); req_req[csl[7]]=1'b1; @(posedge clk); req_req[csl[7]]=1'b0; while(!req_ready[csl[7]]) @(posedge clk); req_active[csl[7]]=1'b0; end + join + + tests = tests + 1; + $display("PASS: %0d simultaneous requests completed (none silently lost)", n); + + for (k = 0; k < n; k = k + 1) + check_slot(csl[k], caddr[k], cpat[k]); + end + endtask + + // real watchdog -- this testbench originally had none, and a real + // protocol bug in a new module (unlike every other testbench in + // this project, which uses a `wd` cycle-counted watchdog inside its + // own completion-wait loop) spun forever burning CPU with zero + // output instead of failing cleanly. Real, generalizable lesson. + initial begin + #500000; + $display("FAIL: WATCHDOG TIMEOUT -- simulation did not complete within 500000ns"); + $finish; + end + + initial begin + errors = 0; tests = 0; + rst = 1; req_active = 0; req_req = 0; req_wr = 0; req_addr = 0; req_wdata = 0; req_wmask = 0; + repeat(5) @(posedge clk); + rst = 0; + @(posedge clk); + + $display("=== TEST 1: sequential single-requester, spanning weight-fetch/PE/host slots across all 4 groups ==="); + check_slot(wf_slot(0), 25'd0, 16'hA000); + check_slot(pe_slot(0,0), 25'd8, 16'hA100); + check_slot(pe_slot(0,3), 25'd16, 16'hA200); + check_slot(wf_slot(2), 25'd24, 16'hA300); + check_slot(pe_slot(2,1), 25'd32, 16'hA400); + check_slot(NUM_REQ-1 /* host */, 25'd40, 16'hA500); + + $display("=== TEST 2: WITHIN-group contention (leaf-level arbitration) -- group 1's weight-fetch + all 4 PEs simultaneously ==="); + csl[0]=wf_slot(1); caddr[0]=25'd100; cpat[0]=16'hB000; + csl[1]=pe_slot(1,0); caddr[1]=25'd108; cpat[1]=16'hB100; + csl[2]=pe_slot(1,1); caddr[2]=25'd116; cpat[2]=16'hB200; + csl[3]=pe_slot(1,2); caddr[3]=25'd124; cpat[3]=16'hB300; + csl[4]=pe_slot(1,3); caddr[4]=25'd132; cpat[4]=16'hB400; + concurrent_contention(5); + + $display("=== TEST 3: CROSS-group contention (top-level arbitration) -- all 4 groups' own weight-fetch simultaneously ==="); + csl[0]=wf_slot(0); caddr[0]=25'd200; cpat[0]=16'hC000; + csl[1]=wf_slot(1); caddr[1]=25'd208; cpat[1]=16'hC100; + csl[2]=wf_slot(2); caddr[2]=25'd216; cpat[2]=16'hC200; + csl[3]=wf_slot(3); caddr[3]=25'd224; cpat[3]=16'hC300; + concurrent_contention(4); + + $display("=== TEST 4: full contention -- one PE from EACH of the 4 groups, simultaneously, plus host at the same time (real, worst-case mixed cross-group + bypass-path contention) ==="); + csl[0]=pe_slot(0,2); caddr[0]=25'd300; cpat[0]=16'hD000; + csl[1]=pe_slot(1,3); caddr[1]=25'd308; cpat[1]=16'hD100; + csl[2]=pe_slot(2,0); caddr[2]=25'd316; cpat[2]=16'hD200; + csl[3]=pe_slot(3,1); caddr[3]=25'd324; cpat[3]=16'hD300; + csl[4]=NUM_REQ-1; caddr[4]=25'd332; cpat[4]=16'hD400; + concurrent_contention(5); + + $display("=== %0d/%0d tests, %0d errors ===", tests-errors, tests, errors); + if (errors == 0) $display("ALL TESTS PASSED (tb_sdram_arbiter_hier)"); + $finish; + end +endmodule