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

[Tue Ly via Phabricator via libc-commits]

lntue added a comment.

Thanks for adding support for `fmod` functions with improved performance on regular range!  Let's discuss a bit more to see if we can also improve or maintain the performance for denormal inputs.



================
Comment at: libc/src/__support/FPUtil/FPBits.h:30
 };
 
 // A generic class to represent single precision, double precision, and quad
----------------
`clz` are also used in `sqrt` functions, which would also be used again by `FMA` function.  Would you mind factoring clz functions to another library similar to https://reviews.llvm.org/D124495 ?  You can overwrite what I did over there, as this change should be landed before that one.  Thanks!


================
Comment at: libc/src/__support/FPUtil/generic/CMakeLists.txt:19
+)
\ No newline at end of file

----------------
Please fix.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:16
+
+#include <algorithm>
+#include <cerrno>
----------------
I'm not sure that we can include that many C++ standard headers in here,  as it might introduce circular dependency among the libraries.  @sivachandra should have known more about these than me.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:82
+  static std::optional<T> PreCheck(T x, T y) {
+    if (unlikely(y == 0 || std::isnan(y) || !std::isfinite(x))) {
+      force_eval<T>((x * y) / (y * x));
----------------
I don't think you would want to use `std::isnan` or `std::isfinite` in here.  As you can imagine, `FPUtil` functions would be the ones to provide the *backbone* of `libc`, and hence `std::` functions, so if FPUtil functions depending on `std::` math functions, it would likely create circular dependency when building or linking.  So it's best to reuse or reimplement those simple math functions in the `FPUtil / FPBits` themselves.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:112
+  using intU_t = typename FPB::UIntType;
+  static constexpr intU_t set_normal_value(intU_t a) {
+    return (FPB::FloatProp::MANTISSA_MASK + 1) |
----------------
This function seems to better be in `FPBits` class.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:161
+
+  inline static constexpr FPB make_value(intU_t number, int ep) {
+    FPB result;
----------------
This function seems to be generic enough to be a static function in `FPBits` class.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:229
+      // Shift hy right until the end or n = 0
+      int right_shift = std::min(n, tzhy);
+      hy >>= right_shift;
----------------
We might not be able to use `std::min` here.


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:256
+  static inline T make(T x, T y) {
+    std::optional<T> out = Wrapper::PreCheck(x, y);
+    if (out.has_value())
----------------
You might have to reimplement `std::optional` in `__support` or `__support/CPP` to prevent circular dependency.


================
Comment at: libc/src/math/generic/CMakeLists.txt:1119
+)
\ No newline at end of file

----------------
Please fix.


================
Comment at: libc/test/src/math/differential_testing/CMakeLists.txt:491
+)
\ No newline at end of file

----------------
Please fix.


================
Comment at: libc/test/src/math/exhaustive/FMod_test.cpp:14
+#include <array>
+// #include <cmath>
+#include <limits>
----------------
Is this still needed?


================
Comment at: libc/test/src/math/exhaustive/FMod_test.cpp:43
+      for (int iy = min2; iy < max2; iy++) {
+        T y = by * std::pow<T>(2, iy);
+        if (y == 0 || !std::isfinite(y))
----------------
`by * 2^iy` is better implemented with `ldexp` functions https://en.cppreference.com/w/cpp/numeric/math/ldexp 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046