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

[Tue Ly via Phabricator via libc-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115082