This is an archive of the discontinued LLVM Phabricator instance.

[libc][NFC] refactor math implementations
AbandonedPublic

Authored by gchatelet on Dec 3 2021, 3:00 PM.

Details

Reviewers
lntue
Summary

Move math functions from src/math/generic to src/math folder.

Diff Detail

Event Timeline

gchatelet created this revision.Dec 3 2021, 3:00 PM
gchatelet requested review of this revision.Dec 3 2021, 3:00 PM
gchatelet updated this revision to Diff 391751.Dec 3 2021, 3:04 PM
  • Remove commented lines
gchatelet added subscribers: sivachandra, michaelrj.
gchatelet added inline comments.Dec 3 2021, 3:15 PM
libc/src/math/sqrt.cpp
9–24

@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

lntue added inline comments.Dec 3 2021, 5:21 PM
libc/src/math/sqrt.cpp
9–24

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.

sivachandra added inline comments.Dec 3 2021, 8:00 PM
libc/src/math/sqrt.cpp
9–24

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.

gchatelet planned changes to this revision.Dec 6 2021, 2:22 AM
gchatelet added inline comments.
libc/src/math/sqrt.cpp
9–24

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

gchatelet abandoned this revision.Nov 28 2022, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:59 AM
libc/src/math/truncf.cpp