Bit-sliced storage for finite-field vectors#251
Open
JoeyBF wants to merge 26 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds bit-sliced field kernels and group-layout trait support, then updates field implementations, vector and iterator accessors, matrix conversion, and serde expectations to use the new storage layout. ChangesBit-sliced prime field arithmetic
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
119-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert equal slice lengths before group addition.
add_bitslicedderives all source group ranges fromself’s length. Ifotheris shorter, this can read outsideother’s logical slice and add unrelated lanes or panic.Proposed fix
pub fn add(&mut self, other: FqSlice<'_, F>, c: FieldElement<F>) { assert_eq!(self.fq(), c.field()); assert_eq!(self.fq(), other.fq()); + assert_eq!(self.as_slice().len(), other.len()); if self.as_slice().is_empty() { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/crates/fp/src/vector/impl_fqslicemut.rs` around lines 119 - 276, `add_bitsliced` and `add_bitsliced_shifted` currently compute source group ranges from `self.as_slice().len()` without verifying `other` has the same logical length, which can cause out-of-bounds reads or incorrect cross-lane addition. Add an explicit slice-length equality check at the start of `FqSliceMut::add` or `add_bitsliced` (alongside the existing field assertions) so `self`, `other`, and the derived group spans are guaranteed to match before any calls to `add_group_masked`, `add_groups`, or the shifted-plane path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/crates/fp/src/vector/impl_fqslicemut.rs`:
- Around line 344-355: The shift-handling branch in impl_fqslicemut.rs can
underflow when `shift` is greater than the current slice length because
`new_len` is computed from `self.as_slice().len() - shift` without validation.
Update the `self.fq().is_bitsliced()` path to explicitly handle `shift >= len`
in `shift_left`/the relevant slice-shifting logic by either early-returning with
an empty result or clamping the length before the gather/scatter loop, and make
sure the `end_mut` adjustment cannot subtract past zero.
---
Outside diff comments:
In `@ext/crates/fp/src/vector/impl_fqslicemut.rs`:
- Around line 119-276: `add_bitsliced` and `add_bitsliced_shifted` currently
compute source group ranges from `self.as_slice().len()` without verifying
`other` has the same logical length, which can cause out-of-bounds reads or
incorrect cross-lane addition. Add an explicit slice-length equality check at
the start of `FqSliceMut::add` or `add_bitsliced` (alongside the existing field
assertions) so `self`, `other`, and the derived group spans are guaranteed to
match before any calls to `add_group_masked`, `add_groups`, or the shifted-plane
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6505ca49-17e3-4b25-8901-54e682a63b8f
📒 Files selected for processing (12)
ext/crates/fp/src/field/bitslice.rsext/crates/fp/src/field/field_internal.rsext/crates/fp/src/field/fp.rsext/crates/fp/src/field/mod.rsext/crates/fp/src/field/smallfq.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/iter.rsext/crates/fp/tests/serde_format.rs
Add a standalone, self-contained prototype of bit-sliced (bit-plane) storage for F_p vectors, to validate the performance claim before the larger FqVector refactor. Not wired into the real vector types. - `bitslice_proto::BitSlicedVec`: groups of 64 elements stored across k = ceil(log2 p) bit-planes (one limb per plane per group). - Generic add/scale kernels for any prime: ripple-carry adder + single conditional subtraction of p, and double-and-add scalar multiply. Fully branch-free, no reduction tables. - Hand-written F3 fast circuit (2-plane add + plane-swap negation). - Exhaustive correctness tests against (a + c*b) % p for all small primes, plus pack/unpack roundtrip and F3-vs-generic agreement. - `benches/bitslice.rs`: compares bit-sliced add/scale vs packed FpVector across p = 3,5,7,251 and lengths 100..100k. This module and bench are temporary and will be removed in Phase 5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Document the prototype benchmark findings. Verdict: GO. The F3 specialized
circuit is ~2.5x faster than packed for add and ~1.75x for scale. The generic
kernel beats packed by 3-4x for large primes (p=251) because packed's odd-prime
reduce is a slow element-wise fallback for everything outside {2,3,5}. This
supports the all-primes/replace strategy: specialized circuits for 3/5/7,
generic kernel for the rest.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Replace the prototype generic kernel's fixed [Limb; 24] scratch and large
by-value returns with k-sized reusable scratch written directly into the
destination planes; replace double-and-add's doubling with a plane shift.
The tightened generic kernel now beats the packed path for all p >= 7
(p=7: 1.3x add / 1.4x scale; p=251: 3.4x add / 4.6x scale at length 100k).
Only the SWAR-tuned p in {3,5} still need specialized circuits, which the F3
circuit confirms wins (2.9x add). Exhaustive correctness tests still pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Replace the runtime-k heap-scratch generic kernel with a dispatch on k = ceil(log2 p) to const-generic implementations (add_groups_k/scale_groups_k and the add_mod_k/double_mod_k/scalar_mul_k helpers). Each monomorphization uses exactly-K-sized stack arrays and fully-unrolled loops tuned to the prime. The dispatch covers k=1..=16 directly and falls back to the heap path for very large primes. Exhaustive correctness tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
The const-generic K dispatch is the decisive win: the generic bit-slice kernel now beats packed for every prime (3.0x add at p=3, up to 24x at p=251) and is competitive with the hand-written F3 circuit. This means the const-K generic kernel can be the single code path for all primes, making per-prime specialized circuits optional polish rather than a requirement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Add entries_per_group/limbs_per_group (packed defaults: entries_per_limb and 1), group_of/lane_of, and gather/scatter for entry-level access. Re-express number() and range() in terms of groups so a future bit-sliced layout can relocate every entry just by overriding the two layout methods. Route FqSlice::entry and FqSliceMut::set_entry through gather/scatter. Behavior is unchanged for the packed layout: full fp test suite (516 lib + 32 integration/doc tests) passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Make FqVector storage uniformly bit-sliced: a group of 64 entries occupies k = ceil(log2 q) limbs (bit-planes), with entry i at bit i of each plane. - field_internal.rs: group-layout defaults (entries_per_group=64, uniform gather/scatter dispersing the encoded value across planes) and element-wise add_groups/scale_groups defaults usable by any field. - field/bitslice.rs: const-generic K-dispatched ripple-carry + conditional- subtract add/scale circuits for prime fields (heap fallback for k>16). - fp.rs: Fp overrides limbs_per_group=ceil(log2 p) and the bulk kernels with the bit-circuit. smallfq.rs: limbs_per_group=bit_length(); uses the element-wise default arithmetic (Zech-log per lane). - vector ops route bulk add/scale through add_groups/scale_groups; entry/iter/ slice/copy_from_slice/is_zero/to_owned and matrix from_vec/to_vec take a bit-sliced path (element-wise via gather/scatter for the irregular cases). - F_2 is k=1, byte-identical to the old packed layout, so its SIMD/BLAS/m4ri paths are untouched. The on-disk/serde byte format changes for odd primes (F_2 unchanged); the p3/p5 golden format tests are regenerated. All 516 fp lib proptests, row_reduce, and serde tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Route group-aligned bit-sliced slice add (both slices starting at lane 0 — the common case: whole vectors and matrix rows) through the fast plane kernel add_groups for the complete groups, with only the <64 trailing entries and non-aligned slices falling back to entry-wise addition. This makes matrix row reduction over odd primes use the bit-circuit instead of per-entry gather/scatter. All fp tests still pass (516 lib + row_reduce + serde + doc). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Row reduction adds rows starting at the pivot column, so both slices share a nonzero lane offset. Use the plane kernel for the interior full groups in that case (only the partial leading/trailing groups go entry-wise), instead of falling back to fully entry-wise addition. Cuts odd-prime row reduction time by ~6x (e.g. p=3 dim 1000: 1.55s -> 235ms). row_reduce test still passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Replace the element-wise leading/trailing partial-group handling in bit-sliced slice add with a masked plane-circuit add (add_group_masked): mask the source group to its in-range lanes and run the kernel, leaving out-of-range lanes untouched. ~2x faster odd-prime row reduction vs the element-wise version (p=3 dim 1000: 235ms -> 111ms). row_reduce test passes. Note: still ~3x slower than the packed baseline for p=3/p=5 (which have hand-tuned SWAR reduce); investigation of the remaining per-add overhead is ongoing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
- Compute the per-plane modulus mask inline from p inside the const-K circuits instead of building a 65-element array on every add_groups call. - Dispatch add_group_masked to an exactly-K-sized masked kernel rather than zero-filling a 64-element scratch array per boundary call. Measured effect on odd-prime row reduction (dim 1000): p=3 111ms -> 66ms, p=5 205ms -> 130ms, p=7 237ms -> 159ms. All fp tests pass. Remaining: p=3/p=5 row reduction is still ~1.85x/2.9x slower than the SWAR-tuned packed path (the circuit's sequential ripple/borrow chains and double-and-add scalar multiply lose to a single packed madd+SWAR-reduce); p>=7 and large primes are faster bit-sliced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Replace the generic ripple-carry+conditional-subtract path for p=3 with a flat 6-gate circuit (two XOR / two XOR / two AND-NOT layers, mapping to x86 andn with short dependency chains) on the (hi,lo) plane encoding; multiplication by 2 = negation is a plane swap. Circuit contributed by the user; verified exhaustively against all 9 valid input pairs and via the existing p=3 proptests. Row reduction at p=3, dim 1000: 66ms (generic bit-slice) -> 28ms, now 1.09x faster than the packed SWAR baseline (30.5ms) instead of 1.85x slower. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Replace the generic ripple-carry+conditional-subtract+double-and-add path for p=5 with flat "indicator" circuits: one-hot lane masks per operand value recombined with no carry/borrow chain, for both add and scalar multiply. This restores the instruction-level parallelism the sequential generic circuit lost. Verified via the existing p=5 proptests and row_reduce. Row reduction at p=5, dim 1000: 106ms -> 80ms. Still ~2.1x slower than the packed baseline (38ms): packed p=5 has a hand-tuned SWAR reduce and packs 12 entries/limb, so its ops-per-entry is lower than the ~70-gate bit-sliced fma. (Unlike p=7+, which has no tuned packed reduce and is faster bit-sliced.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Two correctness bugs that the fp test suite missed but ext's odd-prime resolutions exercised: - FqSliceMut::shl_assign computed the limb shift from the packed entries_per_limb, which is meaningless in the bit-sliced layout and indexed out of bounds. Add a bit-sliced path that shifts entries down via gather/scatter (reading i+shift strictly ahead of writing i keeps it correct in place). - FpVector::padded_len, used to align AugmentedMatrix segments, computed num_limbs * entries_per_limb. For bit-sliced fields that mixes the bit-sliced limb count with the packed entries-per-limb (e.g. padded_len(3, 1) = 42), leaving segments straddling 64-entry groups. Round up to a whole number of groups (entries_per_group) instead; this equals the old value for the packed layout / F2 and correctly group-aligns bit-sliced segments. With these, the full ext test suite passes at odd primes (extend_identity, milnor_vs_adem, resolve_iterate, resolve all green). The pre-existing test_tempdir_lock failure is unrelated (it relies on file read-only bits, which root ignores in this environment; it also fails on the packed baseline). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Every odd-characteristic field is now bit-sliced; only F_2 (k=1) uses the packed bit-shift realignment path (add_shift_none/left/right). Drop the unreachable odd-prime `else` branch in FqSliceMut::add (collapsing to F_2 shift vs add_bitsliced) and remove FpVector::num_limbs, which padded_len no longer calls. The add_shift_* methods are kept: they remain F_2's unaligned slice-add implementation, and routing F_2 through the element-wise bit-sliced fallback would regress the dominant prime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
…ed add) A bit-sliced vector is k independent single-bit planes; realigning two slices with different lane offsets is a per-plane bit shift. That is exactly what the F2-only add_shift_none/left/right did for one plane, so generalize it to k planes: add_bitsliced_shifted builds, per target group, the k source planes shifted into alignment and adds them via the masked group circuit. With this, FqSliceMut::add routes every field through add_bitsliced (F2 is just k=1), and the ~290 lines of add_shift_none/left/right plus AddShiftLeft/RightData are deleted. Add p==2 XOR fast paths to bitslice::add_groups/add_group_masked so F2's aligned slice add stays as cheap as the old add_shift_none. No regression (row reduction, dim 1000, same machine): F2 1.551ms -> 1.543ms (identical within noise), p3/p5/p7 unchanged. Odd-prime *unaligned* slice adds are now faster (shifted circuit instead of element-wise). All fp tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Run cargo +nightly fmt across the touched files, and stop the bitslice_proto module docs from linking to the crate-private `Limb` (rustdoc -D warnings). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Delete the throwaway validation artifacts: src/bitslice_proto.rs, the benches/bitslice.rs comparison bench, and benches/BITSLICE_PROTO_RESULTS.md, along with their lib.rs module declaration and Cargo.toml [[bench]] entry. The real bit-sliced implementation lives in field/bitslice.rs and the vector layer. Also silence clippy under -D warnings: replace two manual plane-shift copy loops in field/bitslice.rs with copy_from_slice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
- bitslice.rs: use `as_chunks`/`as_chunks_mut` for const-size group iteration (clippy `chunks_exact_to_as_chunks`, denied under `-D warnings`). - field_internal.rs: fix broken intra-doc links to `Self::gather`/`Self::scatter`. - impl_fqslicemut.rs: assert equal slice lengths in `add`, and guard `shl_assign` against `shift >= len` underflow (empties the slice). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
The `as_chunks`/`as_chunks_mut` iterator chains exceeded the line width; apply `cargo +nightly fmt` so `cargo fmt --all -- --check` passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
JoeyBF
force-pushed
the
claude/finite-field-bit-storage-fyb15c
branch
from
2f4f82f to
08c9ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crates/fp/src/vector/impl_fqvector.rs (1)
249-256: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRoute
add_truncatethrough the group layout.
self.limbs()now stores bit-planes for fields withlimbs_per_group() > 1, but this still treats each limb as packed entries viafma_limb/truncate. Callingadd_truncateon a bit-sliced vector can mutate planes as if they were packed field elements and return incorrect results. Add a bitsliced branch here, or explicitly reject unsupported layouts before mutating.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/crates/fp/src/vector/impl_fqvector.rs` around lines 249 - 256, `add_truncate` currently assumes `limbs()` are packed field elements and uses `fma_limb`/`truncate` directly, which breaks bit-sliced layouts when `limbs_per_group() > 1`. Update `FqVector::add_truncate` in `impl_fqvector.rs` to either route through the group/bit-sliced layout handling used elsewhere in this type or fail fast before mutating when the layout is unsupported. Make sure the logic distinguishes the `limbs_mut()`/`limbs()` representation from grouped bit-planes and keeps the existing packed-path behavior only for supported layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/crates/fp/src/field/field_internal.rs`:
- Around line 295-298: The Range::range method in field_internal.rs should
return an empty range when start equals end instead of expanding to the
containing limb group. Update the logic in the range(self, start, end) helper so
it preserves empty entry ranges (for example, by checking for start == end
before computing group bounds) and returns an empty limb range that makes
limb_range.is_empty() behave correctly. Keep the fix localized to the Range
implementation used by group_of, limbs_per_group, and number.
In `@ext/crates/fp/src/matrix/matrix_inner.rs`:
- Around line 324-338: Reject ragged input in Matrix::from_vec before entering
either storage path. The bitsliced branch in matrix_inner.rs currently assumes
every row matches input[0].len(), so add a single upfront validation that all
rows have the same width as the first row and return the same error used by the
non-bitsliced path if not. Keep the check near the existing columns/stride setup
so the behavior is consistent for both the row_mut/set_entry loop and the packed
branch.
---
Outside diff comments:
In `@ext/crates/fp/src/vector/impl_fqvector.rs`:
- Around line 249-256: `add_truncate` currently assumes `limbs()` are packed
field elements and uses `fma_limb`/`truncate` directly, which breaks bit-sliced
layouts when `limbs_per_group() > 1`. Update `FqVector::add_truncate` in
`impl_fqvector.rs` to either route through the group/bit-sliced layout handling
used elsewhere in this type or fail fast before mutating when the layout is
unsupported. Make sure the logic distinguishes the `limbs_mut()`/`limbs()`
representation from grouped bit-planes and keeps the existing packed-path
behavior only for supported layouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63dc9d89-0073-4e50-8d05-db72152e7bbe
📒 Files selected for processing (12)
ext/crates/fp/src/field/bitslice.rsext/crates/fp/src/field/field_internal.rsext/crates/fp/src/field/fp.rsext/crates/fp/src/field/mod.rsext/crates/fp/src/field/smallfq.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/iter.rsext/crates/fp/tests/serde_format.rs
- add_truncate: route bit-sliced fields through the group add (their lanes reduce independently and cannot carry, so the truncation check never fails); the packed fma_limb/truncate loop would corrupt bit-planes. F2 (k=1) keeps the packed path. - FieldInternal::range: return an empty limb range for an empty entry range instead of the containing group, so is_empty() guards behave correctly. - Matrix::from_vec: assert all input rows share a width up front, so both the packed and bit-sliced storage paths reject ragged input consistently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
hoodmane
reviewed
Jun 29, 2026
Comment on lines
+32
to
+34
| for (j, m) in masks.iter_mut().enumerate().take(k + 1) { | ||
| *m = if (p >> j) & 1 == 1 { !0 } else { 0 }; | ||
| } |
Contributor
There was a problem hiding this comment.
Feels to me like this would be clearer to write as a loop:
for i in 0..min(k+1, BITS_PER_LIMB + 1) {
if (p >> i) & 1 {
masks[i] = !0;
}
}
Collaborator
Author
There was a problem hiding this comment.
Done in 8e81da3 — rewrote it with the explicit if ... { = !0 } instead of the ternary. I kept masks.iter_mut().enumerate() rather than a bare for i in 0..n { masks[i] = ... }, because the latter trips clippy's needless_range_loop and CI runs clippy with -D warnings. Same shape otherwise.
Generated by Claude Code
Per review: clearer as a loop setting the mask only when the bit is set, rather than a ternary. Keep `enumerate()` (not a bare index range) so clippy's needless_range_loop stays satisfied under -D warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
hoodmane
reviewed
Jun 29, 2026
| /// The full-width lane mask for bit `j` of `p`: all-ones if set, zero otherwise. | ||
| #[inline(always)] | ||
| fn pmask(p: u32, j: usize) -> Limb { | ||
| 0u64.wrapping_sub(((p >> j) & 1) as Limb) |
Contributor
There was a problem hiding this comment.
What's the deal with casting the argument as Limb?
Collaborator
Author
There was a problem hiding this comment.
(p >> j) & 1 is a u32 (since p: u32), and the function returns a Limb (u64), so the bit has to be widened before the negate-to-mask trick (1 -> !0, 0 -> 0). I've simplified it in 4c67bbc to Limb::from((p >> j) & 1).wrapping_neg(), which drops the as cast in favor of the lossless From and reads more directly.
Generated by Claude Code
Per review: replace `0u64.wrapping_sub((... ) as Limb)` with the lossless `Limb::from(bit).wrapping_neg()`, dropping the `as` cast and the literal-u64 inconsistency. Behavior is identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
The bit-sliced storage approach and the optimized 6-gate F3 addition circuit were both contributed by Carl McTague. Acknowledge this in the module docs and on f3_add_planes. Co-Authored-By: Carl McTague <carl.mctague@bc.edu> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Comment-only corrections from a review pass: - impl_fqslicemut.rs: add_bitsliced handles partial groups via the masked plane circuit (add_group_masked), not entry-wise as the doc claimed. - iter.rs: FqVectorNonZeroIterator's `start` is the slice's absolute start, not a group base. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
Remove comments that restate adjacent code (F_2 masked-XOR, the single-group label, the shift-out empty guard, the scale-by-one no-op) and tighten a few verbose ones (pmask, number, the fp.rs bit-sliced-layout note, padded_len) by dropping migration/backwards-compat asides. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFZzmjYg4m8F2SHyFpVqEA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the packed-limb representation of finite-field vectors in the
fpcrate with a bit-sliced (bit-plane) layout. Instead of packing each element
into
ceil(log2(p*(p-1)))consecutive bits of one limb, a group of 64 elementsis stored across
k = ceil(log2 p)limbs (planes): planejholds bitjofall 64 elements. Addition and scalar multiplication become branch-free boolean
circuits over the planes that operate on 64 elements at once, with no separate
reduction step.
F2 is the
k = 1instance of this layout and is byte-identical to the oldpacked representation, so the SIMD / m4ri / BLAS F2 fast paths are untouched.
Acknowledgments
The idea of bit-slicing finite-field vectors, and the optimized 6-gate F3
addition circuit, were both contributed by Carl McTague
(carl.mctague@bc.edu, @mctague434). He is
credited as a co-author on the relevant commit and in the source.
Motivation
add/scaleare the hot path of Gaussian elimination and resolutioncomputation. The old path did a fused multiply-add producing non-reduced limbs
followed by a separate SWAR
reduce. The bit-sliced circuits remove the reducestep entirely and were validated by benchmark before the refactor:
scale vs packed at length 100k).
Kdispatch (exactly-sized stack arrays, unrolledper-prime loops) made the generic kernel beat packed for every prime
(add: ~3× at p=3 up to ~24× at p=251; scale: ~1.1× up to ~36×).
What changed
FieldInternal—entries_per_group,limbs_per_group,group_of/lane_of,gather/scatter,add_groups/scale_groups/add_group_masked,is_bitsliced.number/rangere-expressed in terms of groups. Packed is the degenerate case
limbs_per_group == 1.field/bitslice.rs(new) — const-K kernels (1..=16, heap fallbackbeyond) for add / masked-add / scale, plus hand-written F3 (6-gate) and F5
flat circuits and the generic ripple-carry + conditional-subtract-p /
double-and-add scalar mult.
Fpdelegatesadd_groups/scale_groups/add_group_maskedto thebit-slice kernels;
SmallFquses the same bit-sliced storage with thedefault element-wise (Zech-log) ops.
gather/scatter/add_groups. Slice addition handles equal nonzero laneoffsets via a masked group path and arbitrary offsets via a general
per-plane shifted add (this replaced the old
add_shift_*methods).tests/serde_format.rsregenerated.Testing
field_tests!and the layout-agnostic vector proptests pass againstthe bit-sliced layout.
tests/row_reduce.rsand the wider workspace resolution tests pass.cargo +nightly fmt,cargo doc(-D warnings), and clippy are clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests