zama-ai / tfhe-rs

TFHE-rs: A Pure Rust implementation of the TFHE Scheme for Boolean and Integer Arithmetics Over Encrypted Data.
Other
903 stars 143 forks source link

feat(gpu): Implement unsigned_overflowing_scalar_add #1234

Closed bbarbakadze closed 3 months ago

bbarbakadze commented 3 months ago

PR content/description

Implement overflowing scalar add

Check-list:

tmontaigu commented 3 months ago

I don't think the first commit is right with regards to how the GPU works compared to CPU

On the CPU, when the input to a PBS is a trivial ciphertext, we actually skip the PBS and apply the LUT directly via clear operations, so the output is still a trivial ciphertext without noise https://github.com/zama-ai/tfhe-rs/blob/e825277219cc142bf13959b486192ddc97711445/tfhe/src/shortint/server_key/mod.rs#L750

I don't think you do that on GPU, meaning that you apply PBS on trivial inputs meaning the output has noise and is no longer a trivial

This is something you should probably also do if possible as it allows using trivial ciphertext to debug fhe operations much faster because the computations are faster (no pbs) and since there is no noise you can use a debuger/print-debug to see values and then figure out what is wrong

agnesLeroy commented 3 months ago

Ok so we'll remove the first commit and in case the gpu feature is activated we won't check if NoiseLevel is zero in case a trivial ciphertext is given as input to unsigned_overflowing_scalar_add

agnesLeroy commented 3 months ago

We just pushed the change

tmontaigu commented 3 months ago

No you should keep that check I think,

Because on GPU you should also be able to see that the clear value being added trivialy overflows

https://github.com/zama-ai/tfhe-rs/blob/e825277219cc142bf13959b486192ddc97711445/tfhe/src/integer/server_key/radix_parallel/scalar_add.rs#L26

tmontaigu commented 3 months ago

Hum ok I looked at the wrong think, I check again