Ohadn/qm31 arithmetics in math utils #1944

ohad-nir-starkware · 2025-02-10T18:40:40Z

qm31 arithmetics in math utils

Description

add functions that compute packed reduced qm31 arithmetics to math_utils

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.
- CHANGELOG has been updated.

This change is

github-actions · 2025-02-10T18:50:09Z

**Hyper Thereading Benchmark results**




hyperfine -r 2 -n "hyper_threading_main threads: 1" 'RAYON_NUM_THREADS=1 ./hyper_threading_main' -n "hyper_threading_pr threads: 1" 'RAYON_NUM_THREADS=1 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 1
  Time (mean ± σ):     27.153 s ±  0.010 s    [User: 26.446 s, System: 0.705 s]
  Range (min … max):   27.146 s … 27.160 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 1
  Time (mean ± σ):     28.139 s ±  0.032 s    [User: 27.409 s, System: 0.728 s]
  Range (min … max):   28.116 s … 28.162 s    2 runs
 
Summary
  hyper_threading_main threads: 1 ran
    1.04 ± 0.00 times faster than hyper_threading_pr threads: 1




hyperfine -r 2 -n "hyper_threading_main threads: 2" 'RAYON_NUM_THREADS=2 ./hyper_threading_main' -n "hyper_threading_pr threads: 2" 'RAYON_NUM_THREADS=2 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 2
  Time (mean ± σ):     15.217 s ±  0.128 s    [User: 26.685 s, System: 0.750 s]
  Range (min … max):   15.127 s … 15.307 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 2
  Time (mean ± σ):     15.611 s ±  0.034 s    [User: 27.456 s, System: 0.800 s]
  Range (min … max):   15.588 s … 15.635 s    2 runs
 
Summary
  hyper_threading_main threads: 2 ran
    1.03 ± 0.01 times faster than hyper_threading_pr threads: 2




hyperfine -r 2 -n "hyper_threading_main threads: 4" 'RAYON_NUM_THREADS=4 ./hyper_threading_main' -n "hyper_threading_pr threads: 4" 'RAYON_NUM_THREADS=4 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 4
  Time (mean ± σ):     11.221 s ±  0.221 s    [User: 39.709 s, System: 0.896 s]
  Range (min … max):   11.065 s … 11.377 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 4
  Time (mean ± σ):     10.918 s ±  0.057 s    [User: 40.892 s, System: 0.907 s]
  Range (min … max):   10.878 s … 10.958 s    2 runs
 
Summary
  hyper_threading_pr threads: 4 ran
    1.03 ± 0.02 times faster than hyper_threading_main threads: 4




hyperfine -r 2 -n "hyper_threading_main threads: 6" 'RAYON_NUM_THREADS=6 ./hyper_threading_main' -n "hyper_threading_pr threads: 6" 'RAYON_NUM_THREADS=6 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 6
  Time (mean ± σ):     10.860 s ±  0.273 s    [User: 39.943 s, System: 0.904 s]
  Range (min … max):   10.667 s … 11.052 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 6
  Time (mean ± σ):     11.036 s ±  0.324 s    [User: 41.067 s, System: 0.934 s]
  Range (min … max):   10.807 s … 11.264 s    2 runs
 
Summary
  hyper_threading_main threads: 6 ran
    1.02 ± 0.04 times faster than hyper_threading_pr threads: 6




hyperfine -r 2 -n "hyper_threading_main threads: 8" 'RAYON_NUM_THREADS=8 ./hyper_threading_main' -n "hyper_threading_pr threads: 8" 'RAYON_NUM_THREADS=8 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 8
  Time (mean ± σ):     10.763 s ±  0.018 s    [User: 40.194 s, System: 0.928 s]
  Range (min … max):   10.750 s … 10.776 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 8
  Time (mean ± σ):     10.931 s ±  0.004 s    [User: 41.362 s, System: 0.918 s]
  Range (min … max):   10.928 s … 10.933 s    2 runs
 
Summary
  hyper_threading_main threads: 8 ran
    1.02 ± 0.00 times faster than hyper_threading_pr threads: 8




