feat(core): use ntt-based polynomial multiplication for large polynomials #394

sarah-ek · 2023-06-29T09:59:02Z

PR content/description

use concrete-ntt for negacyclic polynomial multiplication. this speeds up keygen by 2x for MESSAGE_2_CARRY_2 and 6x for MESSAGE_4_CARRY_4

Check-list:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
Relevant issues are marked as resolved/closed, related issues are linked in the description
Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

github-actions · 2023-06-29T09:59:14Z

@slab-ci cpu_fast_test

IceTDrinker

Could you check the tests of the polynomial mul and cases to be sure we cover all the threshold domains ?

i.e. N < 64
64 <= N <= 256
256 < N

github-actions · 2023-07-18T12:40:54Z

@slab-ci cpu_fast_test

github-actions · 2023-07-18T12:55:29Z

@slab-ci cpu_fast_test

IceTDrinker

Thanks for the schoolbook test function, still the question about specific tests for each threshold range

IceTDrinker · 2023-07-19T15:05:34Z

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

-            let polynomial_log = (rng.gen::<usize>() % 7) + 6;
+            let polynomial_log = (rng.gen::<usize>() % 7) + 4;


so the range changes from 6..12 to 4..10 ?

can we maybe expand it or keep it the same ? as we now have params with polynomial_log up to 16 IIRC ?

IceTDrinker · 2023-07-19T15:06:52Z

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

 const KARATUSBA_STOP: usize = 64;
+const NTT_THRESHOLD: usize = 256;


I don't see the specialized/hard coded test cases for the various ranges of possible Polynomial sizes ? To be sure each domain is tested

github-actions · 2023-08-21T12:10:56Z

@slab-ci cpu_fast_test

mayeul-zama · 2023-08-21T12:49:26Z

