[libc-commits] [PATCH] D127046: [libc][math] fmod/fmodf implementation.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/FPUtil/FPBits.h:173
+    // offset: +1 for sign, but -1 for implicit first bit
+    int lz = fputil::clz(number) - FloatProp::EXPONENT_WIDTH;
+    number <<= lz;
----------------
For safety, you might need to add a quick return for when `number == 0`:
```
  if (unlikely(number == 0)) return zero ...
```


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:70
+// https://en.cppreference.com/w/cpp/numeric/math/fmod
+template <typename T> class FModCStandardWrapper {
+
----------------
There is only one public method for this class, you can replace `class` with `struct` and remove `public:`.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:70
+// https://en.cppreference.com/w/cpp/numeric/math/fmod
+template <typename T> class FModCStandardWrapper {
+
----------------
lntue wrote:
> There is only one public method for this class, you can replace `class` with `struct` and remove `public:`.
Maybe rename the class to `FModErrorHandler` and add to the comment: following C99 standard with a link to `https://en.cppreference.com/w/c/numeric/math/fmod`


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:81
+
+    if (unlikely(fputil::isnan(x) || fputil::isnan(y))) {
+      out = fputil::quiet_NaN(T(0));
----------------
>From the C standard, `fmod(0, NaN)` and `fmod(0, inf)` should be `0`
```
If x is ±0 and y is not zero, ±0 is returned
```


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:87
+    if (unlikely(fputil::isinf(x) || y == 0)) {
+      force_eval<T>((x * y) / (y * x));
+      out = with_errno(fputil::quiet_NaN(T(0)), EDOM);
----------------
What's the main purpose of this evaluation?


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:95
+    }
+    if (unlikely(x == 0)) {
+      out = x;
----------------
This should be moved to above `isnan(x) || isnan(y)`, I don't think it is hit here.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:99
+    }
+    return false;
+  }
----------------
I don't think this line is hit, and all of the `unlikely`'s above are not needed.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:103
+
+template <typename T> class FModGCCInspiredWrapper {
+
----------------
We don't need this anymore, just use C standard.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:144
+  template <bool B = InverseMultiplication>
+  typename std::enable_if<
+      !B, intU_t>::type inline constexpr static division_loop(int n,
----------------
Instead of using `enable_if`with the boolean `InverseMultiplication`, it might be better to separate these 2 versions of `division_loop` into 2 separate structs and then pass them to the second template argument, similar to the way you do exceptional handling.  Something like:
```
// Comments explains what these functions do and what the input parameters are.
template<typename T>
struct FModDivisionLoop {
  static T execute(int n, int hyltzeros, T hx, T hy) { ... }
};
template<typename T>
struct FModInverseMultiplicationLoop {
  static T execute( ... )
};
```
Then the main class will be something like:
```
template<typename T, class ErrorHandler = FModErrorHandler<T>, class MainLoop = InverseMultiplicationLoop<T>>
class FMod { ... };
```


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:265
+public:
+  static inline T make(T x, T y) {
+    if (T out; Wrapper::PreCheck(x, y, out))
----------------
`FMod<T>::eval()` or `execute` instead of `make`?


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:25
+//    Some common  cases, like  abs(x) < abs(y)  or  abs(x) < 1000 *  abs(y) are
+//    treated specially to increase  performance.  The part of checking  special
+//    cases, numbers NaN, INF etc. is inspired by GCC libm implementation.
----------------
nit: treated *separately*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046