feat: N=16 real timing CLOSED via extra MAC pipeline stage (EXP-0097, branch n16-timing-closure)

neural_processor_packed.v: split the original single "Stage 1" (packed
DSP48E1 multiply + INT8 unpack + register) into two real stages --
Stage 1a registers the raw DSP48E1 product with zero logic in between,
Stage 1b does the carry-heavy unpack (the real critical path EXP-0094
traced) from that already-registered value. Adds exactly one real
clock cycle of latency; throughput unaffected (real valid/ready
handshaking throughout, no fixed-latency assumption downstream).

Real verification: isolated bit-exact vs 2x real neural_processor.v
(18/18 PASS, testbench fixed to latch each core's result independently
since result_valid is a one-shot pulse and the DUT is now one cycle
deeper -- not an RTL bug). Full-system functional xsim on real DDR3:
32/32 PASS. Real, full P&R: WNS=+0.269ns, WHS=+0.026ns, 0 failing
setup or hold endpoints -- N=16 TIMING CLOSES.

Also root-caused (not an RTL bug, folded into CLAUDE.md): a real
Vivado incremental-synthesis quirk silently carried forward a
N_GROUPS=2 parameter binding from an earlier sweep run despite no
-generic override and an intervening reset_run -- fixed by always
passing -generic explicitly and confirming the real elaborated value
via a post-synth DSP48E1 count.

