Skip to content

Feature/split bitwise byte op#577

Draft
jotabulacios wants to merge 8 commits into
mainfrom
feature/split-bitwise-byte-op
Draft

Feature/split bitwise byte op#577
jotabulacios wants to merge 8 commits into
mainfrom
feature/split-bitwise-byte-op

Conversation

@jotabulacios
Copy link
Copy Markdown
Collaborator

Splits the unified BITWISE precomputed table into BITWISE (20-bit lookups: Zero/IsB20/Hwsl) and a new BYTE_OPS table (byte-pair lookups: AndByte/OrByte/XorByte/IsByte/IsHalf/Msb16), and peels AND/OR/XOR rows out of the unified CPU table into a dedicated CPU_BITWISE chip. The CPU chip can then drop the unified BusId::Bitwise sender from its bus_interactions, so non-bitwise CPU rows stop paying aux EF cells for that bus

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

Codex Code Review

Findings

  • High Security: CPU_BITWISE is created with no transition constraints, even though it now contains real AND/OR/XOR instruction rows. See test_utils.rs and the split in trace_builder.rs. This drops CPU constraints for bitwise instructions, including arg1/arg2 construction, rvd == res, extension bits, selector bitness, and next-PC checks. Bus lookups alone do not replace those row-local CPU constraints. Reuse create_all_cpu_constraints() for CPU_BITWISE, or add a constrained CPU-bitwise-specific AIR that proves the same required CPU invariants.

  • Medium Bug/Performance: cpu_bitwise is generated as one unsplit trace with cpu::generate_cpu_trace(&bitwise_cpu_ops) in trace_builder.rs, ignoring max_rows.cpu. Programs with many bitwise instructions can create a very large singleton CPU-shaped table, bypassing the existing chunking limit and causing avoidable prover memory/runtime spikes. It should be chunked like cpus, with verifier metadata updated if multiple chunks are needed.

  • Low Bug: BusId::Bitwise was inserted into the enum, but TryFrom<u64> was not renumbered consistently. At types.rs, 3 still maps to AndByte even though BusId::Bitwise as u64 is now 3; 22 maps to Bitwise. Currently this appears debug-facing, but it will mislabel/parse bus IDs and is a trap for future use.

Tests not run; review was based on the requested PR diff.

22 => Ok(BusId::Bitwise),
other => Err(other),
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High — TryFrom<u64> is completely out of sync with From<BusId> for u64

From<BusId> for u64 is id as u64 — it uses the enum discriminant. Because Bitwise was inserted before AndByte without an explicit value, the auto-assigned discriminants shifted:

Variant Old discriminant New discriminant (after PR)
Bitwise 3
AndByte 3 4
OrByte 4 5
CommitNextByte 20 21
Commit 21 22

TryFrom maps 3 ⇒ AndByte and 22 ⇒ Bitwise, but From now emits 3 for Bitwise and 22 for Commit. The two impls are inverses of completely different mappings.

While TryFrom isn't on the core proof path, it's used in filtering helpers and any future serialisation. Any round-trip gives a wrong BusId for every variant ≥ position 3.

Fix: append Bitwise at the end of the enum with an explicit discriminant:

Commit = 21,
Bitwise = 22,   // ← explicit, no shift to existing variants

Comment thread prover/src/instruments.rs
entry.4 += 1;
// tt.aux_cols may differ across instances of a split table; track the
// last value seen (split tables share aux_col count by construction).
entry.1 = tt.main_cols;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — comment says "aux_col count" but updates main_cols

entry.1 is the main_cols slot. The comment is a copy-paste error: it says aux_col count but both the field updated and the value assigned are main_cols.

Suggested change
entry.1 = tt.main_cols;
// main_cols is identical for every chunk of a split table.
entry.1 = tt.main_cols;

@jotabulacios
Copy link
Copy Markdown
Collaborator Author

/bench

@github-actions
Copy link
Copy Markdown

Benchmark — fib_iterative_8M (median of 3)

Table parallelism: auto (cores / 3)

Metric main PR Δ
Peak heap 52994 MB 45594 MB -7400 MB (-14.0%) 🟢
Prove time 25.254s 23.160s -2.094s (-8.3%) 🟢

🎉 Improvement detected — heap or time decreased by more than 5%.

✅ Low variance (time: 0.4%, heap: 1.2%)

Commit: 2d6a26a · Baseline: built from main · Runner: self-hosted bench

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