fix(poly): drop wrong packing-width assert on the middle eval slice - #266
Merged
TomWambsgans merged 3 commits intoAug 14, 2026
Merged
Conversation
`base_eval_eq_packed_with_packed_output` asserted `log_packing_width <= eval_points.len()`, but `eval_points` is the *middle* slice produced by `par_eval_eq`, not the full point: the `log_packing_width` suffix is already folded into `eq_evals` and the `log_chunks` prefix into the packed scalar. Its length is therefore `n - log_packing_width - log_chunks` and has nothing to do with the packing width. The callers only guarantee it is at least 2, while the assert demanded at least `log_packing_width`, so every `n` in `[lpw + log_chunks + 2, 2*lpw + log_chunks)` panicked in debug builds. On a 32-thread AVX512 host that is log_chunks=7, lpw=4, so 13- and 14-variable polynomials aborted `test_packed_eval_eq` and `lean_prover`'s `test_small_memory`. The band is machine-dependent and non-empty on any target with a packing width above 4. The invariant is real, but it belongs to the callers, which already check it against the full point (`compute_eval_eq_base_packed` and `compute_eval_eq_base_packed_batched`). This restores 5cf504a, which removed the same assert for the same reason and was reverted by e45a0ed. It regressed because no test covered the band, so add one that computes the bounds from the runtime thread count and SIMD width rather than hardcoding them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test added in the previous commit derives its `n_vars` band from `parallel_split()` and the packing width. With packing width 4 (arm64 NEON) that band collapses to a single `n_vars` whose middle slice is exactly `log_packing_width`, which the old assert accepted, so the test passes with the assert restored. It does not cover the bug on the machines this repo is developed on. The band was not uncovered before either: `test_packed_eval_eq` already loops `n_vars` from `log_packing_width` to 20 through `compute_eval_eq_base_packed`, which is why the assert aborted it on a 32-thread AVX512 host. What let the assert come back in e45a0ed is that `cargo testall` and CI both run `--release`, where `debug_assert!` is compiled out. So call `base_eval_eq_packed_with_packed_output` directly on slices of length 1 to 3 and compare against its unpacked-output twin `base_eval_eq_packed`. Those are exactly the hardcoded arms the assert made unreachable, and the check no longer depends on the thread count or SIMD width, so it fails on arm64 too. It still only fires in a debug build; gating it in CI would need a debug-assertions job. Also trim the comment at the fix site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TomWambsgans
marked this pull request as ready for review
August 14, 2026 20:14
Nightly clippy (1.99.0-nightly, 2026-08-13) extended `clippy::double_must_use` to `impl Trait` return types, so `to_ext_lanes` and `packed_ext_powers_capped` now fail the CI lint job under `-Dwarnings`. `Iterator` is already `#[must_use]`, so the attribute never added anything and dropping it changes no behaviour. These are the only two occurrences in the workspace. The failure is independent of the branch it appears on: it breaks `main` and every open PR, and because `field` fails to lint, no downstream crate is reached at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
|
grazie mille |
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
2 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.
(Not reviewed yet, working on another branch and pulled out this fix)