Isolated on this branch -- does not touch the physical board already
in fabrication on v3-artix7 (N=8, unmodified).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MUG92aM9m68TRc4rG55BcC
This commit is contained in:
2026-09-22 00:17:18 +02:00
co-authored by Claude Sonnet 5
parent 4c59b7e7c8
commit f9b366d747
4 changed files with 271 additions and 40 deletions
+17
View File
@@ -81,6 +81,23 @@ unmodified by v3, e.g. `layer_prefetch_ctrl.v`/`layer_weight_buffer.v`).
Real fix: explicitly `set_property top <newmodule> [get_filesets
sources_1]` *before* calling `synth_design -top ...` — the `-top`
command-line flag alone wasn't sufficient this time.
- **A top-level module's own default parameter value can silently NOT
apply**, even with no `-generic` override on the `synth_design`
command line, an empty real `GENERIC` property on the run, and no
stale imported RTL copy (EXP-0097) — a real elaboration bound
`N_GROUPS` to a value from a DIFFERENT, EARLIER `-generic` override
used against the SAME top module in the SAME Vivado session/project
(an N=8 sweep run before an N=16 run), despite an intervening
`reset_run`. Most likely Vivado's own "Incremental synthesis
strategy default" silently carrying forward a parameter binding.
Real fix: always pass every `-generic` value EXPLICITLY on every
real `synth_design` call for a parameterized top-level, never rely
on "no override = the RTL's own default" once that module has EVER
been synthesized with a different override earlier in the same
project — and confirm the real elaborated value afterward (e.g. a
post-synth DSP48E1/cell count check) before trusting anything
downstream, don't assume the log's own "Parameter ... bound to"
line will be checked in time otherwise.
- **Testbench stimulus must use nonblocking assignment (`<=`), not blocking
(`=`), when driving a DUT's inputs from a separate `always`/`initial`
block.** Blocking assignment races the DUT's own `posedge`-triggered
+125
View File
@@ -6784,3 +6784,128 @@ docs/PINOUT.md, BOM confirmation from docs/BOM.md -- both already real
and unaffected by the N_GROUPS choice, since DDR3/SPI/flash/config
pins are package-level, not internal-core-count-dependent) is the real
next milestone now that a real, closed, deployable RTL target exists.
EXP-0097 -- N=16 REAL TIMING CLOSED: extra MAC pipeline stage (branch
`n16-timing-closure`), the previously-deferred real fix now built and
verified (2026-09-21/22, user's own explicit direction: "creare una
branch del progetto e lavora per scoprire come fare funzionare il
timing", physical board fabrication continues in parallel on the
already-fixed N=8 design, unaffected by this branch)
CONTEXT: EXP-0094's own real, traced remaining N=16 bottleneck (after
the hierarchical arbiter + P&R directive tuning already closed most of
the gap, WNS -0.913ns -> -0.338ns) was inside `neural_processor_
packed.v`'s own DSP48E1 MAC datapath -- a pre-existing, N=2-era design
(unchanged since EXP-0059) with an already razor-thin real margin
(+0.099962ns) that N=16's own higher real die congestion eroded past
zero. EXP-0094's own `next_action` flagged real MAC-datapath pipelining
as the most direct remaining fix, deliberately not attempted then
(shared, load-bearing module, needed explicit direction + isolation
from the definitive N=8 signoff -- hence the real, separate branch).
METHOD: real, traced worst-violated-path analysis (EXP-0094's own real
post-route report) pinpointed the exact real gap: a DSP48E1's own
(Vivado-auto-retimed) product register feeding STRAIGHT THROUGH the
real carry-heavy INT8-unpack logic (`pb_comb`'s own shift + conditional
+1 carry-propagate add, CARRY4-dominated) into `proda1`/`prodb1` in a
SINGLE real cycle. Real fix: split the original single "Stage 1" into
two real stages -- **Stage 1a** registers the RAW DSP48E1 product with
zero logic in between (`product_reg`, a real, explicit register
boundary immediately after the multiply); **Stage 1b** does the
carry-heavy unpack FROM the already-registered `product_reg` and
registers the result into `proda1`/`prodb1` (unchanged real math,
now one real cycle later). Real, deliberate consequence: end-to-end
per-tile latency grows by exactly ONE real clock cycle; throughput is
unaffected (still accepts one new operand per cycle, real valid/ready
handshaking throughout, no fixed-latency assumption anywhere
downstream). `pipeline_busy`/`valid_tree`'s own level-0 input and the
module's own header comment updated to match.
REAL BUG FOUND AND FIXED IN THE TESTBENCH BEFORE A TRUSTWORTHY RESULT
WAS POSSIBLE (not an RTL bug): `tb_neural_processor_packed.v`'s own
comparison logic required all three cores (2 real reference `neural_
processor.v` instances + the DUT) to assert `result_valid`
SIMULTANEOUSLY -- correct only when all three share the exact same
real pipeline depth. Since `result_valid` is a genuine ONE-SHOT pulse
in every one of these FSMs (self-clears the cycle after `result_ready`
is seen, identical pattern in both v2 and v3 cores), and the DUT is
now deliberately one real cycle deeper than the reference cores, the
reference cores' own `result_valid` had already dropped by the time
the DUT's own pulse arrived -- the original three-way AND never
triggered again, a real 18/18 watchdog-timeout false-failure, not an
actual DUT bug (confirmed via a real, controlled A/B: the SAME failure
does NOT reproduce against the unmodified reference-only comparison
path). Fixed by latching each core's own result independently the
cycle its own `result_valid` first pulses, then comparing the three
LATCHED values once all three have arrived -- correct regardless of
real relative pipeline depth.
Also hit and root-caused (real, not guessed): `tb_np_packed_layer_
reuse.v` fails (3/16 PASS) identically against BOTH the modified AND
the original, unmodified `neural_processor_packed.v` (confirmed via a
real, direct A/B comparison) -- a real, PRE-EXISTING, already-broken/
stale testbench (real port-width mismatch warning on `layer_prefetch_
ctrl.v`'s own `ctrl_wdata`/`ctrl_rdata`, 128 bits wired against a
256-bit real port -- dates from before EXP-0084's own 32-bit DDR3
widening, apparently never updated), unrelated to this real fix, out
of scope for this branch's own task.
Also hit and root-caused (real Vivado project-state quirk, not an RTL
bug): a first real P&R attempt on this branch elaborated with
`N_GROUPS` bound to 2, not the RTL's own real default of 4, despite no
`-generic` override on the actual `synth_design` command line, an
empty real `GENERIC` property on the `synth_1` run, the correct real
`top` property, and no stale imported copy of `n16_system_ddr3_top.v`
anywhere in the project (all confirmed via direct real queries, not
assumed) -- most likely Vivado's own "Incremental synthesis strategy
default" silently carrying forward a parameter binding from this
session's own earlier `-generic N_GROUPS=2` sweep run (EXP-0095),
despite an intervening `reset_run`. Real fix: pass `-generic
N_GROUPS=4` explicitly on the `synth_design` command line rather than
relying on the RTL's own default resolving correctly -- confirmed via
a real, explicit post-synth DSP48E1 cell-count check (128, matching
real N=16) before trusting anything downstream this time.
REAL RESULT: (1) isolated bit-exact verification,
`tb_neural_processor_packed.v` (real Icarus xsim, against 2x real
`hardware/v2/rtl/neural_processor.v`): **18/18 PASS, 0 errors**. (2)
real, full-system functional xsim, `tb_n16_system_ddr3.v` (real DDR3
model, real Vivado xsim): **32/32 PASS, 0 errors**, `$finish` at
197735.6335ns (same real completion time as the pre-fix EXP-0094
result -- the extra real pipeline cycle is fully absorbed by DDR3's
own already-dominant real latency, no observable end-to-end slowdown
at this scale). (3) real, full P&R (`n16_system_ddr3_top.v`, real
XC7A100T-CSG324-2, `Explore`/`ExtraNetDelay_high`/`AggressiveExplore`
directive stack, EXP-0094's own real N_GROUPS=4 explicitly confirmed
via a real post-synth DSP48E1 count of 128): **WNS=+0.269ns,
WHS=+0.026ns, TNS=0.000ns, 0 FAILING SETUP OR HOLD ENDPOINTS --
TIMING CONSTRAINTS ARE MET.** Real utilization: 19903 LUTs (31.39%),
35409 registers (27.93%, up from 19936/27.92%... i.e. genuinely more
registers than the pre-fix EXP-0094 result, matching the real,
expected cost of the added pipeline stage across 16 real PE
instances), 128 DSP48E1 (53.33%).
DECISION: N=16 hybrid systolic (`n16_system_ddr3_top.v`) is now REAL,
functionally verified, AND timing-CLOSED, on this real, isolated
branch (`n16-timing-closure`) -- does not touch or affect the physical
board fabrication already underway on N=8 (`v3-artix7`, unmodified).
This is a real, significant milestone: it confirms the N=16 hybrid
systolic architecture is fundamentally viable at full real scale, not
just "close" -- the earlier N=8-as-definitive decision was a real,
reasonable engineering choice under the "ship something real now"
constraint (WNS=0.000ns exact-zero margin vs. this fix's own real,
more comfortable +0.269ns), not a permanent architectural ceiling.
next_action: (1) real, dedicated functional xsim + P&R re-confirmation
specifically for the definitive N=8 configuration WITH this same MAC
pipeline fix applied (verify it does not regress N=8's own real,
already-closed signoff, and ideally IMPROVES its own already-thin
future margin) -- not yet done on this branch. (2) a real, explicit
decision with the user on whether/when to promote this fix back to
`v3-artix7` (the physical board's own branch) -- given the board is
already in fabrication as the UNMODIFIED N=8 design, this is a real
question about a FUTURE board revision, not the current one. (3) this
branch's own real bug findings (the testbench latching fix, the
Vivado incremental-synthesis generic-binding quirk) are worth folding
into CLAUDE.md's own hard-won-lessons section regardless of the
promotion decision.
+85 -24
View File
@@ -12,12 +12,20 @@
// to keep this module's own pipeline depth/stage count identical to
// the V2 original for a direct structural comparison).
//
// Pipeline stages match V2's neural_processor.v exactly, just doubled
// on the accumulator side (one accumulate/bias/activation/saturation
// path per job, A and B, sharing the SAME multiply/adder-tree stages
// since they consume the SAME weight stream):
// Stage 0 input alignment (x0_a, x0_b, w0 -- ONE shared weight)
// Stage 1 P_IN packed-MAC lanes: p0[i]=x0_a[i]*w0[i], p1[i]=x0_b[i]*w0[i]
// Pipeline stages, originally matched V2's neural_processor.v exactly
// (one accumulate/bias/activation/saturation path per job, A and B,
// sharing the SAME multiply/adder-tree stages since they consume the
// SAME weight stream). EXTENDED BY ONE REAL STAGE on the
// n16-timing-closure branch (real fix for EXP-0094's own real, traced
// N=16 P&R timing failure -- see Stage 1a/1b's own header comments for
// the full real root-cause story):
// Stage 0 input alignment (x0_a, x0_b, w0 -- ONE shared weight)
// Stage 1a P_IN real DSP48E1 packed multiplies, registered raw
// (product_reg) -- NEW real stage
// Stage 1b unpack the two packed INT8 products from product_reg:
// p0[i]=x0_a[i]*w0[i], p1[i]=x0_b[i]*w0[i] -- same real
// math as the original single "Stage 1", now one real
// cycle later
// Stage 2..(1+TREE_LEVELS) TWO balanced adder trees (A and B)
// Stage (2+TREE_LEVELS) TWO accumulators
// Stage (3+TREE_LEVELS) bias add (shared bias/activation -- same
@@ -25,6 +33,14 @@
// + activation, per job
// Stage (4+TREE_LEVELS) INT8 saturation / output register, per job
//
// Real, deliberate consequence: end-to-end per-tile latency grows by
// exactly ONE real clock cycle versus the original design (throughput
// is unaffected -- the pipeline still accepts one new operand per
// cycle in steady state). Functional behavior (the actual packed-MAC
// arithmetic) is byte-for-byte unchanged -- verified bit-exact against
// the same real reference used since EXP-0059 (2x real
// hardware/v2/rtl/neural_processor.v), `tb_neural_processor_packed.v`.
//
// job_bias/job_activation are SHARED between A and B (same resident
// neuron), matching this project's own weight-reuse semantics (a
// neuron/filter's bias and activation type don't vary by spatial
@@ -116,17 +132,28 @@ module neural_processor_packed #(
end
// ============================================================
// STAGE 1 -- P_IN packed-MAC lanes (mac2_dsp_packed.v's own
// verified combinational formula, inlined per lane)
// STAGE 1a -- P_IN real DSP48E1 packed multiplies, registered RAW
// (n16-timing-closure branch, real fix for EXP-0094's own real,
// traced N=16 critical path). EXP-0093/0094's own real post-route
// reports found the worst violated path running from a DSP48E1's
// own (Vivado-auto-retimed) product register straight through the
// pb_comb unpack logic below (a real, CARRY4-heavy shift + carry-
// propagate add) into proda1/prodb1 in a SINGLE cycle -- already
// razor-thin at N=2 (WNS=+0.0999962ns, EXP-0088) and pushed
// negative by N=16's own extra real placement congestion (EXP-
// 0093/0094). This stage makes the DSP's own real output register
// explicit in RTL (captures the WHOLE raw packed product, zero
// logic in between) instead of relying on the tool to retime one
// in automatically -- the actual, additional real pipeline stage
// this fix needs is STAGE 1b below, which now has its own full
// real clock period to do the unpack work in.
// ============================================================
reg valid1, last1;
reg signed [ACC_WIDTH-1:0] proda1 [0:P_IN-1];
reg signed [ACC_WIDTH-1:0] prodb1 [0:P_IN-1];
localparam A_WIDTH = 3*DATA_WIDTH + 1;
localparam PRODUCT_WIDTH = A_WIDTH + DATA_WIDTH;
wire signed [PROD_WIDTH-1:0] pa_comb [0:P_IN-1];
wire signed [PROD_WIDTH-1:0] pb_comb [0:P_IN-1];
wire signed [PRODUCT_WIDTH-1:0] product_comb [0:P_IN-1];
reg signed [PRODUCT_WIDTH-1:0] product_reg [0:P_IN-1];
genvar gm;
generate
@@ -134,12 +161,7 @@ module neural_processor_packed #(
wire signed [A_WIDTH-1:0] x0_sext25 = {{(A_WIDTH-DATA_WIDTH){xa0[gm][DATA_WIDTH-1]}}, xa0[gm]};
wire signed [A_WIDTH-1:0] x1_shifted = $signed(xb0[gm]) <<< (2*DATA_WIDTH);
wire signed [A_WIDTH-1:0] packed_a = x1_shifted + x0_sext25;
wire signed [A_WIDTH+DATA_WIDTH-1:0] product = packed_a * w0[gm];
assign pa_comb[gm] = product[PROD_WIDTH-1:0];
wire signed [A_WIDTH+DATA_WIDTH-2*DATA_WIDTH-1:0] pb_raw =
$signed(product) >>> (2*DATA_WIDTH);
assign pb_comb[gm] = pb_raw[PROD_WIDTH-1:0] + (pa_comb[gm][PROD_WIDTH-1] ? 1'b1 : 1'b0);
assign product_comb[gm] = packed_a * w0[gm];
end
endgenerate
@@ -150,6 +172,45 @@ module neural_processor_packed #(
end else begin
valid1 <= valid0;
last1 <= last0;
for (gi = 0; gi < P_IN; gi = gi + 1)
product_reg[gi] <= product_comb[gi];
end
end
// ============================================================
// STAGE 1b -- unpack the two packed INT8 products from the
// ALREADY-REGISTERED product_reg (real, added pipeline stage --
// the actual timing fix). pa_comb/pb_comb's own math is byte-for-
// byte IDENTICAL to the original single-stage version, only the
// source (product_reg, a real register) and the register that
// captures the result (proda1/prodb1, now one real cycle later)
// changed -- functional behavior is unchanged, only latency grows
// by exactly one real clock cycle.
// ============================================================
reg valid1b, last1b;
reg signed [ACC_WIDTH-1:0] proda1 [0:P_IN-1];
reg signed [ACC_WIDTH-1:0] prodb1 [0:P_IN-1];
wire signed [PROD_WIDTH-1:0] pa_comb [0:P_IN-1];
wire signed [PROD_WIDTH-1:0] pb_comb [0:P_IN-1];
genvar gp;
generate
for (gp = 0; gp < P_IN; gp = gp + 1) begin : GEN_UNPACK
assign pa_comb[gp] = product_reg[gp][PROD_WIDTH-1:0];
wire signed [A_WIDTH+DATA_WIDTH-2*DATA_WIDTH-1:0] pb_raw =
$signed(product_reg[gp]) >>> (2*DATA_WIDTH);
assign pb_comb[gp] = pb_raw[PROD_WIDTH-1:0] + (pa_comb[gp][PROD_WIDTH-1] ? 1'b1 : 1'b0);
end
endgenerate
always @(posedge clk) begin
if (rst) begin
valid1b <= 1'b0;
last1b <= 1'b0;
end else begin
valid1b <= valid1;
last1b <= last1;
for (gi = 0; gi < P_IN; gi = gi + 1) begin
proda1[gi] <= {{(ACC_WIDTH-PROD_WIDTH){pa_comb[gi][PROD_WIDTH-1]}}, pa_comb[gi]};
prodb1[gi] <= {{(ACC_WIDTH-PROD_WIDTH){pb_comb[gi][PROD_WIDTH-1]}}, pb_comb[gi]};
@@ -183,8 +244,8 @@ module neural_processor_packed #(
valid_tree[gl] <= 1'b0;
last_tree[gl] <= 1'b0;
end else begin
valid_tree[gl] <= (gl == 0) ? valid1 : valid_tree[gl-1];
last_tree[gl] <= (gl == 0) ? last1 : last_tree[gl-1];
valid_tree[gl] <= (gl == 0) ? valid1b : valid_tree[gl-1];
last_tree[gl] <= (gl == 0) ? last1b : last_tree[gl-1];
end
end
for (gn = 0; gn < (P_IN >> (gl+1)); gn = gn + 1) begin : GEN_TREE_NODE
@@ -203,8 +264,8 @@ module neural_processor_packed #(
end
endgenerate
wire valid_tree_out = (TREE_LEVELS == 0) ? valid1 : valid_tree[TREE_LEVELS-1];
wire last_tree_out = (TREE_LEVELS == 0) ? last1 : last_tree[TREE_LEVELS-1];
wire valid_tree_out = (TREE_LEVELS == 0) ? valid1b : valid_tree[TREE_LEVELS-1];
wire last_tree_out = (TREE_LEVELS == 0) ? last1b : last_tree[TREE_LEVELS-1];
wire signed [ACC_WIDTH-1:0] tile_sum_a = (TREE_LEVELS == 0) ? proda1[0] : treea[TREE_LEVELS][0];
wire signed [ACC_WIDTH-1:0] tile_sum_b = (TREE_LEVELS == 0) ? prodb1[0] : treeb[TREE_LEVELS][0];
@@ -294,7 +355,7 @@ module neural_processor_packed #(
end
end
wire pipeline_busy = valid0 || valid1 || (|valid_tree) || valid5 || valid6 || valid7;
wire pipeline_busy = valid0 || valid1 || valid1b || (|valid_tree) || valid5 || valid6 || valid7;
assign job_ready = (np_state == NP_IDLE) && !pipeline_busy;
// ============================================================
+44 -16
View File
@@ -148,29 +148,57 @@ module tb;
v2_tile_last = 0;
tile_last = 0;
// real fix (n16-timing-closure branch): result_valid is a
// real ONE-SHOT pulse in every one of these FSMs (`NP_
// WRITE_RESULT: if (result_valid && result_ready)
// result_valid<=0`, identical in neural_processor.v and
// neural_processor_packed.v) -- with result_ready already
// held high before this wait begins, each core's own
// result_valid self-clears the very next cycle after it
// first asserts, independent of whether the OTHER cores
// have caught up yet. The original three-way simultaneous
// AND assumed all three cores share the exact same real
// pipeline depth -- true before this branch's own real
// extra pipeline stage in neural_processor_packed.v (added
// to fix EXP-0094's own real N=16 timing failure), no
// longer true now that the DUT is deliberately one real
// cycle deeper than the reference cores. Real fix: latch
// each core's own result independently the cycle its own
// result_valid pulses, then compare the three LATCHED
// values once all three have arrived -- correct regardless
// of real relative pipeline depth between DUT and
// reference.
v2_result_ready = 1;
result_ready = 1;
watchdog = 0;
while (!(v2a_result_valid && v2b_result_valid && result_valid) && watchdog < 300) begin
@(posedge clk);
watchdog = watchdog + 1;
end
begin : capture
reg v2a_got, v2b_got, dut_got;
reg signed [DATA_WIDTH-1:0] v2a_val, v2b_val, dut_val_a, dut_val_b;
v2a_got = 0; v2b_got = 0; dut_got = 0;
watchdog = 0;
while (!(v2a_got && v2b_got && dut_got) && watchdog < 300) begin
@(posedge clk);
if (!v2a_got && v2a_result_valid) begin v2a_got = 1; v2a_val = v2a_result_data; end
if (!v2b_got && v2b_result_valid) begin v2b_got = 1; v2b_val = v2b_result_data; end
if (!dut_got && result_valid) begin dut_got = 1; dut_val_a = result_data_a; dut_val_b = result_data_b; end
watchdog = watchdog + 1;
end
if (!v2a_result_valid || !v2b_result_valid || !result_valid) begin
$display("FAIL n=%0d: watchdog timeout waiting for results (v2a=%b v2b=%b dut=%b)",
n, v2a_result_valid, v2b_result_valid, result_valid);
errors = errors + 1;
end else begin
if (result_data_a !== v2a_result_data || result_data_b !== v2b_result_data) begin
$display("FAIL n=%0d bias=%0d act=%0d: v2a=%0d v2b=%0d dut_a=%0d dut_b=%0d MISMATCH",
n, bias, activation, v2a_result_data, v2b_result_data, result_data_a, result_data_b);
if (!v2a_got || !v2b_got || !dut_got) begin
$display("FAIL n=%0d: watchdog timeout waiting for results (v2a_got=%b v2b_got=%b dut_got=%b)",
n, v2a_got, v2b_got, dut_got);
errors = errors + 1;
end else begin
$display("PASS n=%0d bias=%0d act=%0d: a=%0d b=%0d (bit-exact vs 2x real neural_processor.v)",
n, bias, activation, result_data_a, result_data_b);
if (dut_val_a !== v2a_val || dut_val_b !== v2b_val) begin
$display("FAIL n=%0d bias=%0d act=%0d: v2a=%0d v2b=%0d dut_a=%0d dut_b=%0d MISMATCH",
n, bias, activation, v2a_val, v2b_val, dut_val_a, dut_val_b);
errors = errors + 1;
end else begin
$display("PASS n=%0d bias=%0d act=%0d: a=%0d b=%0d (bit-exact vs 2x real neural_processor.v)",
n, bias, activation, dut_val_a, dut_val_b);
end
end
@(posedge clk);
end
@(posedge clk);
while (!job_ready || np_state !== 4'd0 || !v2a_job_ready || !v2b_job_ready) @(posedge clk);
end