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

Tue Jun 21 05:30:48 PDT 2022

orex 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;
----------------
lntue wrote:
> For safety, you might need to add a quick return for when `number == 0`:
> ```
>   if (unlikely(number == 0)) return zero ...
> ```
It was Siva suggestion to move the function here. There is another class called `NormalFloat` which can handle such things. I propose to move the function back to FMod and implement "full" functionality in that class. What do you think? `if number == 0` is not only one case which we need to check. `ep` value after processing below should also be checked for overflow. 

================
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:
> 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`
Sure.

================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:70
+// https://en.cppreference.com/w/cpp/numeric/math/fmod
+template <typename T> class FModCStandardWrapper {
+
----------------
orex wrote:
> lntue wrote:
> > 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`
> Sure.
It is not an error handler, but special numbers `cases` processor. Do you have another name in mind? If no, let's go for Error.

================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:81
+
+    if (unlikely(fputil::isnan(x) || fputil::isnan(y))) {
+      out = fputil::quiet_NaN(T(0));
----------------
lntue wrote:
> 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
> ```
Thank you. This is a very good comment. Obviously C standard "F.10.7.1" is not full. It do not describe at all, for example, NaN NaN case. I also do not think, that fmod(0, NaN) should return zero. I prefer to go for better consistent standard which are described here
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fmod.html
https://en.cppreference.com/w/c/numeric/math/fmod
What do you think about this?

================
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);
----------------
lntue wrote:
> What's the main purpose of this evaluation?
Thank you for the comment. I don't know any other approach to raise floating-point exception. Please suggest me something better.

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

================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:99
+    }
+    return false;
+  }
----------------
lntue wrote:
> I don't think this line is hit, and all of the `unlikely`'s above are not needed.
`return false`. Yes you are right, but I can't return nothing from the function. It will be a warning `warning: non-void function does not return a value in all control paths` I don't want my code to produce such things.
`unlikely` I would like to explicitly say compiler to make just one conditional jump in the function. It is not so big problem, of course, I can remove this.

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

================
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,
----------------
lntue wrote:
> 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 { ... };
> ```
Thank you. Sounds reasonable. I will do this. 

================
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))
----------------
lntue wrote:
> `FMod<T>::eval()` or `execute` instead of `make`?
Yes. Sounds better. Thank you.

================
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.
----------------
lntue wrote:
> nit: treated *separately*
Sure.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046