-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add secure powers generation for simd #930
Add secure powers generation for simd #930
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #930 +/- ##
==========================================
+ Coverage 91.48% 91.49% +0.01%
==========================================
Files 98 98
Lines 13733 13762 +29
Branches 13733 13762 +29
==========================================
+ Hits 12563 12591 +28
- Misses 1055 1056 +1
Partials 115 115 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I didn't explain properly
the function should take alpha, n_powers and return Vec, that is the n_powers powers of alpha (exclusive) : 0, a, a^2....a^(n-1)
use larger vectors to perform multiple operations at a time
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @shaharsamocha7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2
.
Benchmark suite | Current: 0a2e9b9 | Previous: cd8b37b | Ratio |
---|---|---|---|
iffts/simd ifft/22 |
13635674 ns/iter (± 219842 ) |
6306399 ns/iter (± 210024 ) |
2.16 |
iffts/simd ifft/25 |
133703364 ns/iter (± 845083 ) |
65891527 ns/iter (± 1735211 ) |
2.03 |
iffts/simd ifft/26 |
278573898 ns/iter (± 3255932 ) |
136655216 ns/iter (± 2242165 ) |
2.04 |
iffts/simd ifft/27 |
646251801 ns/iter (± 15536196 ) |
312588285 ns/iter (± 9816879 ) |
2.07 |
iffts/simd ifft/28 |
1351168377 ns/iter (± 36149701 ) |
643771030 ns/iter (± 19147376 ) |
2.10 |
simd rfft 20bit |
3234196 ns/iter (± 21252 ) |
1617070 ns/iter (± 66435 ) |
2.00 |
merkle throughput/simd merkle |
30000903 ns/iter (± 498754 ) |
13712527 ns/iter (± 579195 ) |
2.19 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @shaharsamocha7
3057bea
to
06768d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 14 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 62 at r2 (raw file):
// TODO(Gali): Remove #[allow(dead_code)]. #[allow(dead_code)] pub fn generate_secure_powers_for_simd(felt: SecureField, n_powers: usize) -> Vec<SecureField> {
Suggestion:
pub fn generate_secure_powers_simd(felt: SecureField, n_powers: usize) -> Vec<SecureField> {
crates/prover/src/core/backend/simd/utils.rs
line 63 at r2 (raw file):
#[allow(dead_code)] pub fn generate_secure_powers_for_simd(felt: SecureField, n_powers: usize) -> Vec<SecureField> { let step_arr: [SecureField; N_LANES] = (0..N_LANES)
that logic is already written in the CPU function, use it?
crates/prover/src/core/backend/simd/utils.rs
line 69 at r2 (raw file):
Some(res) }) .collect::<Vec<SecureField>>()
less noise
Suggestion:
.collect_vec()
crates/prover/src/core/backend/simd/utils.rs
line 71 at r2 (raw file):
.collect::<Vec<SecureField>>() .try_into() .expect("Failed generating secure powers.");
I think it's alright to .unwrap() here, you won't fail the try_into...
non - blocking
crates/prover/src/core/backend/simd/utils.rs
line 77 at r2 (raw file):
let step_felt = step_arr[N_LANES - 1] * felt; let mut packed_powers_vec = Vec::new();
this is a good place to use vec::with_capacity
crates/prover/src/core/backend/simd/utils.rs
line 80 at r2 (raw file):
let mut curr_power: usize = 0; while curr_power < n_powers {
consider using scan land collect ike you did above, easier to read
crates/prover/src/core/backend/simd/utils.rs
line 81 at r2 (raw file):
while curr_power < n_powers { let base_packed_felt = PackedSecureField::from_array([base_felt; N_LANES]);
I think you can avoid ::from_array inside the loop, this is a load and is somewhat expensive
perhaps define the step as Simd([a^16; 16]), and multiply the current value with it each time (your step is a small register)
crates/prover/src/core/backend/simd/utils.rs
line 81 at r2 (raw file):
while curr_power < n_powers { let base_packed_felt = PackedSecureField::from_array([base_felt; N_LANES]);
also you can use broadcast to get [value; N_LANE] cheaper
crates/prover/src/core/backend/simd/utils.rs
line 87 at r2 (raw file):
} let powers_vec: Vec<SecureField> = packed_powers_vec
Suggestion:
let secure_powers = packed_powers_vec
crates/prover/src/core/backend/simd/utils.rs
line 88 at r2 (raw file):
let powers_vec: Vec<SecureField> = packed_powers_vec .iter()
Suggestion:
.into_iter()
crates/prover/src/core/backend/simd/utils.rs
line 90 at r2 (raw file):
.iter() .flat_map(|x| x.to_array().to_vec()) .collect();
Suggestion:
collect_vec();
crates/prover/src/core/backend/simd/utils.rs
line 92 at r2 (raw file):
.collect(); powers_vec[0..n_powers].to_vec()
you're doing 2 copies here, I think you can reduce that to 0 (trim the front/back...?)
Code quote:
let powers_vec: Vec<SecureField> = packed_powers_vec
.iter()
.flat_map(|x| x.to_array().to_vec())
.collect();
powers_vec[0..n_powers].to_vec()
crates/prover/src/core/backend/simd/utils.rs
line 129 at r2 (raw file):
fn generate_secure_powers_works() { let felt = qm31!(1, 2, 3, 4); let n_powers = 10;
if you had a bug (you don't) with n > 16 this test would not catch it, use a bigger number.
crates/prover/src/core/backend/simd/utils.rs
line 134 at r2 (raw file):
assert_eq!(powers.len(), n_powers); assert_eq!(powers[0], SecureField::one());
Suggestion:
let cpu_powers = generate_secure_powers(felt, n_powers)
let powers = generate_secure_powers_for_simd(felt, n_powers);
assert_eq!(powers, cpu_powers);
crates/prover/src/core/backend/simd/utils.rs
line 140 at r2 (raw file):
#[test] fn generate_empty_secure_powers_works() {
I don't think this edge case is of concern, remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 127 at r2 (raw file):
#[test] fn generate_secure_powers_works() {
rename the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 63 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
that logic is already written in the CPU function, use it?
Done.
crates/prover/src/core/backend/simd/utils.rs
line 69 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
less noise
Done.
crates/prover/src/core/backend/simd/utils.rs
line 77 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
this is a good place to use vec::with_capacity
Done.
crates/prover/src/core/backend/simd/utils.rs
line 80 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
consider using scan land collect ike you did above, easier to read
Done.
crates/prover/src/core/backend/simd/utils.rs
line 81 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
I think you can avoid ::from_array inside the loop, this is a load and is somewhat expensive
perhaps define the step as Simd([a^16; 16]), and multiply the current value with it each time (your step is a small register)
Done. Swapped base-step variables names as well
crates/prover/src/core/backend/simd/utils.rs
line 81 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
also you can use broadcast to get [value; N_LANE] cheaper
Done.
crates/prover/src/core/backend/simd/utils.rs
line 92 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
you're doing 2 copies here, I think you can reduce that to 0 (trim the front/back...?)
Done. Not sure that I got it
crates/prover/src/core/backend/simd/utils.rs
line 127 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
rename the tests
Done.
crates/prover/src/core/backend/simd/utils.rs
line 129 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
if you had a bug (you don't) with n > 16 this test would not catch it, use a bigger number.
Done.
crates/prover/src/core/backend/simd/utils.rs
line 140 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
I don't think this edge case is of concern, remove.
Done.
crates/prover/src/core/backend/simd/utils.rs
line 62 at r2 (raw file):
// TODO(Gali): Remove #[allow(dead_code)]. #[allow(dead_code)] pub fn generate_secure_powers_for_simd(felt: SecureField, n_powers: usize) -> Vec<SecureField> {
Done.
crates/prover/src/core/backend/simd/utils.rs
line 87 at r2 (raw file):
} let powers_vec: Vec<SecureField> = packed_powers_vec
Done.
crates/prover/src/core/backend/simd/utils.rs
line 88 at r2 (raw file):
let powers_vec: Vec<SecureField> = packed_powers_vec .iter()
Done.
crates/prover/src/core/backend/simd/utils.rs
line 90 at r2 (raw file):
.iter() .flat_map(|x| x.to_array().to_vec()) .collect();
Done.
crates/prover/src/core/backend/simd/utils.rs
line 134 at r2 (raw file):
assert_eq!(powers.len(), n_powers); assert_eq!(powers[0], SecureField::one());
Done.
06768d9
to
c35fcd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 92 at r2 (raw file):
Previously, Gali-StarkWare wrote…
Done. Not sure that I got it
the collect_vec().into_iter() is redundant, you already have an iterator (scan), that collect is a copy for nothing
crates/prover/src/core/backend/simd/utils.rs
line 64 at r3 (raw file):
#[allow(dead_code)] pub fn generate_secure_powers_simd(felt: SecureField, n_powers: usize) -> Vec<SecureField> { let base_arr: [SecureField; N_LANES] =
Suggestion:
let base_arr =
crates/prover/src/core/backend/simd/utils.rs
line 69 at r3 (raw file):
let step_felt = base_arr[N_LANES - 1] * felt; let step_packed_felt = PackedSecureField::broadcast(step_felt);
style
non-blocking
Suggestion:
let step = base_arr[N_LANES - 1] * felt;
let step = PackedSecureField::broadcast(step_felt);
crates/prover/src/core/backend/simd/utils.rs
line 75 at r3 (raw file):
} else { (n_powers / N_LANES) + 1 };
Suggestion:
let packed_size = (n_powers + N_LANES - 1) / N_LANES;
crates/prover/src/core/backend/simd/utils.rs
line 85 at r3 (raw file):
.collect_vec() .into_iter() .flat_map(|x| x.to_array().to_vec())
x.to_array() is a copy, use transmute?
crates/prover/src/core/backend/simd/utils.rs
line 85 at r3 (raw file):
.collect_vec() .into_iter() .flat_map(|x| x.to_array().to_vec())
this invoked a heap allocation for every vector :(
Suggestion:
flat_map(|x| x.to_array())
crates/prover/src/core/backend/simd/utils.rs
line 119 at r3 (raw file):
#[test] fn generate_secure_powers_simd_returns_the_same_as_cpu() {
Suggestion:
fn test_generate_secure_powers_simd() {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
fn generate_secure_powers_simd_returns_the_same_as_cpu() { let felt = qm31!(1, 2, 3, 4); let n_powers = 30;
bigger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 92 at r2 (raw file):
Previously, ohad-starkware (Ohad) wrote…
the collect_vec().into_iter() is redundant, you already have an iterator (scan), that collect is a copy for nothing
Done.
crates/prover/src/core/backend/simd/utils.rs
line 85 at r3 (raw file):
Previously, ohad-starkware (Ohad) wrote…
this invoked a heap allocation for every vector :(
Done.
crates/prover/src/core/backend/simd/utils.rs
line 85 at r3 (raw file):
Previously, ohad-starkware (Ohad) wrote…
x.to_array() is a copy, use transmute?
Done.
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
Previously, ohad-starkware (Ohad) wrote…
bigger?
Done.
crates/prover/src/core/backend/simd/utils.rs
line 64 at r3 (raw file):
#[allow(dead_code)] pub fn generate_secure_powers_simd(felt: SecureField, n_powers: usize) -> Vec<SecureField> { let base_arr: [SecureField; N_LANES] =
Done.
crates/prover/src/core/backend/simd/utils.rs
line 75 at r3 (raw file):
} else { (n_powers / N_LANES) + 1 };
Done.
crates/prover/src/core/backend/simd/utils.rs
line 119 at r3 (raw file):
#[test] fn generate_secure_powers_simd_returns_the_same_as_cpu() {
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 85 at r3 (raw file):
Previously, Gali-StarkWare wrote…
Done.
sorry the transmute is a lie I forgot were in the extension field
c35fcd7
to
9bed75c
Compare
9bed75c
to
f5e3e66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)
crates/prover/src/core/backend/simd/utils.rs
line 69 at r3 (raw file):
Previously, ohad-starkware (Ohad) wrote…
style
non-blocking
alternatively
let felt_pows = PackedSecureField::from_array(generate_secure_powers(felt, N_LANES).try_into().unwrap());
let felt_pow_n_lanes = PackedSecureField::broadcast(base_arr[N_LANES - 1] * felt);
crates/prover/src/core/backend/simd/utils.rs
line 75 at r3 (raw file):
Previously, Gali-StarkWare wrote…
Done.
there is a function
n_powers.div_ceil(N_LANES)
crates/prover/src/core/backend/simd/utils.rs
line 119 at r3 (raw file):
Previously, Gali-StarkWare wrote…
Done.
To be more explicit:
Test name should describe what the test is checking rather than the implementation itself.
crates/prover/src/core/backend/simd/utils.rs
line 125 at r3 (raw file):
let cpu_powers = generate_secure_powers(felt, n_powers); let powers = generate_secure_powers_simd(felt, n_powers); assert_eq!(powers, cpu_powers);
Add a new line here
https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/
Suggestion:
let powers = generate_secure_powers_simd(felt, n_powers);
assert_eq!(powers, cpu_powers);
f5e3e66
to
0a2e9b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also changed some variables' names, LMK if it's better
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 75 at r3 (raw file):
Previously, shaharsamocha7 wrote…
there is a function
n_powers.div_ceil(N_LANES)
Done.
crates/prover/src/core/backend/simd/utils.rs
line 85 at r3 (raw file):
Previously, ohad-starkware (Ohad) wrote…
sorry the transmute is a lie I forgot were in the extension field
Done.
crates/prover/src/core/backend/simd/utils.rs
line 119 at r3 (raw file):
Previously, shaharsamocha7 wrote…
To be more explicit:
Test name should describe what the test is checking rather than the implementation itself.
I thought that specifying that we want the same functionality as the of the other function is good for future changes (if we'll ever change one of them to do something different), but I guess there's no point doing that..
crates/prover/src/core/backend/simd/utils.rs
line 125 at r3 (raw file):
Previously, shaharsamocha7 wrote…
Add a new line here
https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)
crates/prover/src/core/backend/simd/utils.rs
line 119 at r3 (raw file):
Previously, Gali-StarkWare wrote…
I thought that specifying that we want the same functionality as the of the other function is good for future changes (if we'll ever change one of them to do something different), but I guess there's no point doing that..
The function purpose is to generate_secure_powers (with simd).
It will still exists if we deleted the cpu implementation.
The way you chose to test it is by checking equivalency to the cpu function.
Problem with adding it to the test name is that implementations tend to change (without changing the test name) while the purpose is more solid.
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
Previously, Gali-StarkWare wrote…
Done.
Actually why n_powers is so large?
we don't want this test to take too much time.
If so, maybe there are edge cases where n_powers is small.
crates/prover/src/core/backend/simd/utils.rs
line 116 at r5 (raw file):
let powers = generate_secure_powers_simd(felt, n_powers); assert_eq!(powers, cpu_powers);
consider to rename
Suggestion:
let simd_powers = generate_secure_powers_simd(felt, n_powers);
assert_eq!(simd_powers, cpu_powers);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
Previously, shaharsamocha7 wrote…
Actually why n_powers is so large?
we don't want this test to take too much time.If so, maybe there are edge cases where n_powers is small.
I thougth something like a 100, larger than two SIMD vectors ant not a multiple of 16
there is an edge case for <16 and 0, My thoughts were that you are already using the CPU function in your implementation, so comparing against that can be redundant when the function is simple.
I'm not sure so wait for Shahar's input on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)
crates/prover/src/core/backend/simd/utils.rs
line 119 at r3 (raw file):
Previously, shaharsamocha7 wrote…
The function purpose is to generate_secure_powers (with simd).
It will still exists if we deleted the cpu implementation.The way you chose to test it is by checking equivalency to the cpu function.
Problem with adding it to the test name is that implementations tend to change (without changing the test name) while the purpose is more solid.
Done.
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
Previously, shaharsamocha7 wrote…
100 is fine IMO.
I would also check <16 but it's nice to have.
non-blocking
Should I add the <16 check in the same test or in a new one? Based on the test's naming and the discussion above it should be in the same test, just making sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware)
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
Previously, Gali-StarkWare wrote…
Should I add the <16 check in the same test or in a new one? Based on the test's naming and the discussion above it should be in the same test, just making sure
The way I see it you can either adapt the test such that
n_powers_vec = [16, 100]
or split it to two tests
test_generate_secure_powers_simd_small
test_generate_secure_powers_simd_large
In this specific case I would go with option 1, but your call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware)
crates/prover/src/core/backend/simd/utils.rs
line 121 at r3 (raw file):
Previously, shaharsamocha7 wrote…
The way I see it you can either adapt the test such that
n_powers_vec = [16, 100]
or split it to two tests
test_generate_secure_powers_simd_small
test_generate_secure_powers_simd_large
In this specific case I would go with option 1, but your call
Done. Also added 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Gali-StarkWare)
-- commits
line 2 at r6:
please squash the commits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @Gali-StarkWare)
crates/prover/src/core/backend/simd/utils.rs
line 114 at r7 (raw file):
let n_powers_vec = [0, 16, 100]; let cpu_powers_vec: Vec<Vec<SecureField>> = n_powers_vec
you're doing 3 separate tests, have 3 corresponding asserts
so if one fails you know which of the edge cases are failing
83f4603
to
60828bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)
Previously, ohad-starkware (Ohad) wrote…
please squash the commits
Done.
crates/prover/src/core/backend/simd/utils.rs
line 114 at r7 (raw file):
Previously, ohad-starkware (Ohad) wrote…
you're doing 3 separate tests, have 3 corresponding asserts
so if one fails you know which of the edge cases are failing
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)
crates/prover/src/core/backend/simd/utils.rs
line 126 at r8 (raw file):
assert_eq!(simd_powers_vec[0], cpu_powers_vec[0]); assert_eq!(simd_powers_vec[1], cpu_powers_vec[1]); assert_eq!(simd_powers_vec[2], cpu_powers_vec[2]);
That code is not robust in the sense that if you will want to add another test case you'll also have to make the change here.
We don't want to limit our code for no reason
I put a suggestion for refactor, LMK what you think
Suggestion:
n_powers_vec.iter().for_each(|&n_powers| {
let expected = generate_secure_powers(felt, n_powers);
let actual = generate_secure_powers_simd(felt, n_powers);
assert_eq!(expected, actual);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @Gali-StarkWare and @shaharsamocha7)
Previously, Gali-StarkWare wrote…
Done.
are you sure?
I expected a single line commit message
crates/prover/src/core/backend/simd/utils.rs
line 126 at r8 (raw file):
Previously, shaharsamocha7 wrote…
That code is not robust in the sense that if you will want to add another test case you'll also have to make the change here.
We don't want to limit our code for no reasonI put a suggestion for refactor, LMK what you think
i'd also add n_powers to the panic message
60828bf
to
522eafa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)
crates/prover/src/core/backend/simd/utils.rs
line 126 at r8 (raw file):
Previously, ohad-starkware (Ohad) wrote…
i'd also add n_powers to the panic message
I think so too. Added these asserts because Ohad requested to and I do agree with him that it will be easier to debug his way. Should have thought of a better way to do so. This way is nicer - it's both generic in terms of the number of test cases and will fail the specific test case.
522eafa
to
93f8815
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)
Previously, ohad-starkware (Ohad) wrote…
are you sure?
I expected a single line commit message
Done.
1086811
to
479efce
Compare
479efce
to
3a203f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Gali-StarkWare)
crates/prover/src/core/backend/simd/utils.rs
line 121 at r13 (raw file):
assert_eq!( expected, actual, "Error generating secure powers in n_powers = {}.",
No description provided.