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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libc/src/math/fmaf.cpp:21
+  fputil::FPBits<double> bitz(static_cast<double>(z));
+  fputil::FPBits<double> bit_sum(static_cast<double>(bit_prod) + bitz);
+
----------------
What I meant was, instead of using the conversion operator, we can do something like:

```
fputil::FPBits<double> bit_sum(static_cast<double>(bit_prod) + static_cast<double>(z));
```

This avoids calling `FPBits` conversion operator. Likewise with `bit_prod` may be? That is, store the product in a `double` value instead of `FPBits` object, and use it to calculate the sum.

```
  double prod = static_cast<double>(x) * static_cast<double>(y);
  fputil::FPBits<double> bit_prod(prod);
  fputil::FPBits<double> bitz(static_cast<double>(z));
  fputil::FPBits<double> bit_sum(prod + static_cast<double>(z));
```


================
Comment at: libc/utils/FPUtil/FPBits.h:89
+                                  (cpp::IsIntegral<XType>::Value &&
+                                   (sizeof(XType) == sizeof(UIntType))),
+                              int> = 0>
----------------
lntue wrote:
> 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.
Ah, sorry I misread! Ignore my original comment.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:365
+  constexpr unsigned int prec = Precision<InputType>::value;
+  MPFRNumber inputX(x, prec), inputY(y, prec), inputZ(z, prec);
+  switch (op) {
----------------
sivachandra wrote:
> Can you add a comment explaining why we can't use a single large precision for all types?
Did you add any comment for this somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94018