[libc-commits] [PATCH] D115082: [libc][NFC] refactor math implementations

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet planned changes to this revision.
gchatelet added inline comments.


================
Comment at: libc/src/math/sqrt.cpp:9-24
+#include "src/__support/architectures.h"
+
+#if defined(LLVM_LIBC_ARCH_AARCH64)
+#include "aarch64/sqrt.cpp"
+#elif defined(LLVM_LIBC_ARCH_X86)
+#include "x86_64/sqrt.cpp"
+#else
----------------
sivachandra wrote:
> lntue wrote:
> > gchatelet wrote:
> > > @lntue I'm not entirely sure that the redirection to subfolder makes sense. The inlined version would look like this
> > > 
> > > ```
> > > #include "sqrt.h"
> > > #include "src/__support/architectures.h"
> > > #include "src/__support/FPUtil/Sqrt.h"
> > > #include "src/__support/common.h"
> > > namespace __llvm_libc {
> > > 
> > > LLVM_LIBC_FUNCTION(double, sqrt, (double x)) {
> > > #if defined(LLVM_LIBC_ARCH_AARCH64)
> > >   double y;
> > >   __asm__ __volatile__("fsqrt %d0, %d1\n\t" : "=w"(y) : "w"(x));
> > >   return y;
> > > #elif defined(LLVM_LIBC_ARCH_X86)
> > >   double result;
> > >   __asm__ __volatile__("sqrtsd %x1, %x0" : "=x"(result) : "x"(x));
> > >   return result;
> > > #else
> > >   return fputil::sqrt(x);
> > > #endif
> > > }
> > > 
> > > } // namespace __llvm_libc
> > > ```
> > > 
> > > I find it more readable and it's less files overal. Let me know what you think
> > In my opinion, having #ifdef inside the function definition like this will be hard to read for more complicated functions, but for one-line functions like sqrt probably doesn't matter.  It is also a bit hard to figure out which one has arch specializations, and which arch is missing, compared to separate files.
> > 
> > Another option for separate files that I think work quite well for organizing files and readability is to use separate namespaces for separate arc, something like
> > 
> > #if define(LLVM_LIBC_ARCH_AARCH64)
> > #include ...
> > namespace impl = ::__llvm_libc::fputil::aarch64;
> > #elif defined(LLVM_LIBC_ARCH_X86)
> > #include ...
> > namespace impl = ::__llvm_libc::fputil::x86;
> > #else
> > #include ...
> > namespace impl = ::__llvm_libc::fputil::generic;
> > #endif
> > 
> > LLVM_LIBC_FUNCTION(double, sqrt, (double x)) { return impl::sqrt(x); }
> > 
> > One function that I'm optimizing is polyeval, for which by separating arch's, it is easier to manage and copy-paste than #if #else inside the real implementations.  But again, I'm fine with this one-line functions such as sqrt. 
> In general I favor what @lntue has said. Even if `sqrt` is just a one liner, I will vote for consistency across all one-liner and not-a-one-liner implemention.
SGTM, I agree that being able to know which arch is implemented just by glancing at the filesystem is valuable, also consistency is important. So I'll stick to the subfolder approach as you both of you suggested.

This patch and the following ones will largely impact the build system (CMakeLists.txt and downstream) so to save me some time I will move forward with D114712 first. Once Bazel setup is in place we will be able to restructure the build system more easily (no separate downstream fixes).

In the meantime, @michaelrj @lntue @sivachandra please move forward with your various pending patches, I'll rebase (or recreate) mine when I'm ready to move forward.

Thank you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115082