Skip to content

Make the u32 value domain a compile-time fact for MemwOperation#792

Merged
MauroToscano merged 6 commits into
perf/tracegen-cpu-optimizationsfrom
review/pr786-u32-domain
Jul 7, 2026
Merged

Make the u32 value domain a compile-time fact for MemwOperation#792
MauroToscano merged 6 commits into
perf/tracegen-cpu-optimizationsfrom
review/pr786-u32-domain

Conversation

@MauroToscano

Copy link
Copy Markdown
Contributor

Third follow-up from the #786 review, stacked on #791. Types-only change; produced traces are byte-identical.

#786 shrank MemwOperation.value/old to [u32; 8] but kept [u64; 8] constructor signatures, narrowing with debug_assert! + as u32 — release builds would silently truncate an out-of-domain value (a bus-imbalance debugging nightmare if a future caller ever passes a full 64-bit word), and the M1/M3/M5 register fast path cast as u32 at its call sites with no assert at all, in any build profile.

This moves the domain to the type system instead of guarding it at runtime:

  • MemwOperation::new and with_old take [u32; 8] directly; the narrowing maps and their debug_asserts are deleted. (old_timestamp stays [u64; 8] — timestamps can reach u64::MAX.)
  • pack_register_value returns u32 limbs, so the fast path's bare casts disappear rather than gaining asserts.
  • MemoryState::read_bytes returns [u32; 8] values (they are u8 memory bytes), and the byte-domain call sites (load, store, commit, keccak, ECSM) build [u32; 8] at the source.
  • LoadOperation keeps its existing u64 interface via a widening map(u64::from) at the boundary.

Out-of-domain values are now unrepresentable in every build profile — the "error cleanly" version of the guard, at zero runtime cost.

Testing

  • make lint passes; cargo clippy --all-targets no warnings; cargo fmt --check clean.
  • Full cargo test -p lambda-vm-prover --lib: 503 passed — identical to the pre-change baseline (the 5 failures are the known missing rust-guest ELF fixtures, failing at file-read before any prover code runs; CI builds them and runs the full suite).

MemwOperation::new and with_old take [u32; 8] directly and
pack_register_value returns u32 limbs, so the u64->u32 narrowing
debug_asserts (and the release-mode silent-truncation hazard behind
them) are deleted rather than guarded: out-of-domain values are now
unrepresentable. The register fast path's bare `as u32` casts, which
skipped even the debug_assert, disappear for the same reason.
…ations

- Collectors now take &mut BitwiseHistogram instead of returning
  Vec<BitwiseOperation>. The heavy sources count with no per-source
  vector at all: MEMW_R bumps one IS_HALFWORD per row (tens of
  millions of rows), PAGE bumps one ARE_BYTES per byte of every
  touched page, and CPU padding collapses to two bump_n calls
  instead of an O(padding_rows) vector of identical zero ops.
- reduce_with replaces reduce(identity, ..) in the rayon tree-reduce,
  eliminating one zeroed 80 MiB identity histogram per reduce leaf.
- RegRow.address shrinks to u16 (register index 0..=255), taking the
  largest persisted array of the walk from 40 to 32 bytes per row.
@MauroToscano MauroToscano force-pushed the review/pr786-single-source-of-truth branch from eedf9d9 to bf2aa3e Compare July 7, 2026 14:44
@MauroToscano MauroToscano force-pushed the review/pr786-u32-domain branch from 19b75b4 to 1287123 Compare July 7, 2026 14:44
@MauroToscano

Copy link
Copy Markdown
Contributor Author

Rebased on the updated branch. Note on 3b6840e (which upgraded the narrowing guards to always-on assert!): this PR resolves the same concern one level deeper — with [u32; 8] at the constructor boundary the assert becomes tautological and is deleted. Out-of-domain values are unrepresentable in every build profile, with no release-mode panic path in the trace-gen hot loop.

Base automatically changed from review/pr786-single-source-of-truth to perf/tracegen-cpu-optimizations July 7, 2026 15:30
MauroToscano and others added 4 commits July 7, 2026 13:45
The bitwise-histogram comments were phrased against the pre-refactor code
("byte-identical to the previous serial .extend() chain", "instead of an
O(n) op vector") — narration that is meaningless once this merges and the
old path is gone. Restate them as standalone rationale: the multiplicities
are order-independent (permutation-invariant bus) so parallel collection is
sound, and the reduce_with note now explains why to prefer it over
reduce(identity) rather than referencing a removed path. Also fix the stale
add_padding_byte_checks doc (it described the pre-shrink 14-op CPU layout).
Repo-wide sweep of the trace-generation code for comments that narrate a
change the next reader can't see, or that contradict the code:

- MemwBuckets doc and collect_all_ops no longer reference "the old two-stage
  partition" / "byte-identical to partition-ing one combined vec" — that
  partition sweep no longer exists anywhere (grep '.partition(' → 0 hits).
  Restated as the current invariant (register-first-then-aligned, deterministic
  insertion order the multiplicity counts rely on).
- Fix the MEMW_R ADDRESS column doc: register index range is 0-255 (x0-x31 plus
  the x254 commit index and x255 PC), not 0-31.
The Bus-14 comment in collect_store_op_from_cpu said the packed value "must
match CPU M7 which sends full rv2 as [lo32, hi32]", but M7 (the CPU's old
inline store MEMW at timestamp+1) was removed in the ALU-bus migration. The
store value now flows CPU -> MEMORY/MEMOP (rv2 as [lo32, hi32]) -> STORE chip
-> MEMW write. Reword to describe that path; the byte-layout requirement it
gestured at is unchanged.
Feed bitwise collectors straight into the histogram; cut reduce allocations
@MauroToscano MauroToscano merged commit 6cb96d8 into perf/tracegen-cpu-optimizations Jul 7, 2026
13 checks passed
@MauroToscano MauroToscano deleted the review/pr786-u32-domain branch July 7, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant