Timing model hardening: overlay/execution drift, picosecond stage accounting, classification drift detection

[qichao-arlo-wang]

Follow-up to #98, closing the four known limitations its description listed. One
commit per problem.

1. The overlap model had already drifted from execution (09c250b)

timing_access_for_opcode hand-mirrors the SRAM addressing of the execution arms
in do_ops, with no shared source of truth. Auditing the two side by side turned
up a divergence:

do_ops has zero-time no-op arms — V_RED_SUM { rd: 0 } and
V_RED_MAX { rd: 0 } discard their fp0 write-back and return without touching
vram. The overlay classified those opcodes by shape alone and reported a read
range anyway, and record ran retire_dependent_prefetches unconditionally. A
no-op could therefore retire a prefetch it never read, permanently forfeiting the
chance to hide it and inflating dependent_prefetch_stalls and compute_ops.
The error was conservative rather than flattering. No checked-in ASM emits
V_RED_SUM f0, so this was latent rather than observed — but it is exactly the
drift this mirroring invites, and review found two more instances of it that are
not latent (see §5).

The first attempt keyed on observed elapsed time — "an opcode that consumed no
simulator time performed no SRAM access". Review showed that premise is false
(Cell::resolve_with returns without awaiting on a Ready cell, so SRAM reads
cost nothing and all duration comes from explicit cycle! calls), so a zero
latency config would have silently dropped real dependencies. What shipped is the
explicit mirror: V_RED_SUM { rd: 0 } / V_RED_MAX { rd: 0 } arms matching
do_ops. See §5.

Two structural guards on top:

• The match is now exhaustive — the _ => Other wildcard is gone, so a newly
  added opcode fails to compile until someone classifies it rather than being
  silently absorbed into "touches no SRAM".
• The mirroring invariant is documented on the function, including what neither
  guard catches: a wrong address or extent.

2. H_STORE_V was modelled as a barrier (09c250b)

It mapped to Barrier, which abandoned every pending prefetch. It is really
an SRAM read (src_addr = gp(rd), extent VLEN * STORE_V_AMOUNT) feeding an HBM
write, so a program interleaving stores would get essentially no overlap credit.
(No ASM currently checked into testbench/ contains H_STORE_V, so this too is a
correctness fix ahead of the workloads that need it, not a measured regression.)

Added a Store access kind that retires the prefetches it genuinely depends on
but does not hide pending prefetch time. That last part is a conservative choice,
not a derived one: the model already lets a prefetch issued before a store survive
across it and be hidden by a later compute, which assumes the prefetch is in
flight while the store runs — so "they contend for DMA" would equally forbid
that. Without a bandwidth model there is no principled answer, and declining to
credit the store's own duration is the direction that cannot overstate achievable
overlap. C_BREAK stays a real barrier.

The module docs now state plainly that the result is not a bound in either
direction: optimistic on contention (no DMA bandwidth, queue depth, SRAM port
or HBM channel modelling, unbounded pending queue), pessimistic on barriers and
stores, and RAW-only so writes by compute are invisible. It is useful for
relative comparison between programs on the same model, not as a hardware figure.

3. Stage profile time rounded per opcode (8afaf40)

StageProfiler rounded every opcode up to a whole clock period before adding
it to a bucket, so n sub-cycle opcodes billed n cycles instead of
ceil(n * duration / period). Since Σ div_ceil(tᵢ) ≥ div_ceil(Σ tᵢ), per-stage
cycles and total_profiled_cycles would be systematically inflated.
timing_overlay was hardened against precisely this bias in #98; the profiler was
not.

StageRuntime and ResourceRuntime now accumulate picoseconds and round once per
reported quantity. Every level emits both *_picos (exact, additive) and
*_cycles (the rounded display view). They cannot both be additive — buckets
round independently, so bucket cycles can exceed the parent's by up to one cycle
each, and a set of n siblings by up to n-1 — so PROFILE_CAVEAT and a new
time_unit_status say it outright: do arithmetic on picos, display cycles.
time_fraction is computed from picos so per-stage fractions sum to exactly 1;
the old cycle_fraction became the same number and was dropped. A new
period_picos field lets a consumer holding only the JSON convert between the two
views.

cycle_accounting_status now compares picoseconds, making the verdict exact and
independent of the clock period.

