[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;
----------------
orex wrote:
> 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. 
The main reason you need to check for `number == 0` here is because `fputil::clz` is a wrapper around `__built_in_clz` and when the inputs are 0, the behavior/output is undefined.


================
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:
> 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.
I think technically these are not errors per se.  They actually handle exceptional inputs / outputs, and as a class name, it's more appropriate to use a noun.  So I think `FModExceptionalHandler` or `FModExceptionalInputHandler` or `Helper` might be better.


================
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);
----------------
orex wrote:
> 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.
You can use `https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/FEnvImpl.h` to set the floating point exceptions.  It's a bit more verbose but much clearer.  And these are exceptional cases, so we don't care much about the performance anyway.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:99
+    }
+    return false;
+  }
----------------
orex wrote:
> 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.
You can remove the last `if` and put `x == 0` in the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046