chore: pipp review#23407
Open
iakovenkos wants to merge 13 commits into
Open
Conversation
iakovenkos
force-pushed
the
lde/zacs-pippenger
branch
from
758407a to
b1fe65a
Compare
iakovenkos
requested review from
IlyasRidhuan,
MirandaWood and
jeanmon
as code owners
iakovenkos
force-pushed
the
si/reviewing_new_pipp
branch
from
89c2f72 to
6a640b6
Compare
Centralise the per-worker Zone W byte walk in pippenger_arena_layout.hpp so the sizer, live allocator, and arena-fit test all share one source of truth (prior drift here caused four historical arena bugs). Move the 270-line pippenger_bn254_arena_layout_fits_for_test helper out of scalar_multiplication.cpp into the test TU and expose compute_arena_bytes_for_msm via the header (BN254 explicitly instantiated).
The sizer (outer + per-schedule lambda) and the live allocator each carried their own copy of the per_window_bytes / phase_one_prologue_bytes / phase_a caps / wpb solver formulas — 5 duplicates total. Replace the three copies with shared free functions in pippenger_arena_layout.hpp (compute_per_window_bytes, compute_phase_one_prologue_bytes, compute_phase_a_caps, compute_dense_stride, compute_bucket_partials_max, compute_global_max_overflow_per_window, solve_wpb). Net -85 lines and the formulas can no longer drift across sites.
…antine.hpp Move the ~360 lines of Constantine slice params, scalar packed-digit recoder, and SIMD x4 specialisations out of scalar_multiplication.cpp into a dedicated pippenger_constantine.hpp under the same round_parallel_detail namespace. Pure code motion.
iakovenkos
commented
May 21, 2026
| #include <wasm_simd128.h> | ||
| #endif | ||
|
|
||
| namespace bb::scalar_multiplication::round_parallel_detail { |
Contributor
Author
There was a problem hiding this comment.
simply moving the constantine logic from the main file
iakovenkos
commented
May 21, 2026
| return bump_alloc_within.template operator()<T>(count, arena_cursor, arena_capacity, 0); | ||
| }; | ||
| const size_t arena_total_bytes = compute_arena_bytes_for_msm<Curve>(n_input, external_glv_provided, dedup_active); | ||
| round_parallel_detail::MsmArena arena(arena_total_bytes, external_arena); |
Contributor
Author
There was a problem hiding this comment.
trying to make this method more readable
iakovenkos
commented
May 21, 2026
| uint8_t empty = 1; | ||
| }; | ||
|
|
||
| template <typename Curve> struct PerWorkerArenaLayout { |
Contributor
Author
There was a problem hiding this comment.
was duplicated across several places
iakovenkos
commented
May 21, 2026
| { | ||
| const size_t saved_threads = bb::get_num_cpus(); | ||
|
|
||
| // CI regression from HonkRecursionConstraintTestWithoutPredicate/2.GenerateVKFromConstraints: |
Contributor
Author
There was a problem hiding this comment.
Inspired by real regression where arena layout was incorrectly computed
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
1 participant
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.
.