Note the inflation this fixes has never actually been observed. hbm2_preset
gives a DRAM tCK of 1 ns, identical to PERIOD (both measured: period=1.000ns
for HBM2_2Gbps, 833ps for DDR4_2400), so every event lands on a cycle boundary
and div_ceil is exact today. Switching preset or core clock breaks that, and
cycle_accounting_status would then have reported a mismatch for pure rounding
reasons, indistinguishable from a real accounting hole. When it does break, the
error concentrates in stages with many short opcodes — the per-opcode rounding
error is at most one cycle regardless of opcode length — not in the long DMA ops.

Schema bumped to v3.

4. The classification guard could not see misclassification (14ce854)

#98's guard measures unclassified_fraction, which only catches a compiler
comment rename that matches nothing and drops opcodes into Other. The other
failure mode is invisible to it: a renamed comment that happens to match a
different rule in classify_comment's priority chain keeps the opcodes classified
— just in the wrong stage — and the unclassified fraction never moves. Given how
much that chain leans on conjunctions, one negation, and stateful carry-over
("vram matrix mul" appears in two branches, resolved only by order), that is a
realistic outcome of an innocuous upstream rename.

Declared the classifier's comment substrings as STAGE_VOCABULARY data and report
which of them the ASM actually contained. A rename now surfaces as a term
disappearing from vocabulary_terms_present, whatever it re-matched. The list is
for reporting only — classify_comment keeps the rules, since a flat table cannot
express them.

gpt_oss_moe_expert_test (already in CI) asserts these guards. The vocabulary
check is a subset test — emitting an extra recognised comment is harmless,
dropping one is the drift being hunted. The routing/gather/scatter terms are
legitimately absent from this single-expert program. Review then showed this pair
is still not enough; see §5.

Also added classify_comment_pins_the_stage_vocabulary, locking in behaviour on
representative real comment lines including the order-sensitive and stateful
cases, so inserting or reordering a rule cannot silently re-home opcodes that
already matched an earlier one.

5. Review round (afb66bd, ad76d58)

Five reviewers went over the three commits above. Their confirmed findings are
fixed in two follow-up commits, split by provenance so the pre-existing defects
can be cherry-picked or reverted independently of the corrections to this PR's
own work.

afb66bd — defects that predate this PR

• M_MM_WO / M_MV_WO really do read VRAM. They are read-modify-writes, so
  the empty read set (and the comment defending it) was wrong, and the write-out's
  own duration was being used to hide a prefetch it depended on. This fires on
  gpt_oss_moe_expert, the program in CI.
• V_TOPK reads two VLEN rows under the 128-expert policy, not one.
• classify_comment had a dead rule mis-attributing ~25% of opcodes. Its
  route-weight branch claimed "vram matrix mul", a string emitted by the
  compiler's general-purpose matrix helper. That made the guarded copy below it
  unreachable and billed 53 instructions to expert_route_weight in a program
  with no routing. Removed, and guarded by the new stage_instruction_counts
  signal, since neither existing check can see a rename that re-homes opcodes into
  a different existing stage.

ad76d58 — corrections to this PR's own commits

• The zero-elapsed guard's premise was false. SRAM reads resolve a Ready
  cell without awaiting, so elapsed time tracks cycle! calls, not memory
  traffic; a zero latency config would have dropped real dependencies. Replaced
  with explicit no-op arms.
• resource_accounting_status was a tautology, as were
  total_stage_wall_*; cycle_fraction and time_fraction had collapsed into
  the same number. Removed, with the invariant kept as a debug_assert.
• Retired and barrier-discarded prefetch time was reported nowhere. Added
  retired_prefetch_picos / discarded_prefetch_picos, with a test that every
  issued picosecond lands in exactly one bucket, and split the stall counter by
  cause so it does not jump across the Barrier → Store change.
• Neither timing_access_for_opcode nor StageProfiler::record → to_json
  had any test. Both changes were revertible with the suite green. Both now
  covered, plus a CI assertion on cycle_accounting_status — the only thing that
  catches a caller passing cycles instead of picoseconds.
