overflowing neg

[tmontaigu]

This change is 

[nsarlin-zama]

Reviewed 9 of 13 files at r1, all commit messages.
Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @agnesLeroy)

---

tfhe/src/integer/server_key/radix_parallel/neg.rs line 122 at r1 (raw file):

            // And in unsigned integers, the only case that is not an overflow is -0,
            // so we can just invert the result
            self.boolean_bitnot_assign(&mut overflowed);

I am not sure I understand this correctly, but I don't think this is how overflows for neg should be handled. If you take the rust doc for neg on unsigned and signed:

• signed: overflows when input is i32::MIN (0x80000000)
• unsigned: overflows when input is not 0

I think the case where !input + 1 overflows is not considered an overflow of the neg operation because it still yields the correct result. The fact that the addition overflows is an implementation detail but does not really matter as the result is correct for the user. On intel CPU the neg operation behaves as "0 - input" so the OF is not set when input is 0.

neg overflows for i32::MIN because the 2's complement representation is not balanced so there is no correct value for "-i32::MIN". I think for unsigned it overflows everywhere except for 0 because you can't represent negative numbers.

---

tfhe/src/integer/gpu/server_key/radix/bitwise_op.rs line 104 at r1 (raw file):

        streams: &CudaStreams,
    ) {
        // We do (-ciphertext) + (msg_mod -1) as it allows to avoid an allocation

I'm not sure because of the cuda impl, but since this is only a boolean maybe there is an easier way to do the bitnot ?

[tmontaigu]

tfhe/src/integer/server_key/radix_parallel/neg.rs line 122 at r1 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

I am not sure I understand this correctly, but I don't think this is how overflows for neg should be handled. If you take the rust doc for neg on unsigned and signed:

• signed: overflows when input is i32::MIN (0x80000000)
• unsigned: overflows when input is not 0

I think the case where !input + 1 overflows is not considered an overflow of the neg operation because it still yields the correct result. The fact that the addition overflows is an implementation detail but does not really matter as the result is correct for the user. On intel CPU the neg operation behaves as "0 - input" so the OF is not set when input is 0.

neg overflows for i32::MIN because the 2's complement representation is not balanced so there is no correct value for "-i32::MIN". I think for unsigned it overflows everywhere except for 0 because you can't represent negative numbers.

The end result we have is correct, for unsigned, 0 will not be considered an overflow for unsigned (but all others will),and for signed only the min will overflow

And to me this is the correct way to handle it:

doing 0 -input is like doing 0 + -input <=> 0 + (!input + 1) <=> !input + 1`

However, when doing 0 + (!input + 1) since that is like doing an subtraction in twos complement, it means the that for unsigned integer neither the carry flag nor oveflowflag are valid indicators of overflow. You can look at unsigned oveflowing sub which uses the subtraction with borrow

[tmontaigu]

tfhe/src/integer/server_key/radix_parallel/neg.rs line 122 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

The end result we have is correct, for unsigned, 0 will not be considered an overflow for unsigned (but all others will),and for signed only the min will overflow

And to me this is the correct way to handle it:

doing 0 -input is like doing 0 + -input <=> 0 + (!input + 1) <=> !input + 1`

However, when doing 0 + (!input + 1) since that is like doing an subtraction in twos complement, it means the that for unsigned integer neither the carry flag nor oveflowflag are valid indicators of overflow. You can look at unsigned oveflowing sub which uses the subtraction with borrow

So the comment is just trying to explain that we cant use the overflow flag since we are in unsigned mode, but 'luckily' flipping it yields correct result, otherwise we would have to do a sub_with_borrow(0, input which would need a full implementation, so its just faster and less code to use this trick

[tmontaigu]

tfhe/src/integer/gpu/server_key/radix/bitwise_op.rs line 104 at r1 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

I'm not sure because of the cuda impl, but since this is only a boolean maybe there is an easier way to do the bitnot ?

What do you have in mind ? I just took the code used for CudaUnsignedRadixCiphertext, and boolean block is defined as pub struct CudaBooleanBlock(pub CudaUnsignedRadixCiphertext); so apart from hardcoding a Vec of len 1 , I dont seen any possible changes

[nsarlin-zama]

Looks good to me!

Reviewed 4 of 13 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @agnesLeroy)

---

tfhe/src/integer/gpu/server_key/radix/bitwise_op.rs line 104 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

What do you have in mind ? I just took the code used for CudaUnsignedRadixCiphertext, and boolean block is defined as pub struct CudaBooleanBlock(pub CudaUnsignedRadixCiphertext); so apart from hardcoding a Vec of len 1 , I dont seen any possible changes

Ok it just felt weird to have to build the vec but it looks like gpu operations only work on ciphertext lists

---

tfhe/src/integer/server_key/radix_parallel/neg.rs line 122 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

So the comment is just trying to explain that we cant use the overflow flag since we are in unsigned mode, but 'luckily' flipping it yields correct result, otherwise we would have to do a sub_with_borrow(0, input which would need a full implementation, so its just faster and less code to use this trick

Ok sorry I misunderstood the comment. Indeed this looks correct!

[tmontaigu]

tfhe/src/integer/server_key/radix_parallel/neg.rs line 122 at r1 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

Ok sorry I misunderstood the comment. Indeed this looks correct!

I can update the comment in the code so that it may be clearer

[nsarlin-zama]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @agnesLeroy)

---

tfhe/src/integer/server_key/radix_parallel/neg.rs line 122 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

I can update the comment in the code so that it may be clearer

Maybe just a function doc comment that specifies the cases where it overflows for signed and unsigned will do it.

What confused me is that I thought that the overflow in the unsigned was the opposite of the signed case, but I forgot that overflowing_scalar_add_assign_parallelized was generic too so it was already handling signed and unsigned overflows differently.