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

[Kirill Okhotnikov via Phabricator via libc-commits]

orex marked 5 inline comments as done.
orex added a comment.

Thank you, Intue for your useful comments. I implement all of them.
As for performance issues with denormalized values, I've checked the code of glibc e_fmod.c <https://github.com/bminor/glibc/blob/master/sysdeps/ieee754/dbl-64/e_fmod.c>. It looks like (I assume, but I did not check assembler code) that glibc function comes to denormalaized values faster than in my implementation, where I check most common cases first. I don't think that we should do something with it, because the case is very rare. I can hardly imaging it's real practical usage.

Kirill.



================
Comment at: libc/src/__support/FPUtil/FPBits.h:30
 };
 
 // A generic class to represent single precision, double precision, and quad
----------------
lntue wrote:
> `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!
I create a new helper with wrapped bulitins. libc/src/__support/FPUtil/builtin_wrappers.h


================
Comment at: libc/src/__support/FPUtil/generic/FMod.h:16
+
+#include <algorithm>
+#include <cerrno>
----------------
lntue wrote:
> 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.
All std includes were removed except cerrno. 


================
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())
----------------
lntue wrote:
> You might have to reimplement `std::optional` in `__support` or `__support/CPP` to prevent circular dependency.
Use "old style" returning optional values. I don't think, that `optional` reimplementing is needed for this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046