tfhe/benches/keygen/bench.rs

    par_allocate_and_generate_new_lwe_bootstrap_key, ActivatedRandomGenerator, CiphertextModulus,
    EncryptionRandomGenerator, SecretRandomGenerator,
 };
 use tfhe::core_crypto::seeders::new_seeder;
 use tfhe::shortint::prelude::*;

 fn criterion_bench(c: &mut Criterion) {
-    let parameters = PARAM_MESSAGE_2_CARRY_2_KS_PBS;
+    let parameters = PARAM_MESSAGE_4_CARRY_4_KS_PBS;


Why change?

mayeul-zama · 2023-08-21T12:50:42Z

tfhe/benches/keygen/bench.rs

+    let small_lwe_secret_key = allocate_and_generate_new_binary_lwe_secret_key(
+        parameters.lwe_dimension,
+        &mut secret_generator,
+    );


I guess this is an unrelated fix

mayeul-zama · 2023-08-21T12:51:49Z

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

@@ -752,11 +815,11 @@ mod test {

    #[test]
    pub fn test_multiply_karatsuba_u32() {


I guess "karatsuba" should be removed from the name
Same comment for next test

mayeul-zama · 2023-08-21T13:00:52Z

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

-            // test
-            assert_eq!(&sb_mul, &ka_mul);
+                // test
+                assert_eq!(&sb_mul, &ka_mul);


Why not also compare with the ntt?

mayeul-zama · 2023-08-21T13:03:56Z

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

@@ -703,40 +748,58 @@ mod test {
        assert_eq!(&poly, &ground_truth);
    }

+    fn negacyclic_multiply_schoolbook<Scalar: UnsignedInteger>(


Could be used in the else block of polynomial_wrapping_add_mul_assign

IceTDrinker · 2023-08-28T07:25:18Z

Hey just back from vacation, where are we at for this ?

github-actions · 2023-09-08T08:07:32Z

@slab-ci cpu_fast_test

github-actions · 2023-09-08T08:23:26Z

@slab-ci cpu_fast_test

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

IceTDrinker · 2023-09-08T08:44:49Z

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs

+    /// test if we have the same result when using schoolbook or ntt/karatsuba
+    /// for random polynomial multiplication
+    fn test_sub_mul<T: UnsignedTorus>() {
+        // 50 times the test


comment is out of date

IceTDrinker

Two small comments, did you clear up the NTT overflow you mentioned at some point ?

I seem to recall you were worried some computations might overflow

…ials

github-actions · 2023-09-08T11:01:12Z

@slab-ci cpu_fast_test

sarah-ek · 2023-09-08T11:04:00Z

Two small comments, did you clear up the NTT overflow you mentioned at some point ?

I seem to recall you were worried some computations might overflow

i still haven't determined the overflow limit. i'll need to compute it from the prime values in concrete-ntt. just to be safe. i'll add a test case for a polynomial size of 2^16

IceTDrinker · 2023-09-08T11:05:00Z

Two small comments, did you clear up the NTT overflow you mentioned at some point ?
I seem to recall you were worried some computations might overflow

i still haven't determined the overflow limit. i'll need to compute it from the prime values in concrete-ntt. just to be safe. i'll add a test case for a polynomial size of 2^16

do we want to wait for you to have the overflow limit ? So that we are safe here before merging ?

sarah-ek · 2023-09-08T11:10:38Z

Two small comments, did you clear up the NTT overflow you mentioned at some point ?
I seem to recall you were worried some computations might overflow

i still haven't determined the overflow limit. i'll need to compute it from the prime values in concrete-ntt. just to be safe. i'll add a test case for a polynomial size of 2^16

do we want to wait for you to have the overflow limit ? So that we are safe here before merging ?

as long as one of the polynomial operands contains only small values (positive, or negative), then we should be safe
i believe this is the case since for keygen, one of the polynomials has only 0/1 values

IceTDrinker

For now as a safety, requesting changes until we have the thresholds for NTT potential overflows and a counter measure in place

IceTDrinker · 2023-09-08T11:25:04Z

Two small comments, did you clear up the NTT overflow you mentioned at some point ?
I seem to recall you were worried some computations might overflow

i still haven't determined the overflow limit. i'll need to compute it from the prime values in concrete-ntt. just to be safe. i'll add a test case for a polynomial size of 2^16

do we want to wait for you to have the overflow limit ? So that we are safe here before merging ?

as long as one of the polynomial operands contains only small values (positive, or negative), then we should be safe i believe this is the case since for keygen, one of the polynomials has only 0/1 values

Indeed for the small values, I know for research they used some very big polynomials when testing something we may want to check with JB to see what's preferred in that case, I just would like to avoid having silent failures in the NTT polynomial code

IceTDrinker · 2023-09-08T11:25:49Z

Maybe with unit tests with worst cases and not using NTT for the larger cases where there may be overflows but we need to be rock solid on that

IceTDrinker · 2023-09-13T08:13:25Z

@sarah-ek could we maybe separate the test overhaul in a different PR and merge it ? so that the improvements are in main and the ntt-only problem can be investigated without blocking the fixes you did ? 🙂

- initial work done in #394 - useful reworks of the tests have been waiting in that PR, this is to have those tests while NTT usage gets validated co-authored-by: sarah-ek

- initial work done in #394 - useful reworks of the tests have been waiting in that PR, this is to have those tests while NTT usage gets validated co-authored-by: sarah-ek <sarah.elkazdadi@zama.ai>

cla-bot bot added the cla-signed label Jun 29, 2023

IceTDrinker reviewed Jun 29, 2023

View reviewed changes

sarah-ek force-pushed the sk/feat/ntt-keygen branch from 5215448 to 32afcba Compare July 18, 2023 12:40

sarah-ek force-pushed the sk/feat/ntt-keygen branch from 32afcba to 6550bfa Compare July 18, 2023 12:55

sarah-ek requested a review from IceTDrinker July 19, 2023 14:17

IceTDrinker reviewed Jul 19, 2023

View reviewed changes

sarah-ek force-pushed the sk/feat/ntt-keygen branch from 6550bfa to cea0e28 Compare August 21, 2023 12:10

mayeul-zama reviewed Aug 21, 2023

View reviewed changes

sarah-ek force-pushed the sk/feat/ntt-keygen branch from cea0e28 to ffb5779 Compare September 8, 2023 08:07

sarah-ek force-pushed the sk/feat/ntt-keygen branch from ffb5779 to 432ddf9 Compare September 8, 2023 08:23

IceTDrinker reviewed Sep 8, 2023

View reviewed changes

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs Show resolved Hide resolved

IceTDrinker reviewed Sep 8, 2023

View reviewed changes

feat(core): use ntt-based polynomial multiplication for large polynom…

a28c475

…ials

sarah-ek force-pushed the sk/feat/ntt-keygen branch from 432ddf9 to a28c475 Compare September 8, 2023 11:01

IceTDrinker requested changes Sep 8, 2023

View reviewed changes

IceTDrinker mentioned this pull request Oct 11, 2023

test(core): use polynomial tests from NTT PR #624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): use ntt-based polynomial multiplication for large polynomials #394

feat(core): use ntt-based polynomial multiplication for large polynomials #394

sarah-ek commented Jun 29, 2023 •

edited

github-actions bot commented Jun 29, 2023

IceTDrinker left a comment

github-actions bot commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

IceTDrinker left a comment

IceTDrinker Jul 19, 2023

IceTDrinker Jul 19, 2023

github-actions bot commented Aug 21, 2023

mayeul-zama Aug 21, 2023

mayeul-zama Aug 21, 2023

mayeul-zama Aug 21, 2023

mayeul-zama Aug 21, 2023

mayeul-zama Aug 21, 2023

IceTDrinker commented Aug 28, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

IceTDrinker Sep 8, 2023

IceTDrinker left a comment

github-actions bot commented Sep 8, 2023

sarah-ek commented Sep 8, 2023

IceTDrinker commented Sep 8, 2023

sarah-ek commented Sep 8, 2023

IceTDrinker left a comment

IceTDrinker commented Sep 8, 2023

IceTDrinker commented Sep 8, 2023

IceTDrinker commented Sep 13, 2023

		let polynomial_log = (rng.gen::<usize>() % 7) + 6;
		let polynomial_log = (rng.gen::<usize>() % 7) + 4;

		const KARATUSBA_STOP: usize = 64;
		const NTT_THRESHOLD: usize = 256;

		@@ -752,11 +815,11 @@ mod test {

		#[test]
		pub fn test_multiply_karatsuba_u32() {

feat(core): use ntt-based polynomial multiplication for large polynomials #394

Are you sure you want to change the base?

feat(core): use ntt-based polynomial multiplication for large polynomials #394

Conversation

sarah-ek commented Jun 29, 2023 • edited

PR content/description

Check-list:

github-actions bot commented Jun 29, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2023

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

IceTDrinker commented Aug 28, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

Choose a reason for hiding this comment

IceTDrinker left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2023

sarah-ek commented Sep 8, 2023

IceTDrinker commented Sep 8, 2023

sarah-ek commented Sep 8, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

IceTDrinker commented Sep 8, 2023

IceTDrinker commented Sep 8, 2023

IceTDrinker commented Sep 13, 2023

sarah-ek commented Jun 29, 2023 •

edited