[libc-commits] [PATCH] D131095: [libc] Prevent overflow from intermediate results when adding UInt<N> values.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/CPP/UInt.h:74
   constexpr uint64_t add(const UInt<Bits> &x) {
-    uint64_t carry = 0;
+    bool carry = false;
     for (size_t i = 0; i < WordCount; ++i) {
----------------
orex wrote:
> Hi, Tue!
> 
> I was thinking about the implementation. I little bit worries about the performance. To many `if` here. I would like to propose you another solution. Of course it is up to you to accept it or not. The strongest point of the solution is slightly reduced number of arithmetic operations and only one "main" if. The second one triggered very rare.
> 
> ```
>     uint64_t carry = 0;
>     for (size_t i = 0; i < WordCount; ++i) {
>       // Will be wrapped if sum more than 2^(sizeof(x)) - 1
>       val[i] += x.val[i];
>       // If an overflow appears, the result is less than both of the initial
>       // variables
>       if (val[i] < x.val[i]) {
>         // Add previous carry. Overflow is not possible.
>         val[i] += carry;
>         // Put 1 to the next digits.
>         carry = 1;
>       } else {
>         val[i] += carry;
>         // Likely no overflow.
>         if (likely(val[i]) != 0)
>           carry = 0;
>         // else carry keeps value in case of carry = 0 it is simply 0 with
>         // no overflow in case of 1 this made overflow and propagates next.
>       }
>     }
>     return carry;
>   }
> ```
> 
Hi Kirill, thanks for thinking about improving the performance!  I should have added some more background to this patch.

The main reason I had to make it a bit complicated is that when using `UInt<128>` to replace `__uint128_t` (which we will need to do for targets without `__uint128_t` builtin supports), it failed some tests that check overflow flags, which are set by the intermediate computation such as `val[i] += x.val[i]` in your improvement or the previous implementation, while the overall `__uint128_t` addition is not overflowed.

Of course this change will make another issue pop up, that is now we don't set overflow flag/trap when the real sum in `__uint128_t` is overflowed.  That's why I left some todo for later patches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131095/new/

https://reviews.llvm.org/D131095