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

Kirill Okhotnikov via Phabricator via libc-commits libc-commits at lists.llvm.org
Sun Jun 5 12:29:37 PDT 2022

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.


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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list