• Vocabulary completeness is now checked by parsing classify_comment's own
  literals rather than relying on author diligence; extract_pair_id's
  pair= / step6_pair are declared and reported; presence is scanned over
  comment lines only. Dropped _sigmoid from the expected set — it is in the ASM
  only because this test names a tensor gate_sigmoid, and its rule is a
  conjunction that never fires here.

Negative tests

Every fix was verified by reintroducing the defect and confirming a specific
failure, since the reviewers' central criticism was that these changes were
silently revertible:

reverted	caught by
classify_comment dead rule	CI distribution guard, {'expert_route_weight': 53}
caller passes cycles not picos	CI accounting guard, profiled 4516 ps of 4516000 ps
H_STORE_V → Barrier	store_reads_the_region_it_drains_rather_than_acting_as_a_barrier
M_MM_WO → empty read set	vector_write_out_ops_read_their_destination_row
V_TOPK → single row	topk_reads_every_row_the_expert_policy_spans

PR #97 rebase notes

#97 will be rebased onto this. Its emulator-side commit (124986e) is a
superseded duplicate of 2539533 and should be dropped; what survives is the
moe_timing/ testbench. Every field access in it was checked against the v3
output — the exact list, verified line by line:

file:line	access	status under v3
replay/utils.py:121	total_simulation_cycles	OK
replay/utils.py:122	total_stage_wall_cycles	removed (was always equal to total_profiled_cycles) — returns None. Use total_profiled_picos.
replay/utils.py:123	cycle_accounting_status	key survives, values renamed (profiled_cycles_match_total → profiled_time_matches_total)
replay/utils.py:134	resource_proxy_cycles	key survives; switch to resource_proxy_picos for anything arithmetic
replay/utils.py:140	wall_cycles	key survives; use wall_picos if summed
replay/utils.py:141	cycle_fraction	removed — returns None. Renamed to time_fraction (same value, now exact).
replay/utils.py:152	resources.get("ramulator_proxy")	removed in #98 — returns None. It was definitionally equal to dma.
qwen/export_pilot_results.py:36	total += stage.get("wall_cycles")	the cross-level cycle arithmetic v3 forbids. Five stages, each rounded independently, so routing_tax_cycles over-reports by up to 4. Sum wall_picos, round once.
qwen/export_pilot_results.py:92,94,95	pass-through	inherits the above
campaign/run_subset.py:1262	Counter(cycle_accounting_status)	no crash, but histogram keys become disjoint across the merge
replay/timing_validation_gates.py:240-241	== total_stage_wall_cycles	always False now (field removed). Compare total_profiled_picos against total_simulation_picos instead — that is exactly what cycle_accounting_status reports.

Also re-run timing_validation_gates.py's g2_prefetch_compute_overlap after the
rebase: this PR changes the overlay (Store, explicit no-op arms, split stall
counters), and that gate is a hard pass/fail on overlay_hidden_cycles > 0. The
microbenchmark has no stores and no zero-duration compute, so it should hold, but
it has not been re-run against these commits.

Still not fixed## Still not fixed

• The overlay still hand-mirrors SRAM addressing. Neither new guard catches a
  wrong address or extent. The real fix is an access descriptor owned by
  op::Opcode and consumed by both paths.
• The overlap model still does not execute prefetches concurrently. Making it do
  so means rebuilding the determinism guarantees Deterministic emulator timing, stage profile v2, and experimental overlap model #98 established, under
  concurrency.
• Stage classification is still string-matching on compiler ASM comments. Both
  guards detect drift; neither removes the coupling. That needs the compiler to
  emit a structured stage map.

Verification

• cargo build --release — clean, no warnings
• cargo test --workspace --release — all suites pass (main binary 59 → 89 tests)
• cargo fmt --all -- --check — clean
• ruff format --check . / ruff check . — clean
• just test-routed-moe-expert — PASSED, 12.2% unclassified, 8/27 vocabulary terms present, distribution {expert_weight_prefetch=9, expert_projection=831, expert_activation=222, other=25}
• Negative test: reintroducing the classify_comment bug makes the new
  distribution guard fail with {'expert_route_weight': 53}
• Emitted stage_profile.json checked by hand: schema_version: 3,
  total_profiled_picos == total_simulation_picos (4516000),
  profiled_time_matches_total, resource_buckets_sum_to_profiled_time, resource
  picos sum exactly, cycle_fraction sums to 1.0

🤖 Generated with Claude Code