Skip to content

Single-source the MEMW_R fill and the BITWISE type↔column map#791

Merged
MauroToscano merged 1 commit into
perf/tracegen-cpu-optimizationsfrom
review/pr786-single-source-of-truth
Jul 7, 2026
Merged

Single-source the MEMW_R fill and the BITWISE type↔column map#791
MauroToscano merged 1 commit into
perf/tracegen-cpu-optimizationsfrom
review/pr786-single-source-of-truth

Conversation

@MauroToscano

Copy link
Copy Markdown
Contributor

Second follow-up from the #786 review, stacked on #790. Removes every "keep the two in sync"-by-comment pair the optimization introduced, with no behavior change (bucket routing, trace cells, and multiplicities are unchanged).

MEMW_R

  • RegRow and the direct column fill move into memw_register.rs; generate_memw_register_trace becomes a thin wrapper (ops → RegRow::from_memw → shared fill). The unit-tested generator and the production fast path now run the same code, so they cannot drift — previously production ran an untested duplicate while the tests exercised the legacy copy.
  • RegRow::new is the single definition of the row encoding (halved address, masked old_ts_lo); the walk fast path and from_memw both delegate to it.
  • The IS_HALFWORD collector moves next to bus_interactions(), so the lookup each row sends and the lookup counted live in the same module.

Routing

  • New MemwRoute + classify_memw shared by MemwBuckets::push, the push_reg_access fallback (now delegating to push), and count_table_lengths' partition_memw — the sizing pass and trace generation can no longer classify differently.
  • Drops the single-use push_register/push_register_row helpers.

BITWISE

  • BitwiseOperationType::ALL is the single origin of NUM_LOOKUP_TYPES, and type_mu_column(i) = mu_column(ALL[i]) — the arithmetic MU_MSB8 + type_idx shortcut and its column-contiguity assumption are gone rather than guarded.
  • The runtime histogram_column_map_guard test is replaced by a compile-time bijection assert: reordering lookup_type_index or forgetting to extend ALL for a new variant is now a compile error instead of a test failure someone has to remember to run.
  • bump()'s indexing comment corrected: in release an out-of-domain z would silently mis-count in another type's lane rather than panic (the old per-op path panicked); what actually upholds the invariant is that every BitwiseOperation constructor masks or debug-asserts z < 16.

Testing

  • Full cargo test -p lambda-vm-prover --lib: 503 passed, including test_prove_verify_with_memw_register and test_prove_elfs_test_bitwise_8 end-to-end prove+verify.
  • The 5 remaining failures (test_pure_commit_rust etc.) are missing rust-guest ELF fixtures in my environment (make compile-programs-rust) — they fail at file-read before any prover code runs.
  • cargo clippy and cargo fmt --check clean.

MEMW_R:
- Move RegRow and the direct column fill into memw_register.rs;
  generate_memw_register_trace is now a thin wrapper (ops -> RegRow::from_memw
  -> shared fill), so the unit-tested generator and the production fast path
  run the same code and cannot drift
- RegRow::new is the single definition of the row encoding (halved address,
  masked old_ts_lo); the walk fast path and from_memw both delegate to it
- Move the IS_HALFWORD collector next to bus_interactions() so the lookup a
  row sends and the lookup counted derive from the same module

Routing:
- Add MemwRoute + classify_memw, shared by MemwBuckets::push, the
  push_reg_access fallback (which now delegates to push), and
  count_table_lengths' partition_memw — one classification, three consumers
- Drop the single-use push_register/push_register_row helpers

BITWISE:
- BitwiseOperationType::ALL is the single origin of NUM_LOOKUP_TYPES;
  type_mu_column is defined as mu_column(ALL[i]), removing the arithmetic
  column-contiguity assumption instead of guarding it
- Replace the runtime histogram_column_map_guard test with a compile-time
  bijection assert (a mismatch is now a compile error, not a test failure)
- Correct bump()'s comment: in release an out-of-domain z would mis-count in
  another lane rather than panic; constructors' masking upholds the invariant
@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-docs-deadcode branch from a0ba503 to ae7d5d4 Compare July 7, 2026 14:44
Base automatically changed from review/pr786-docs-deadcode to perf/tracegen-cpu-optimizations July 7, 2026 15:21
@MauroToscano MauroToscano merged commit 17ad034 into perf/tracegen-cpu-optimizations Jul 7, 2026
16 of 25 checks passed
@MauroToscano MauroToscano deleted the review/pr786-single-source-of-truth branch July 7, 2026 15:30
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