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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115082