This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add implementations for sqrt, sqrtf, and sqrtl.
ClosedPublic

Authored by lntue on Jul 27 2020, 10:43 PM.

Details

Summary

[libc] Add implementations for sqrt, sqrtf, and sqrtl.

Diff Detail

Event Timeline

lntue created this revision.Jul 27 2020, 10:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 10:43 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2020, 11:11 PM
Harbormaster failed remote builds in B65958: Diff 281115!
lntue updated this revision to Diff 281356.Jul 28 2020, 2:04 PM

[libc] Add implementations for sqrt, sqrtf, and sqrtl.

lntue requested review of this revision.Jul 28 2020, 2:04 PM
hungptit added inline comments.
libc/src/math/CMakeLists.txt
498

NIT: compile options are altered by CMAKE_BUILD_TYPE parameters. Do we need to specify the compile options for every entry point?

libc/test/src/math/sqrt_test.cpp
24

NIT: constexpr?

libc/test/src/math/sqrtf_test.cpp
34

Do we plan to raise FE_INVALID?

libc/utils/FPUtil/SqrtLongDoubleX86.h
130

Do these clang-tidy go away if we rename this variable to extractedOutput? If we give this variable a better name then the above comment is redundant.

I need more time to read through the patch. But I have left few comments about the structuring of the code.

libc/utils/FPUtil/Sqrt.h
23

Make all functions implemented in header files static inline.

64

Instead of this, use: #if !(defined(__x86_64__) || defined(__i386__))

177

To keep FPUtil utility like, would it make sense to conditionally include SqrtLongDoubleX86.h. This will also avoid the conditional include in sqrtl.cpp and FPUtil/Sqrt.h will be the only file to include in other places where the square root function will probably be used (say in hypot).

lntue updated this revision to Diff 286473.Aug 18 2020, 9:22 PM
lntue marked 3 inline comments as done.

Update to the new floating point testing facility.

sivachandra accepted this revision.Aug 25 2020, 10:59 PM

Please follow up with patches to switch to NormalFloat and a doc explaining the algorithm.

This revision is now accepted and ready to land.Aug 25 2020, 10:59 PM
lntue added inline comments.Aug 26 2020, 6:21 AM
libc/test/src/math/sqrt_test.cpp
24

constexpr does not work with floating point yet.

libc/test/src/math/sqrtf_test.cpp
34

Floating point exceptions will be implemented in the future. Currently we prioritize expanding functionality first, unless there are immediate requests/needs.

libc/utils/FPUtil/SqrtLongDoubleX86.h
130

I think clang-tidy wants the variables' initials to be capitalized; it's a bit noisy

This revision was landed with ongoing or failed builds.Aug 26 2020, 6:46 AM
This revision was automatically updated to reflect the committed changes.