Add portable scalar fallback for packed Goldilocks#1198
Conversation
The goldilocks_avx module was unavailable without AVX2. Add a scalar fallback (PackedGoldilocksScalar) with the same API that works on all platforms, and a PackedGoldilocks type alias that auto-selects AVX2 on x86_64 or scalar elsewhere. Closes lambdaclass#1153
Greptile SummaryThis PR adds a portable
Confidence Score: 4/5Safe to merge; all correctness concerns are style/test-coverage suggestions and no logic bugs were found. The implementation is straightforward — arithmetic is entirely delegated to the existing, well-tested FieldElement type, so the risk of a correctness regression is low. The outstanding items (verbose PartialEq, missing Hash, sparse boundary tests, stale docs) are all P2 suggestions that do not affect correctness or runtime safety. crates/math/src/field/fields/goldilocks_avx/scalar.rs — PartialEq and test coverage could be improved before the file grows further.
|
| Filename | Overview |
|---|---|
| crates/math/src/field/fields/goldilocks_avx/scalar.rs | New file implementing the portable scalar fallback; API mirrors AVX2 but PartialEq is unnecessarily verbose and tests don't cover field-boundary edge cases. |
| crates/math/src/field/fields/goldilocks_avx/mod.rs | Wires in the scalar module and adds the PackedGoldilocks type alias; unconditional pub mod scalar is correct, but the module-level doc comment no longer reflects the scalar path. |
| crates/math/src/field/fields/mod.rs | Removes the x86_64/avx2/avx512 cfg gate so goldilocks_avx is always exposed; intentional and correct given the new scalar fallback. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Consumer imports goldilocks_avx] --> B{Target platform?}
B -- "x86_64 + avx2 feature" --> C[PackedGoldilocks = PackedGoldilocksAVX2]
B -- "All other platforms" --> D[PackedGoldilocks = PackedGoldilocksScalar]
C --> E[AVX2 SIMD: 4 elements x 256-bit registers\navx2.rs]
D --> F[Scalar fallback: 4 elements x element-wise ops\nscalar.rs NEW]
E --> G[Shared API\nnew / from_u64_array / zero / one\nto_array / from_slice / store_to_slice\nsquare / Add / Sub / Mul / Neg / *Assign]
F --> G
H[avx512.rs] -- "x86_64 + avx512f" --> I[PackedGoldilocksAVX512\n8 elements x 512-bit]
J[avx2_ext.rs] -- "x86_64 + avx2" --> K[PackedGoldilocksFp2AVX2\nPackedGoldilocksFp3AVX2]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/math/src/field/fields/goldilocks_avx/scalar.rs
Line: 79-83
Comment:
**Overly verbose `PartialEq`, inconsistent with `PackedGoldilocksAVX2`**
The scalar `PartialEq` calls `<Goldilocks64Field as IsField>::eq(self.0[i].value(), other.0[i].value())` on raw field values, while the AVX2 sibling uses the much simpler `self.0 == other.0`. Since `FieldElement` already implements `PartialEq` correctly (using canonical comparison), both paths are equivalent for well-formed elements — but the verbose form is harder to read and creates a maintenance gap between the two types.
Consider aligning with the AVX2 pattern:
```suggestion
impl PartialEq for PackedGoldilocksScalar {
fn eq(&self, other: &Self) -> bool {
self.0 == other.0
}
}
```
Additionally, since `FieldElement` derives `Hash`, and `PackedGoldilocksScalar` now has a manual `PartialEq`, `Hash` cannot be automatically derived. If someone tries to use `PackedGoldilocksScalar` as a hash-map key they'll get a compile error. Clippy will also fire the `derived_hash_with_manual_eq`-related lint. Either derive both (`#[derive(PartialEq, Eq)]` — which would make the manual impls unnecessary), or also add a manual `Hash` impl to keep the `a == b ⟹ hash(a) == hash(b)` contract explicit.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/math/src/field/fields/goldilocks_avx/scalar.rs
Line: 140-182
Comment:
**Tests only cover small values; no near-modulus boundary tests**
All five unit tests use small, comfortably-in-range integers (`[1,2,3,4]`, `[10,20,30,40]`, etc.). The field modulus for Goldilocks is `p = 2^64 − 2^32 + 1 = 0xFFFF_FFFF_0000_0001`. The scalar implementation delegates to `FieldElement` arithmetic which handles wrapping, but there are no tests verifying that wrapping behaviour is correct for:
- Addition overflow: `(p-1) + 1` should equal `0`
- Subtraction underflow: `0 - 1` should equal `p-1`
- Multiplication near-modulus: `(p-1) * (p-1)` should equal `1`
- Negation: `-(p-1)` should equal `1`
The AVX2 module includes exactly these cases (`test_avx2_overflow_handling`, `test_avx2_sub_underflow`, `test_avx2_neg`, `test_avx2_mul_near_modulus`). Adding equivalent tests to the scalar module — or a cross-check test comparing scalar against AVX2 results for boundary inputs — would give stronger correctness confidence for the fallback path.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/math/src/field/fields/goldilocks_avx/mod.rs
Line: 55-56
Comment:
**Module name `goldilocks_avx` is now misleading**
The containing module (`crates/math/src/field/fields/goldilocks_avx/`) was previously only compiled on x86_64 with AVX features and its name accurately described its contents. Now that it is unconditionally compiled and includes a portable scalar fallback, a consumer importing `goldilocks_avx::PackedGoldilocks` on an ARM or RISC-V target will encounter a name that implies x86 SIMD hardware.
While renaming the module is a separate, potentially larger change, at minimum the module-level doc comment at the top of `mod.rs` should be updated to reflect that it now covers both SIMD and scalar code paths — especially since the `#![doc]` block on lines 1–42 only describes AVX2/AVX-512 and says nothing about the scalar fallback.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add portable scalar fallback for packed ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl PartialEq for PackedGoldilocksScalar { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| (0..WIDTH).all(|i| <Goldilocks64Field as IsField>::eq(self.0[i].value(), other.0[i].value())) | ||
| } | ||
| } |
There was a problem hiding this comment.
PartialEq, inconsistent with PackedGoldilocksAVX2
The scalar PartialEq calls <Goldilocks64Field as IsField>::eq(self.0[i].value(), other.0[i].value()) on raw field values, while the AVX2 sibling uses the much simpler self.0 == other.0. Since FieldElement already implements PartialEq correctly (using canonical comparison), both paths are equivalent for well-formed elements — but the verbose form is harder to read and creates a maintenance gap between the two types.
Consider aligning with the AVX2 pattern:
| impl PartialEq for PackedGoldilocksScalar { | |
| fn eq(&self, other: &Self) -> bool { | |
| (0..WIDTH).all(|i| <Goldilocks64Field as IsField>::eq(self.0[i].value(), other.0[i].value())) | |
| } | |
| } | |
| impl PartialEq for PackedGoldilocksScalar { | |
| fn eq(&self, other: &Self) -> bool { | |
| self.0 == other.0 | |
| } | |
| } |
Additionally, since FieldElement derives Hash, and PackedGoldilocksScalar now has a manual PartialEq, Hash cannot be automatically derived. If someone tries to use PackedGoldilocksScalar as a hash-map key they'll get a compile error. Clippy will also fire the derived_hash_with_manual_eq-related lint. Either derive both (#[derive(PartialEq, Eq)] — which would make the manual impls unnecessary), or also add a manual Hash impl to keep the a == b ⟹ hash(a) == hash(b) contract explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/math/src/field/fields/goldilocks_avx/scalar.rs
Line: 79-83
Comment:
**Overly verbose `PartialEq`, inconsistent with `PackedGoldilocksAVX2`**
The scalar `PartialEq` calls `<Goldilocks64Field as IsField>::eq(self.0[i].value(), other.0[i].value())` on raw field values, while the AVX2 sibling uses the much simpler `self.0 == other.0`. Since `FieldElement` already implements `PartialEq` correctly (using canonical comparison), both paths are equivalent for well-formed elements — but the verbose form is harder to read and creates a maintenance gap between the two types.
Consider aligning with the AVX2 pattern:
```suggestion
impl PartialEq for PackedGoldilocksScalar {
fn eq(&self, other: &Self) -> bool {
self.0 == other.0
}
}
```
Additionally, since `FieldElement` derives `Hash`, and `PackedGoldilocksScalar` now has a manual `PartialEq`, `Hash` cannot be automatically derived. If someone tries to use `PackedGoldilocksScalar` as a hash-map key they'll get a compile error. Clippy will also fire the `derived_hash_with_manual_eq`-related lint. Either derive both (`#[derive(PartialEq, Eq)]` — which would make the manual impls unnecessary), or also add a manual `Hash` impl to keep the `a == b ⟹ hash(a) == hash(b)` contract explicit.
How can I resolve this? If you propose a fix, please make it concise.| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn scalar_add_works() { | ||
| let a = PackedGoldilocksScalar::from_u64_array([1, 2, 3, 4]); | ||
| let b = PackedGoldilocksScalar::from_u64_array([10, 20, 30, 40]); | ||
| let c = a + b; | ||
| assert_eq!(c, PackedGoldilocksScalar::from_u64_array([11, 22, 33, 44])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn scalar_mul_works() { | ||
| let a = PackedGoldilocksScalar::from_u64_array([2, 3, 4, 5]); | ||
| let b = PackedGoldilocksScalar::from_u64_array([10, 10, 10, 10]); | ||
| let c = a * b; | ||
| assert_eq!(c, PackedGoldilocksScalar::from_u64_array([20, 30, 40, 50])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn scalar_sub_works() { | ||
| let a = PackedGoldilocksScalar::from_u64_array([10, 20, 30, 40]); | ||
| let b = PackedGoldilocksScalar::from_u64_array([1, 2, 3, 4]); | ||
| let c = a - b; | ||
| assert_eq!(c, PackedGoldilocksScalar::from_u64_array([9, 18, 27, 36])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn scalar_square_works() { | ||
| let a = PackedGoldilocksScalar::from_u64_array([3, 4, 5, 6]); | ||
| let b = a.square(); | ||
| assert_eq!(b, PackedGoldilocksScalar::from_u64_array([9, 16, 25, 36])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn scalar_neg_works() { | ||
| let a = PackedGoldilocksScalar::from_u64_array([1, 2, 3, 4]); | ||
| let b = -a; | ||
| let c = a + b; | ||
| assert_eq!(c, PackedGoldilocksScalar::zero()); | ||
| } | ||
| } |
There was a problem hiding this comment.
All five unit tests use small, comfortably-in-range integers ([1,2,3,4], [10,20,30,40], etc.). The field modulus for Goldilocks is p = 2^64 − 2^32 + 1 = 0xFFFF_FFFF_0000_0001. The scalar implementation delegates to FieldElement arithmetic which handles wrapping, but there are no tests verifying that wrapping behaviour is correct for:
- Addition overflow:
(p-1) + 1should equal0 - Subtraction underflow:
0 - 1should equalp-1 - Multiplication near-modulus:
(p-1) * (p-1)should equal1 - Negation:
-(p-1)should equal1
The AVX2 module includes exactly these cases (test_avx2_overflow_handling, test_avx2_sub_underflow, test_avx2_neg, test_avx2_mul_near_modulus). Adding equivalent tests to the scalar module — or a cross-check test comparing scalar against AVX2 results for boundary inputs — would give stronger correctness confidence for the fallback path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/math/src/field/fields/goldilocks_avx/scalar.rs
Line: 140-182
Comment:
**Tests only cover small values; no near-modulus boundary tests**
All five unit tests use small, comfortably-in-range integers (`[1,2,3,4]`, `[10,20,30,40]`, etc.). The field modulus for Goldilocks is `p = 2^64 − 2^32 + 1 = 0xFFFF_FFFF_0000_0001`. The scalar implementation delegates to `FieldElement` arithmetic which handles wrapping, but there are no tests verifying that wrapping behaviour is correct for:
- Addition overflow: `(p-1) + 1` should equal `0`
- Subtraction underflow: `0 - 1` should equal `p-1`
- Multiplication near-modulus: `(p-1) * (p-1)` should equal `1`
- Negation: `-(p-1)` should equal `1`
The AVX2 module includes exactly these cases (`test_avx2_overflow_handling`, `test_avx2_sub_underflow`, `test_avx2_neg`, `test_avx2_mul_near_modulus`). Adding equivalent tests to the scalar module — or a cross-check test comparing scalar against AVX2 results for boundary inputs — would give stronger correctness confidence for the fallback path.
How can I resolve this? If you propose a fix, please make it concise.| // Portable scalar fallback (always available) | ||
| pub mod scalar; |
There was a problem hiding this comment.
goldilocks_avx is now misleading
The containing module (crates/math/src/field/fields/goldilocks_avx/) was previously only compiled on x86_64 with AVX features and its name accurately described its contents. Now that it is unconditionally compiled and includes a portable scalar fallback, a consumer importing goldilocks_avx::PackedGoldilocks on an ARM or RISC-V target will encounter a name that implies x86 SIMD hardware.
While renaming the module is a separate, potentially larger change, at minimum the module-level doc comment at the top of mod.rs should be updated to reflect that it now covers both SIMD and scalar code paths — especially since the #![doc] block on lines 1–42 only describes AVX2/AVX-512 and says nothing about the scalar fallback.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/math/src/field/fields/goldilocks_avx/mod.rs
Line: 55-56
Comment:
**Module name `goldilocks_avx` is now misleading**
The containing module (`crates/math/src/field/fields/goldilocks_avx/`) was previously only compiled on x86_64 with AVX features and its name accurately described its contents. Now that it is unconditionally compiled and includes a portable scalar fallback, a consumer importing `goldilocks_avx::PackedGoldilocks` on an ARM or RISC-V target will encounter a name that implies x86 SIMD hardware.
While renaming the module is a separate, potentially larger change, at minimum the module-level doc comment at the top of `mod.rs` should be updated to reflect that it now covers both SIMD and scalar code paths — especially since the `#![doc]` block on lines 1–42 only describes AVX2/AVX-512 and says nothing about the scalar fallback.
How can I resolve this? If you propose a fix, please make it concise.…pdate module docs
1 participant
Add portable scalar fallback for packed Goldilocks
Description
The goldilocks_avx module was unavailable without AVX2. Add a scalar fallback (PackedGoldilocksScalar) with the same API that works on all platforms, and a PackedGoldilocks type alias that auto-selects AVX2 on x86_64 or scalar elsewhere.
Closes #1153
Checklist