fix: neural_director_packed.v SV literal for synth; exonerate it from a testbench race (EXP-0073)
Replaced three uses of the SystemVerilog '0 self-sizing literal with explicit-width zero-fill so the file synthesizes under Vivado's synth_design (which has no -sv equivalent in this flow), needed while adding this module to the real in-context P&R project. Re-running its isolated regression after that edit surfaced 3/8 failures. Root-caused via git stash (reproduces on the untouched committed file, not caused by this edit) and a DUT-internal $display: tb_neural_director_packed.v's own submit_job task drove DUT inputs with blocking assignment across two separate @(posedge clk) waits, racing the DUT's own always block under Icarus and causing a spurious duplicate enqueue. Fixed by switching to nonblocking assignment (race-free by construction). neural_director_packed.v itself was correct all along - 8/8 tests pass after the testbench fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MUG92aM9m68TRc4rG55BcC
This commit is contained in:
@@ -4475,3 +4475,81 @@ XDC pin/timing constraints takes priority over further integration
|
||||
testing -- every P&R so far in this project has been out-of-context
|
||||
synthesis without real board I/O timing, which is not yet a
|
||||
trustworthy signoff number.
|
||||
|
||||
EXP-0073 -- neural_director_packed.v cleared of suspected pairing bug:
|
||||
root-caused as an Icarus-specific testbench race, not an RTL defect
|
||||
(2026-09-19, same autonomous continuation, discovered while preparing
|
||||
real P&R sources)
|
||||
|
||||
CONTEXT: while adding neural_director_packed.v to the real Vivado
|
||||
project for in-context P&R, Vivado's synth_design rejected its three
|
||||
uses of the SystemVerilog `'0` self-sizing literal (plain Verilog-2001
|
||||
mode, same class of issue as EXP-0070's xvlog -sv requirement, but
|
||||
this time in synth_design itself, which has no -sv-equivalent flag in
|
||||
this flow). Fixed by replacing all three `'0` with explicit-width
|
||||
{$clog2(N_SLOTS){1'b0}} (semantically identical, portable). Re-running
|
||||
this module's own isolated regression (tb_neural_director_packed.v)
|
||||
after that edit, to confirm no behavioral change, surfaced 3/8 FAILING
|
||||
tests -- xb/rb/nb (the "B" job's fields in a pair) landing equal to
|
||||
the "A" job's fields instead of their own.
|
||||
|
||||
INVESTIGATION (root-cause discipline, not guessing): confirmed via
|
||||
`git stash` that the SAME failures reproduce on the untouched,
|
||||
already-committed neural_director_packed.v -- ruling out the width-
|
||||
literal edit as the cause. Instrumented the DUT's own clocked always
|
||||
block directly (a $display inside neural_director_packed.v itself,
|
||||
avoiding any separate-process sampling race in the debug harness) and
|
||||
found job_in_valid sampled as HIGH on TWO CONSECUTIVE clock edges from
|
||||
a SINGLE submit_job() call, both times still carrying the FIRST
|
||||
submitted job's x_base -- a spurious duplicate enqueue, not a Director
|
||||
defect. Traced to tb_neural_director_packed.v's own submit_job task:
|
||||
it drives job_in_valid/job_in_x_base/etc with BLOCKING assignment (=)
|
||||
immediately after `@(posedge clk)`, then clears them after a SECOND
|
||||
`@(posedge clk)` separated by a while-loop that (in the common case)
|
||||
executes zero iterations. Icarus does not consistently order "a
|
||||
process resuming from @(posedge clk) and executing a blocking write"
|
||||
against "the DUT's own always @(posedge clk) block reading that same
|
||||
signal" when both wake on the identical edge -- and this ordering was
|
||||
observed to differ between the SET edge (testbench appears to win,
|
||||
DUT sees the new value immediately) and the CLEAR edge (DUT appears
|
||||
to win, sampling the stale value one extra time) within the SAME
|
||||
submit_job() call, producing the duplicate-enqueue artifact.
|
||||
|
||||
FIX: rewrote submit_job to drive all DUT inputs with NONBLOCKING
|
||||
assignment (<=) instead of blocking, which removes the race by
|
||||
construction (NBA updates commit strictly after the Active region
|
||||
where the DUT's own always block runs, so the DUT is GUARANTEED to
|
||||
see the OLD value at the driving edge, never a same-edge stale-or-new
|
||||
ambiguity). Re-ran the full original test suite: 8/8 tests, 0 errors,
|
||||
including the previously-failing TEST1 field-pairing check and
|
||||
TEST2/2b's mismatched-w_base stall behavior.
|
||||
|
||||
SCOPE CHECK (not fixed today, documented instead): the same blocking-
|
||||
assignment-then-while-loop-then-second-wait idiom appears in numerous
|
||||
other testbenches across this project (grep found 15 files, V2 and
|
||||
V3). Checked the one that matters most for trust -- hardware/v3/sim/
|
||||
tb_n2_system_ddr3.v (EXP-0070's real-DDR3 N=2 system milestone) uses
|
||||
the IDENTICAL vulnerable pattern, but that test was compiled/run
|
||||
through Xilinx's xsim, not Icarus, and produced a correct, bit-exact
|
||||
8/8 result -- meaning xsim's own scheduler did not hit this particular
|
||||
race (a different, but equally LRM-legal, resolution of the same
|
||||
unspecified ordering). EXP-0070's result therefore stands as
|
||||
genuinely verified for the real run that produced it, but the
|
||||
underlying idiom is confirmed fragile/simulator-dependent and should
|
||||
not be trusted blindly in any FUTURE Icarus-based testbench.
|
||||
|
||||
DECISION: neural_director_packed.v is fully exonerated -- its
|
||||
pairing/dispatch/stall logic was correct all along. The nonblocking-
|
||||
assignment idiom (this fix) is now the preferred pattern for driving
|
||||
DUT inputs in this project's future testbenches; existing passing
|
||||
testbenches using the old blocking idiom are not being mass-rewritten
|
||||
today (out of scope, and several were verified via xsim where the
|
||||
race did not manifest), but this log entry exists so a future
|
||||
Icarus-based test that shows a similarly-shaped "shuffled/duplicated
|
||||
fields" failure is investigated as a testbench race FIRST, not
|
||||
assumed to be an RTL bug.
|
||||
|
||||
next_action: resume the real, in-context Vivado P&R (n2_system_ddr3_top.v
|
||||
+ the real MIG-generated XDC), the task this fix was a prerequisite
|
||||
for -- neural_director_packed.v's `'0`-literal fix must propagate into
|
||||
that synthesis run.
|
||||
|
||||
@@ -115,7 +115,7 @@ module neural_director_packed #(
|
||||
reg [$clog2(N_SLOTS)-1:0] free_slot_idx;
|
||||
integer fi;
|
||||
always @(*) begin
|
||||
free_slot_idx = '0;
|
||||
free_slot_idx = {$clog2(N_SLOTS){1'b0}};
|
||||
for (fi = N_SLOTS-1; fi >= 0; fi = fi - 1) begin
|
||||
if (slot_free[fi]) free_slot_idx = fi[$clog2(N_SLOTS)-1:0];
|
||||
end
|
||||
@@ -152,7 +152,7 @@ module neural_director_packed #(
|
||||
reg [$clog2(N_SLOTS)-1:0] done_slot_idx;
|
||||
integer di;
|
||||
always @(*) begin
|
||||
done_slot_idx = '0;
|
||||
done_slot_idx = {$clog2(N_SLOTS){1'b0}};
|
||||
for (di = N_SLOTS-1; di >= 0; di = di - 1) begin
|
||||
if (slot_job_done[di]) done_slot_idx = di[$clog2(N_SLOTS)-1:0];
|
||||
end
|
||||
@@ -178,7 +178,7 @@ module neural_director_packed #(
|
||||
slot_node_id_b_r[fi] <= 16'b0;
|
||||
end
|
||||
job_out_done <= 1'b0;
|
||||
job_out_slot <= '0;
|
||||
job_out_slot <= {$clog2(N_SLOTS){1'b0}};
|
||||
end else begin
|
||||
for (fi = 0; fi < N_SLOTS; fi = fi + 1) slot_job_start_r[fi] <= 1'b0;
|
||||
job_out_done <= 1'b0;
|
||||
|
||||
@@ -113,18 +113,38 @@ module tb;
|
||||
|
||||
integer errors, tests;
|
||||
|
||||
// Drives DUT inputs with NONBLOCKING assignment (<=), not blocking
|
||||
// (=). Root-caused this session: the previous blocking-assignment
|
||||
// version raced neural_director_packed.v's own posedge-triggered
|
||||
// always block -- Icarus does not consistently order "testbench
|
||||
// process resumes from @(posedge clk) and executes a blocking
|
||||
// write" against "DUT's always @(posedge clk) block reads that
|
||||
// same signal" when both wake on the SAME edge, and the ordering
|
||||
// was observed to differ between the SET edge and the CLEAR edge
|
||||
// within the same task call (confirmed via a DUT-internal $display
|
||||
// showing job_in_valid sampled as 1 on TWO consecutive edges from
|
||||
// a single submit_job call, both times with the FIRST job's stale
|
||||
// x_base -- a spurious duplicate enqueue, not a Director bug: the
|
||||
// committed neural_director_packed.v was re-verified bit-identical
|
||||
// via the same test with this fix applied). Nonblocking assignment
|
||||
// removes the race entirely: NBA updates land strictly after the
|
||||
// Active region where the DUT's own always block runs, so the DUT
|
||||
// always samples the OLD value at the driving edge and the NEW
|
||||
// value only from the NEXT edge onward -- deterministic by the
|
||||
// language, not by scheduler luck.
|
||||
task automatic submit_job(
|
||||
input [ADDR_WIDTH-1:0] xb, input [ADDR_WIDTH-1:0] wb,
|
||||
input [15:0] nt, input [ADDR_WIDTH-1:0] resaddr, input [15:0] nid
|
||||
);
|
||||
begin
|
||||
@(posedge clk);
|
||||
job_in_x_base = xb; job_in_w_base = wb; job_in_n_tiles = nt;
|
||||
job_in_result_addr = resaddr; job_in_node_id = nid;
|
||||
job_in_valid = 1'b1;
|
||||
job_in_x_base <= xb; job_in_w_base <= wb; job_in_n_tiles <= nt;
|
||||
job_in_result_addr <= resaddr; job_in_node_id <= nid;
|
||||
job_in_valid <= 1'b1;
|
||||
@(posedge clk);
|
||||
while (!job_in_ready) @(posedge clk);
|
||||
job_in_valid <= 1'b0;
|
||||
@(posedge clk);
|
||||
job_in_valid = 1'b0;
|
||||
end
|
||||
endtask
|
||||
|
||||
|
||||
Reference in New Issue
Block a user