hyperfine -r 2 -n "hyper_threading_main threads: 16" 'RAYON_NUM_THREADS=16 ./hyper_threading_main' -n "hyper_threading_pr threads: 16" 'RAYON_NUM_THREADS=16 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 16
  Time (mean ± σ):     10.782 s ±  0.175 s    [User: 40.678 s, System: 1.006 s]
  Range (min … max):   10.658 s … 10.906 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 16
  Time (mean ± σ):     11.264 s ±  0.070 s    [User: 41.420 s, System: 1.001 s]
  Range (min … max):   11.214 s … 11.313 s    2 runs
 
Summary
  hyper_threading_main threads: 16 ran
    1.04 ± 0.02 times faster than hyper_threading_pr threads: 16

github-actions · 2025-02-10T19:05:10Z

Benchmark Results for unmodified programs 🚀

Command	Mean [s]	Min [s]	Max [s]	Relative
`base big_factorial`	2.202 ± 0.012	2.192	2.226	1.00
`head big_factorial`	2.235 ± 0.022	2.207	2.279	1.02 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base big_fibonacci`	2.177 ± 0.015	2.152	2.193	1.00
`head big_fibonacci`	2.209 ± 0.065	2.175	2.390	1.01 ± 0.03

Command	Mean [s]	Min [s]	Max [s]	Relative
`base blake2s_integration_benchmark`	8.417 ± 0.227	8.135	8.936	1.03 ± 0.03
`head blake2s_integration_benchmark`	8.155 ± 0.044	8.108	8.250	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
`base compare_arrays_200000`	2.323 ± 0.019	2.305	2.356	1.01 ± 0.02
`head compare_arrays_200000`	2.307 ± 0.031	2.281	2.370	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
`base dict_integration_benchmark`	1.502 ± 0.007	1.491	1.517	1.00
`head dict_integration_benchmark`	1.510 ± 0.008	1.494	1.524	1.01 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base field_arithmetic_get_square_benchmark`	1.302 ± 0.008	1.283	1.313	1.00 ± 0.01
`head field_arithmetic_get_square_benchmark`	1.301 ± 0.012	1.288	1.323	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
`base integration_builtins`	8.435 ± 0.249	8.177	9.028	1.02 ± 0.03
`head integration_builtins`	8.244 ± 0.050	8.193	8.342	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
`base keccak_integration_benchmark`	8.560 ± 0.095	8.423	8.730	1.01 ± 0.02
`head keccak_integration_benchmark`	8.513 ± 0.136	8.383	8.857	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
`base linear_search`	2.275 ± 0.016	2.247	2.303	1.00
`head linear_search`	2.283 ± 0.022	2.261	2.330	1.00 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base math_cmp_and_pow_integration_benchmark`	1.578 ± 0.006	1.568	1.586	1.00
`head math_cmp_and_pow_integration_benchmark`	1.599 ± 0.007	1.586	1.611	1.01 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base math_integration_benchmark`	1.529 ± 0.014	1.515	1.563	1.00
`head math_integration_benchmark`	1.549 ± 0.015	1.535	1.587	1.01 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base memory_integration_benchmark`	1.273 ± 0.006	1.267	1.285	1.00
`head memory_integration_benchmark`	1.283 ± 0.010	1.272	1.304	1.01 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base operations_with_data_structures_benchmarks`	1.655 ± 0.013	1.640	1.684	1.00
`head operations_with_data_structures_benchmarks`	1.662 ± 0.006	1.656	1.673	1.00 ± 0.01

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`base pedersen`	551.6 ± 7.4	546.5	571.3	1.00 ± 0.01
`head pedersen`	550.4 ± 1.6	547.5	552.6	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`base poseidon_integration_benchmark`	658.4 ± 6.8	647.1	669.1	1.00
`head poseidon_integration_benchmark`	664.4 ± 6.4	655.5	678.3	1.01 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base secp_integration_benchmark`	1.919 ± 0.017	1.900	1.962	1.00 ± 0.01
`head secp_integration_benchmark`	1.917 ± 0.011	1.904	1.940	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`base set_integration_benchmark`	642.8 ± 3.2	639.3	650.0	1.00
`head set_integration_benchmark`	678.1 ± 2.9	673.3	683.6	1.05 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
`base uint256_integration_benchmark`	4.515 ± 0.035	4.456	4.565	1.01 ± 0.01
`head uint256_integration_benchmark`	4.492 ± 0.043	4.450	4.592	1.00

codecov · 2025-02-10T19:15:28Z

Codecov Report

Attention: Patch coverage is 99.66102% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.41%. Comparing base (7a97bed) to head (38adbe6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
vm/src/math_utils/mod.rs	99.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1944      +/-   ##
==========================================
+ Coverage   96.39%   96.41%   +0.02%     
==========================================
  Files         102      102              
  Lines       41566    41861     +295     
==========================================
+ Hits        40068    40362     +294     
- Misses       1498     1499       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vm/src/math_utils/mod.rs

ohad-nir-starkware

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/math_utils/mod.rs

ohad-nir-starkware

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/math_utils/mod.rs

ohad-nir-starkware

Reviewable status: 8 of 12 files reviewed, 6 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

vm/src/math_utils/mod.rs line 195 at r4 (raw file):

Previously, ohad-nir-starkware (Ohad Nir) wrote…

not sure I understand what is you recommendation, use u64 or u128?

For the sake of VM efficiency, I have changed it to u64.
let me know what you think.

DavidLevitGurevich

Reviewed 4 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dancarmoz, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

vm/src/math_utils/mod.rs line 195 at r4 (raw file):

Previously, ohad-nir-starkware (Ohad Nir) wrote…

For the sake of VM efficiency, I have changed it to u64.
let me know what you think.

@dancarmoz are you sure it is more efficient? or is it not what you meant? because it seems that the code now does % stwo_prime_u128 more time than before

dancarmoz

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

vm/src/math_utils/mod.rs line 194 at r4 (raw file):

Previously, ohad-nir-starkware (Ohad Nir) wrote…

Done.

Should still be upper case C (Computes)

vm/src/math_utils/mod.rs line 195 at r4 (raw file):

Previously, DavidLevitGurevich wrote…

@dancarmoz are you sure it is more efficient? or is it not what you meant? because it seems that the code now does % stwo_prime_u128 more time than before

Certainly it does % stwo_prime_u128 fewer times than before, now it only does it inside the mul, and before it did in every function and every packing (it's just that STWO_PRIME used to be the name of stwo_prime_u128 rather than of the u64 version, which didn't exist...).

As to whether I'm sure it's more efficient - it is mostly intuition that u64 operations should be faster than u128 operations by a factor that justifies these few conversions. IMO the only way to be sure which is faster is to test and benchmark.

vm/src/math_utils/mod.rs line 178 at r6 (raw file):

        coordinates2_u64[2] as u128,
        coordinates2_u64[3] as u128,
    ];

You can also apply the map directly on qm31_packed_reduced_read_coordinates(felt[12])? without defining coordinates[12]_u64.

Suggestion:

    let coordinates1 = coordinates1_u64.map(u128::from);
    let coordinates2 = coordinates2_u64.map(u128::from);

vm/src/math_utils/mod.rs line 179 at r6 (raw file):

        coordinates2_u64[3] as u128,
    ];
    let stwo_prime_u128 = STWO_PRIME as u128;

It would be better (and possibly more efficient) to have STWO_PRIME_U128 be a const as well.

vm/src/math_utils/mod.rs line 180 at r6 (raw file):

    ];
    let stwo_prime_u128 = STWO_PRIME as u128;
    let result_unreduced_coordinates = [

Actually already reduced in this version :)

Suggestion:

let result_coordinates = [

vm/src/math_utils/mod.rs line 213 at r6 (raw file):

/// M31 utility function, used specifically for Stwo.
/// computes inverse in the M31 field using Fermat's little theorem i.e. returns
/// `v^(STWO_PRIME-2) modulo STWO_PRIME` which is the inverse of v unless v % STWO_PRIME == 0

Suggestion:

/// Computes the inverse in the M31 field using Fermat's little theorem, i.e., returns
/// `v^(STWO_PRIME-2) modulo STWO_PRIME`, which is the inverse of v unless v % STWO_PRIME == 0.

ohad-nir-starkware

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dancarmoz, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

vm/src/math_utils/mod.rs line 194 at r4 (raw file):

Previously, dancarmoz (Dan Carmon) wrote…

Should still be upper case C (Computes)

Done.

vm/src/math_utils/mod.rs line 178 at r6 (raw file):

Previously, dancarmoz (Dan Carmon) wrote…

You can also apply the map directly on qm31_packed_reduced_read_coordinates(felt[12])? without defining coordinates[12]_u64.

Done.

vm/src/math_utils/mod.rs line 179 at r6 (raw file):

Previously, dancarmoz (Dan Carmon) wrote…

It would be better (and possibly more efficient) to have STWO_PRIME_U128 be a const as well.

Done.

vm/src/math_utils/mod.rs line 180 at r6 (raw file):

Previously, dancarmoz (Dan Carmon) wrote…

Actually already reduced in this version :)

Done.

vm/src/math_utils/mod.rs line 213 at r6 (raw file):

/// M31 utility function, used specifically for Stwo.
/// computes inverse in the M31 field using Fermat's little theorem i.e. returns
/// `v^(STWO_PRIME-2) modulo STWO_PRIME` which is the inverse of v unless v % STWO_PRIME == 0

Done.

ohad-nir-starkware

Reviewable status: 11 of 12 files reviewed, 5 unresolved discussions (waiting on @dancarmoz, @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

vm/src/math_utils/mod.rs line 195 at r4 (raw file):

Previously, dancarmoz (Dan Carmon) wrote…

Certainly it does % stwo_prime_u128 fewer times than before, now it only does it inside the mul, and before it did in every function and every packing (it's just that STWO_PRIME used to be the name of stwo_prime_u128 rather than of the u64 version, which didn't exist...).

As to whether I'm sure it's more efficient - it is mostly intuition that u64 operations should be faster than u128 operations by a factor that justifies these few conversions. IMO the only way to be sure which is faster is to test and benchmark.

Tested the time it takes to compute qm31_packed_reduced_mul 10^6 times in both versions (u128 and u64):
the u64 version took about 109 ms and the u128 version took about 160 seconds, so even with the extra % calculations the u64 is much faster.
I could also implement another packing function that assumes that the coordinates are already reduced or add a flag to qm31_coordinates_to_packed_reduced that indicates whether to assume that or not.
WDYT?

DavidLevitGurevich

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dancarmoz, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

dancarmoz

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

vm/src/math_utils/mod.rs line 195 at r4 (raw file):

Previously, ohad-nir-starkware (Ohad Nir) wrote…

Tested the time it takes to compute qm31_packed_reduced_mul 10^6 times in both versions (u128 and u64):
the u64 version took about 109 ms and the u128 version took about 160 seconds, so even with the extra % calculations the u64 is much faster.
I could also implement another packing function that assumes that the coordinates are already reduced or add a flag to qm31_coordinates_to_packed_reduced that indicates whether to assume that or not.
WDYT?

I think the current version is good as is 👍

gabrielbosio · 2025-02-19T16:35:28Z

vm/src/math_utils/mod.rs

+/// M31 utility function, used specifically for Stwo.
+/// Computes the inverse in the M31 field using Fermat's little theorem, i.e., returns
+/// `v^(STWO_PRIME-2) modulo STWO_PRIME`, which is the inverse of v unless v % STWO_PRIME == 0.
+pub fn pow2147483645(v: u64) -> u64 {


If this function computes the inverse of the M31 field, I suggest to rename it to something like:

Suggested change

pub fn pow2147483645(v: u64) -> u64 {

pub fn m31_inv(v: u64) -> u64 {

well, it doesn't invert zero, but you have a point.
however this function and sqn are adaptations from
https://github.com/starkware-libs/stwo/blob/dev/crates/prover/src/core/fields/m31.rs
do you still think they should be renamed?

I initially thought of adding starkware-libs/stwo as a dependency and taking all the QM31 arithmetics from there, but I was told that is not option.
Let me know whether that is true or not.

We can leave it like this for now

gabrielbosio · 2025-02-19T16:36:01Z

vm/src/math_utils/mod.rs

+
+/// M31 utility function, used specifically for Stwo.
+/// Computes `v^(2^n) modulo STWO_PRIME`.
+fn sqn(v: u64, n: usize) -> u64 {


I would rename this function to something more descriptive:

Suggested change

fn sqn(v: u64, n: usize) -> u64 {

fn square_n(v: u64, n: usize) -> u64 {

Addressed in the discussion about pow2147483645. let me know what you think.

We can leave it like this for now

gabrielbosio · 2025-02-19T19:16:41Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_add_test() {


Please use the ´test´ word as a prefix for test case names, following the convention for other cases.

Suggested change

fn qm31_packed_reduced_add_test() {

fn test_qm31_packed_reduced_add() {

gabrielbosio · 2025-02-19T19:16:58Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_neg_test() {


Suggested change

fn qm31_packed_reduced_neg_test() {

fn test_qm31_packed_reduced_neg() {

gabrielbosio · 2025-02-19T19:17:14Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_sub_test() {


Suggested change

fn qm31_packed_reduced_sub_test() {

fn test_qm31_packed_reduced_sub() {

gabrielbosio · 2025-02-19T19:17:28Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_mul_test() {


Suggested change

fn qm31_packed_reduced_mul_test() {

fn test_qm31_packed_reduced_mul() {

gabrielbosio · 2025-02-19T19:17:38Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_inv_test() {


Suggested change

fn qm31_packed_reduced_inv_test() {

fn test_qm31_packed_reduced_inv() {

gabrielbosio · 2025-02-19T19:18:30Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_div_test() {


Suggested change

fn qm31_packed_reduced_div_test() {

fn test_qm31_packed_reduced_div() {

gabrielbosio · 2025-02-19T19:19:01Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_inv_extensive_test() {


Suggested change

fn qm31_packed_reduced_inv_extensive_test() {

fn test_qm31_packed_reduced_inv_extensive() {

gabrielbosio · 2025-02-19T20:08:58Z

vm/src/math_utils/mod.rs

+        let mut rng = SmallRng::seed_from_u64(11480028852697973135);
+        let configurations = vec!["zero", "one", "minus_one", "random"];
+        let mut cartesian_product: Vec<[&str; 4]> = vec![];
+        for &a in &configurations {
+            for &b in &configurations {
+                for &c in &configurations {
+                    for &d in &configurations {
+                        cartesian_product.push([a, b, c, d]);
+                    }
+                }
+            }
+        }
+
+        for test_case in cartesian_product {
+            let x_coordinates: [u64; 4] = test_case
+                .iter()
+                .map(|&x| match x {
+                    "zero" => 0,
+                    "one" => 1,
+                    "minus_one" => STWO_PRIME - 1,
+                    "random" => rng.gen_range(0..STWO_PRIME),
+                    _ => unreachable!(),
+                })
+                .collect::<Vec<u64>>()
+                .try_into()
+                .unwrap();
+            if x_coordinates == [0, 0, 0, 0] {
+                continue;
+            }
+            let x = qm31_coordinates_to_packed_reduced(x_coordinates);
+            let res = qm31_packed_reduced_inv(x).unwrap();
+            assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1)));
+        }


In this case I suggest to use proptest. If you want to force testing 0, 1 and -1 you can also add separate test cases for those.

Here's an example of how the proptest could look like:

proptest! { #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn test_qm31_packed_reduced_inv(x_coordinates in uniform4(0..STWO_PRIME).prop_filter("felt is zero", |x| *x != [0, 0, 0, 0])) { let x = qm31_coordinates_to_packed_reduced(x_coordinates); let res = qm31_packed_reduced_inv(x).unwrap(); assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1))); } }

wrote a test inside the existing proptest!, it's called qm31_packed_reduced_inv_x_by_x_is_1, let me know what you think.
@DavidLevitGurevich can this new test replace qm31_packed_reduced_inv_extensive_test?

I don't know this tool, if it goes over all options then it's good, otherwise I feel it's a degradation of the test.

If the idea is to make sure that we include 0, 1 or -1 in the test cases then you need to specify it in the test case function and not in the proptest argument. The proptest argument is useful for the random values.

We can leave this like this by deleting the qm31_packed_reduced_inv_x_by_x_is_1 test. Then we can create a PR for refactoring.

A minor change I think it can be done here is to replace the string cases with an enum (e.g.: replace "zero" with Coordinate::Zero or something like that).

ohad-nir-starkware

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

ohad-nir-starkware · 2025-02-19T22:24:43Z

vm/src/math_utils/mod.rs

+/// M31 utility function, used specifically for Stwo.
+/// Computes the inverse in the M31 field using Fermat's little theorem, i.e., returns
+/// `v^(STWO_PRIME-2) modulo STWO_PRIME`, which is the inverse of v unless v % STWO_PRIME == 0.
+pub fn pow2147483645(v: u64) -> u64 {


well, it doesn't invert zero, but you have a point.
however this function and sqn are adaptations from
https://github.com/starkware-libs/stwo/blob/dev/crates/prover/src/core/fields/m31.rs
do you still think they should be renamed?

I initially thought of adding starkware-libs/stwo as a dependency and taking all the QM31 arithmetics from there, but I was told that is not option.
Let me know whether that is true or not.

ohad-nir-starkware · 2025-02-19T22:24:43Z

vm/src/math_utils/mod.rs

+
+/// M31 utility function, used specifically for Stwo.
+/// Computes `v^(2^n) modulo STWO_PRIME`.
+fn sqn(v: u64, n: usize) -> u64 {


Addressed in the discussion about pow2147483645. let me know what you think.

ohad-nir-starkware · 2025-02-19T22:24:43Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_add_test() {


ohad-nir-starkware · 2025-02-19T22:24:44Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_neg_test() {


ohad-nir-starkware · 2025-02-19T22:24:44Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_sub_test() {


ohad-nir-starkware · 2025-02-19T22:24:44Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_mul_test() {


ohad-nir-starkware · 2025-02-19T22:24:45Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_inv_test() {


ohad-nir-starkware · 2025-02-19T22:24:45Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_inv_extensive_test() {


ohad-nir-starkware · 2025-02-19T22:24:46Z

vm/src/math_utils/mod.rs

+        let mut rng = SmallRng::seed_from_u64(11480028852697973135);
+        let configurations = vec!["zero", "one", "minus_one", "random"];
+        let mut cartesian_product: Vec<[&str; 4]> = vec![];
+        for &a in &configurations {
+            for &b in &configurations {
+                for &c in &configurations {
+                    for &d in &configurations {
+                        cartesian_product.push([a, b, c, d]);
+                    }
+                }
+            }
+        }
+
+        for test_case in cartesian_product {
+            let x_coordinates: [u64; 4] = test_case
+                .iter()
+                .map(|&x| match x {
+                    "zero" => 0,
+                    "one" => 1,
+                    "minus_one" => STWO_PRIME - 1,
+                    "random" => rng.gen_range(0..STWO_PRIME),
+                    _ => unreachable!(),
+                })
+                .collect::<Vec<u64>>()
+                .try_into()
+                .unwrap();
+            if x_coordinates == [0, 0, 0, 0] {
+                continue;
+            }
+            let x = qm31_coordinates_to_packed_reduced(x_coordinates);
+            let res = qm31_packed_reduced_inv(x).unwrap();
+            assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1)));
+        }


wrote a test inside the existing proptest!, it's called qm31_packed_reduced_inv_x_by_x_is_1, let me know what you think.
@DavidLevitGurevich can this new test replace qm31_packed_reduced_inv_extensive_test?

ohad-nir-starkware · 2025-02-19T22:24:46Z

vm/src/math_utils/mod.rs

+
+    #[test]
+    #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+    fn qm31_packed_reduced_div_test() {


DavidLevitGurevich

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed (commit messages unreviewed), 12 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

DavidLevitGurevich · 2025-02-20T09:33:27Z

vm/src/math_utils/mod.rs

+        let mut rng = SmallRng::seed_from_u64(11480028852697973135);
+        let configurations = vec!["zero", "one", "minus_one", "random"];
+        let mut cartesian_product: Vec<[&str; 4]> = vec![];
+        for &a in &configurations {
+            for &b in &configurations {
+                for &c in &configurations {
+                    for &d in &configurations {
+                        cartesian_product.push([a, b, c, d]);
+                    }
+                }
+            }
+        }
+
+        for test_case in cartesian_product {
+            let x_coordinates: [u64; 4] = test_case
+                .iter()
+                .map(|&x| match x {
+                    "zero" => 0,
+                    "one" => 1,
+                    "minus_one" => STWO_PRIME - 1,
+                    "random" => rng.gen_range(0..STWO_PRIME),
+                    _ => unreachable!(),
+                })
+                .collect::<Vec<u64>>()
+                .try_into()
+                .unwrap();
+            if x_coordinates == [0, 0, 0, 0] {
+                continue;
+            }
+            let x = qm31_coordinates_to_packed_reduced(x_coordinates);
+            let res = qm31_packed_reduced_inv(x).unwrap();
+            assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1)));
+        }


I don't know this tool, if it goes over all options then it's good, otherwise I feel it's a degradation of the test.

DavidLevitGurevich

Reviewed all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

gabrielbosio · 2025-02-21T15:41:17Z

@ohad-nir-starkware the QM31 operations should be defined in the future in Lambdaworks along with the Felt252 operations. That's why I think this should be documented somewhere in the QM31 operations.

ohad-nir-starkware

@gabrielbosio added a line in the documentation of each QM31 / M31 function regarding it's future relocation to Lambdaworks.
let me know if any other changes are needed.

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

ohad-nir-starkware · 2025-02-21T19:49:52Z

vm/src/math_utils/mod.rs

+        let mut rng = SmallRng::seed_from_u64(11480028852697973135);
+        let configurations = vec!["zero", "one", "minus_one", "random"];
+        let mut cartesian_product: Vec<[&str; 4]> = vec![];
+        for &a in &configurations {
+            for &b in &configurations {
+                for &c in &configurations {
+                    for &d in &configurations {
+                        cartesian_product.push([a, b, c, d]);
+                    }
+                }
+            }
+        }
+
+        for test_case in cartesian_product {
+            let x_coordinates: [u64; 4] = test_case
+                .iter()
+                .map(|&x| match x {
+                    "zero" => 0,
+                    "one" => 1,
+                    "minus_one" => STWO_PRIME - 1,
+                    "random" => rng.gen_range(0..STWO_PRIME),
+                    _ => unreachable!(),
+                })
+                .collect::<Vec<u64>>()
+                .try_into()
+                .unwrap();
+            if x_coordinates == [0, 0, 0, 0] {
+                continue;
+            }
+            let x = qm31_coordinates_to_packed_reduced(x_coordinates);
+            let res = qm31_packed_reduced_inv(x).unwrap();
+            assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1)));
+        }


gabrielbosio · 2025-02-21T20:53:53Z

vm/src/math_utils/mod.rs

+        assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1)));
+    }
+
+    #[test]


I forgot something, because we decided to keep this test case as it is for now, I suggest to add this:

Suggested change

#[test]

// TODO: Refactor using proptest and separating particular cases

#[test]

ohad-nir-starkware

Reviewable status: 11 of 12 files reviewed, 13 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)

ohad-nir-starkware · 2025-02-21T21:07:42Z

vm/src/math_utils/mod.rs

+/// M31 utility function, used specifically for Stwo.
+/// Computes the inverse in the M31 field using Fermat's little theorem, i.e., returns
+/// `v^(STWO_PRIME-2) modulo STWO_PRIME`, which is the inverse of v unless v % STWO_PRIME == 0.
+pub fn pow2147483645(v: u64) -> u64 {


ohad-nir-starkware · 2025-02-21T21:07:42Z

vm/src/math_utils/mod.rs

+
+/// M31 utility function, used specifically for Stwo.
+/// Computes `v^(2^n) modulo STWO_PRIME`.
+fn sqn(v: u64, n: usize) -> u64 {


ohad-nir-starkware · 2025-02-21T21:07:43Z

vm/src/math_utils/mod.rs

+        assert_eq!(qm31_packed_reduced_mul(x, res), Ok(Felt252::from(1)));
+    }
+
+    #[test]


ohad-nir-starkware requested review from JulianGCalderon and YairVaknin-starkware February 10, 2025 18:40

ohad-nir-starkware self-assigned this Feb 10, 2025

ohad-nir-starkware requested review from igaray, Oppen, fmoletta, juanbono, pefontana and gabrielbosio as code owners February 10, 2025 18:40

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from 3237909 to d6d9a96 Compare February 10, 2025 18:43

ohad-nir-starkware changed the base branch from main to ohadn/u128_encoded_instr February 10, 2025 18:51

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from d6d9a96 to d1f230b Compare February 10, 2025 21:47

ohad-nir-starkware force-pushed the ohadn/u128_encoded_instr branch 2 times, most recently from 1bec5ba to afcbcea Compare February 11, 2025 10:05

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from d1f230b to 4bca3db Compare February 11, 2025 11:50

ohad-nir-starkware force-pushed the ohadn/u128_encoded_instr branch 3 times, most recently from 6f060e8 to ee6ba48 Compare February 11, 2025 19:35

ohad-nir-starkware changed the base branch from ohadn/u128_encoded_instr to main February 11, 2025 20:22

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch 3 times, most recently from ef1aa46 to cbd7519 Compare February 12, 2025 18:09

JulianGCalderon requested changes Feb 12, 2025

View reviewed changes

vm/src/math_utils/mod.rs Outdated Show resolved Hide resolved

ohad-nir-starkware commented Feb 12, 2025

View reviewed changes

vm/src/math_utils/mod.rs Outdated Show resolved Hide resolved

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from cbd7519 to ee011aa Compare February 12, 2025 20:12

ohad-nir-starkware commented Feb 12, 2025

View reviewed changes

vm/src/math_utils/mod.rs Outdated Show resolved Hide resolved

JulianGCalderon reviewed Feb 14, 2025

View reviewed changes

vm/src/math_utils/mod.rs Outdated Show resolved Hide resolved

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from 84ec321 to 9cdce6f Compare February 18, 2025 11:38

ohad-nir-starkware commented Feb 18, 2025

View reviewed changes

DavidLevitGurevich reviewed Feb 18, 2025

View reviewed changes

dancarmoz reviewed Feb 19, 2025

View reviewed changes

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from 9cdce6f to 57521d5 Compare February 19, 2025 10:03

ohad-nir-starkware commented Feb 19, 2025

View reviewed changes

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from 57521d5 to 29ce31e Compare February 19, 2025 12:40

DavidLevitGurevich reviewed Feb 19, 2025

View reviewed changes

dancarmoz reviewed Feb 19, 2025

View reviewed changes

gabrielbosio requested changes Feb 19, 2025

View reviewed changes

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from 29ce31e to d733ade Compare February 19, 2025 22:24

ohad-nir-starkware commented Feb 19, 2025

View reviewed changes

DavidLevitGurevich reviewed Feb 20, 2025

View reviewed changes

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from d733ade to 9b6fbea Compare February 21, 2025 19:45

ohad-nir-starkware commented Feb 21, 2025

View reviewed changes

gabrielbosio approved these changes Feb 21, 2025

View reviewed changes

gabrielbosio requested changes Feb 21, 2025

View reviewed changes

add functions that compute packed reduced qm31 arithmetics to math_utils

38adbe6

ohad-nir-starkware force-pushed the ohadn/qm31_arithmetics-in-math_utils branch from 9b6fbea to 38adbe6 Compare February 21, 2025 21:07

ohad-nir-starkware commented Feb 21, 2025

View reviewed changes

gabrielbosio approved these changes Feb 21, 2025

View reviewed changes

gabrielbosio added this pull request to the merge queue Feb 21, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 21, 2025

ohad-nir-starkware added this pull request to the merge queue Feb 21, 2025

Merged via the queue into main with commit fa977b3 Feb 21, 2025
94 checks passed

ohad-nir-starkware deleted the ohadn/qm31_arithmetics-in-math_utils branch February 21, 2025 22:37

	pub fn pow2147483645(v: u64) -> u64 {
	pub fn m31_inv(v: u64) -> u64 {

	fn sqn(v: u64, n: usize) -> u64 {
	fn square_n(v: u64, n: usize) -> u64 {

	fn qm31_packed_reduced_add_test() {
	fn test_qm31_packed_reduced_add() {

	fn qm31_packed_reduced_neg_test() {
	fn test_qm31_packed_reduced_neg() {

	fn qm31_packed_reduced_sub_test() {
	fn test_qm31_packed_reduced_sub() {

	fn qm31_packed_reduced_mul_test() {
	fn test_qm31_packed_reduced_mul() {

	fn qm31_packed_reduced_inv_test() {
	fn test_qm31_packed_reduced_inv() {

	fn qm31_packed_reduced_div_test() {
	fn test_qm31_packed_reduced_div() {

	fn qm31_packed_reduced_inv_extensive_test() {
	fn test_qm31_packed_reduced_inv_extensive() {

	#[test]
	// TODO: Refactor using proptest and separating particular cases
	#[test]

Ohadn/qm31 arithmetics in math utils #1944

Ohadn/qm31 arithmetics in math utils #1944

Conversation

ohad-nir-starkware commented Feb 10, 2025 • edited Loading

qm31 arithmetics in math utils

Description

Checklist

github-actions bot commented Feb 10, 2025 • edited Loading

github-actions bot commented Feb 10, 2025 • edited Loading

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

DavidLevitGurevich left a comment

Choose a reason for hiding this comment

dancarmoz left a comment

Choose a reason for hiding this comment

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

DavidLevitGurevich left a comment

Choose a reason for hiding this comment

dancarmoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidLevitGurevich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidLevitGurevich left a comment

Choose a reason for hiding this comment

gabrielbosio commented Feb 21, 2025

ohad-nir-starkware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohad-nir-starkware left a comment

ohad-nir-starkware commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading