-
Notifications
You must be signed in to change notification settings - Fork 316
feat(gpu): create noise and pfail tests pbs128 and packingks #3080
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
Conversation
tfhe/src/core_crypto/commons/noise_formulas/noise_simulation/lwe_programmable_bootstrap.rs
Outdated
Show resolved
Hide resolved
|
@guillermo-oyarzun maybe close this one for now given the base commit/branch has not been merged yet ? |
IceTDrinker
left a comment
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.
(commenting through review to remove notification, sorry...)
636de37 to
0c0ac11
Compare
I reactivated it now, since we merged the other PR. I applied similar changes to the ones in the previous PR, if you have time, can you take a look at it? |
yep, I'll try my best |
0c0ac11 to
236fe52
Compare
IceTDrinker
left a comment
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.
Several comments, be very careful on the number of samples
do you have results for these two tests ?
@IceTDrinker reviewed 3 of 25 files at r1, 22 of 22 files at r2, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @agnesLeroy, @guillermo-oyarzun, and @mayeul-zama)
tfhe/src/core_crypto/gpu/mod.rs line 842 at r2 (raw file):
{ unsafe { cuda_modulus_switch_ciphertext_async(streams, &mut *output_lwe_ciphertext, log_modulus);
that does not look reasonable
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/br_dp_packingks_ms.rs line 516 at r2 (raw file):
let mut noise_samples_after_ms = vec![]; let sample_count_per_msg = 100usize;
not sample count I was expecting I think
tfhe/src/integer/gpu/list_compression/server_keys.rs line 208 at r2 (raw file):
) -> CudaGlweCiphertextList<T> { let mut output_cuda_glwe_list = CudaGlweCiphertextList::new( self.meta.as_ref().unwrap().glwe_dimension,
consider managing the case wher the meta is empty, and also use unwrap only once and get the info you need from a single point
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/utils/noise_simulation.rs line 761 at r2 (raw file):
} /// Wrapper struct for CudaCompressionKey to implement packing keyswitch traits pub struct CudaPackingKeyswitchKey64<'a> {
why is this needed ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/utils/noise_simulation.rs line 870 at r2 (raw file):
/// GPU packing keyswitch key wrapper for noise distribution tests pub struct CudaPackingKeyswitchKey128<'a> {
why do you need this ?
tfhe/src/integer/gpu/mod.rs line 10422 at r2 (raw file):
let packed_glwe_list_ffi = prepare_cuda_packed_glwe_ct_ffi(glwe_list); if std::mem::size_of::<T>() == 16 {
use T::BITS
same thing elsewhere
tfhe/src/integer/gpu/mod.rs line 10429 at r2 (raw file):
glwe_index, ); } else if std::mem::size_of::<T>() == 8 {
use T::BITS
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 349 at r2 (raw file):
// Compare that all the results are equivalent vector_pattern_cpu
compare both vectors directly ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 452 at r2 (raw file):
.squash_radix_ciphertext_noise(&sks, &cpu_after_mul) .unwrap(); let _cpu_squashed_noise_ct = cpu_squashed_radix_ct.packed_blocks()[0].lwe_ciphertext();
unused ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 459 at r2 (raw file):
// Validate After PBS result let _after_pbs_ct = pbs_result.as_ct_128_cpu(&streams);
unused ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 462 at r2 (raw file):
let squashed_noise_ct_cpu = cuda_squashed_radix_ct.to_squashed_noise_radix_ciphertext(&streams); let _squashed_noise_ct = squashed_noise_ct_cpu.packed_blocks()[0].lwe_ciphertext();
unused ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 466 at r2 (raw file):
// println!("After PBS CT: {:?}", after_pbs_ct.as_view()); // println!("Squashed Noise CT: {:?}", squashed_noise_ct.as_view()); // println!(
should this be kept ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 1002 at r2 (raw file):
) { sanity_check_encrypt_dp_ks_standard_pbs128_gpu( TEST_PARAM_MESSAGE_2_CARRY_2_KS_PBS_TUNIFORM_2M128,
consider updating all tests to take meta parameters
2875ede to
918cdcb
Compare
guillermo-oyarzun
left a comment
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.
i will leave them running during xmas, but the preliminary noise and pfail had passed.
@guillermo-oyarzun made 14 comments and resolved 4 discussions.
Reviewable status: 18 of 25 files reviewed, 10 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, and @mayeul-zama).
tfhe/src/core_crypto/gpu/mod.rs line 842 at r2 (raw file):
Previously, IceTDrinker wrote…
that does not look reasonable
what do you mean? I think this was from the previous PR
tfhe/src/integer/gpu/mod.rs line 10422 at r2 (raw file):
Previously, IceTDrinker wrote…
use T::BITS
same thing elsewhere
done
tfhe/src/integer/gpu/mod.rs line 10429 at r2 (raw file):
Previously, IceTDrinker wrote…
use T::BITS
done
tfhe/src/integer/gpu/list_compression/server_keys.rs line 208 at r2 (raw file):
Previously, IceTDrinker wrote…
consider managing the case wher the meta is empty, and also use unwrap only once and get the info you need from a single point
done
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/br_dp_packingks_ms.rs line 516 at r2 (raw file):
Previously, IceTDrinker wrote…
not sample count I was expecting I think
right, i was missing a 10x factor, always the magic number :)
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 349 at r2 (raw file):
Previously, IceTDrinker wrote…
compare both vectors directly ?
done
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 452 at r2 (raw file):
Previously, IceTDrinker wrote…
unused ?
yup all this test is not needed, i had just kept it to double check bitwise results against cpu but it turns out fft128 also uses some avx so that comparison is not always true, i removed it
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 459 at r2 (raw file):
Previously, IceTDrinker wrote…
unused ?
same than before, removed the whole test
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 462 at r2 (raw file):
Previously, IceTDrinker wrote…
unused ?
same than before, removed the whole test
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 466 at r2 (raw file):
Previously, IceTDrinker wrote…
should this be kept ?
same than before, removed the whole test
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 1002 at r2 (raw file):
Previously, IceTDrinker wrote…
consider updating all tests to take meta parameters
got it
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/utils/noise_simulation.rs line 761 at r2 (raw file):
Previously, IceTDrinker wrote…
why is this needed ?
not needed indeed, in a first place i was storing more things in the struct, and then i just end up removing all the extra things, now i removed the struct too xD
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/utils/noise_simulation.rs line 870 at r2 (raw file):
Previously, IceTDrinker wrote…
why do you need this ?
same as before, not needed so removed.
IceTDrinker
left a comment
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.
@IceTDrinker reviewed 10 files and all commit messages, made 4 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @agnesLeroy, @guillermo-oyarzun, and @mayeul-zama).
tfhe/src/core_crypto/gpu/mod.rs line 842 at r2 (raw file):
Previously, guillermo-oyarzun (Guillermo Oyarzun) wrote…
what do you mean? I think this was from the previous PR
the &mut * I think, why would it be needed ?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/br_dp_packingks_ms.rs line 516 at r2 (raw file):
Previously, guillermo-oyarzun (Guillermo Oyarzun) wrote…
right, i was missing a 10x factor, always the magic number :)
don't hesitate to make it a constant if it's easier for you, I can update the CPU tests later to also make use of the constant later on and centralize those constants
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 349 at r2 (raw file):
Previously, guillermo-oyarzun (Guillermo Oyarzun) wrote…
done
assert_eq!(vector_pattern_cpu, vector_non_pattern_cpu);
?
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 360 at r2 (raw file):
/// Test function to verify that the noise checking tools match the actual atomic patterns /// implemented in shortint for GPU fn sanity_check_encrypt_dp_ks_standard_pbs128_gpu_vs_cpu<P>(
maybe keep it for core crypto (or in a stash given it will be useful for Beka I think), as I was told there are subtle difference depending on the algorithm used to compute the fft between "scalar", avx2, avx512 code
918cdcb to
3036a0e
Compare
3036a0e to
ece4abc
Compare
guillermo-oyarzun
left a comment
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.
All tests passed, results available in:
https://www.notion.so/zamaai/2026-01-classical-128-PBS-pattern-2e15a7358d5e80c5bf56c8acc1dc38bd
@guillermo-oyarzun made 5 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, and @mayeul-zama).
tfhe/src/core_crypto/gpu/mod.rs line 842 at r2 (raw file):
Previously, IceTDrinker wrote…
the
&mut *I think, why would it be needed ?
right, changed it
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/br_dp_packingks_ms.rs line 516 at r2 (raw file):
Previously, IceTDrinker wrote…
don't hesitate to make it a constant if it's easier for you, I can update the CPU tests later to also make use of the constant later on and centralize those constants
indeed that seems less error prone.
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 349 at r2 (raw file):
Previously, IceTDrinker wrote…
assert_eq!(vector_pattern_cpu, vector_non_pattern_cpu);
?
got it, changed it.
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 360 at r2 (raw file):
Previously, IceTDrinker wrote…
maybe keep it for core crypto (or in a stash given it will be useful for Beka I think), as I was told there are subtle difference depending on the algorithm used to compute the fft between "scalar", avx2, avx512 code
I had already shared it with him, he has his own way of validating it, so no need to keep it. We also decided that we will run the pbs128 tests with both flavors, after Beka merges his changes.
ece4abc to
c524b00
Compare
IceTDrinker
left a comment
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.
@IceTDrinker reviewed 4 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @agnesLeroy, @guillermo-oyarzun, and @mayeul-zama).
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 342 at r5 (raw file):
// Compare that all the results are equivalent assert_eq!(vector_pattern_cpu.len(), vector_non_pattern_cpu.len());
len check not required, it's done by the vec assert eq, sorry I thought you saw it was related
c524b00 to
04db51c
Compare
guillermo-oyarzun
left a comment
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.
@guillermo-oyarzun made 1 comment.
Reviewable status: 25 of 26 files reviewed, 2 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, and @mayeul-zama).
tfhe/src/integer/gpu/server_key/radix/tests_noise_distribution/dp_ks_pbs_128_packingks.rs line 342 at r5 (raw file):
Previously, IceTDrinker wrote…
len check not required, it's done by the vec assert eq, sorry I thought you saw it was related
oops, now i removed it.
IceTDrinker
left a comment
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.
Thanks !
@IceTDrinker reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @agnesLeroy and @mayeul-zama).
closes: please link all relevant issues
PR content/description
Check-list:
This change is