[libc-commits] [PATCH] D94018: [libc] Add implementation of fmaf.

[Tue Ly via Phabricator via libc-commits]

lntue marked an inline comment as done.
lntue added inline comments.


================
Comment at: libc/src/math/fmaf.cpp:22
+  fputil::FPBits<double> bit_sum(static_cast<double>(bit_prod) +
+                                 static_cast<double>(bitz));
+
----------------
sivachandra wrote:
> Nit: Can we use `z` directly?
Change to use bitz directly (which is already in double) to reduce the number of float -> double conversion needed (just in case compiler does not optimize it).


================
Comment at: libc/utils/FPUtil/FPBits.h:89
+                                  (cpp::IsIntegral<XType>::Value &&
+                                   (sizeof(XType) == sizeof(UIntType))),
+                              int> = 0>
----------------
sivachandra wrote:
> This will not be true for `long double` on i386. Will replacing it with an inequality be OK?
> 
> ```
> sizeof(XType) <= sizeof(UIntType)
> ```
the (in)equality only apply when XType is of Integral type, and we were expecting XType == UIntType even when T is long double.  That case was specialized before.  When T is long double on i386, we still define FPBits<long double>::UIntType=__ uint128_t and the previously specialized constructor becomes FPBits<long double>(__uint128_t), so I think equality works correctly.

Changing it to inequality, we will allow extra constructors such as FPBits<long double>(uint32_t), which will not be what we want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94018