This is an archive of the discontinued LLVM Phabricator instance.

[libc] Enable MPFR library for math functions test
ClosedPublic

Authored by hedingarcia on Jul 27 2021, 10:46 AM.

Details

Summary

Included more math functions to Windows's entrypoints
and made a cmake option (-DLLVM_LIBC_MPFR_INSTALL_PATH)
where the user can specify the install path where the MPFR
library was built so it can be linked. The try_compile was
moved to LLVMLibCCheckMPFR.cmake, so the variable that is
set after this process can retain its value in other files
of the same parent file. A direct reason for this is for
LIBC_TESTS_CAN_USE_MPFR to be true when the user specifies
MPFR's path and retain its value even after leaving the file.

Diff Detail

Event Timeline

hedingarcia created this revision.Jul 27 2021, 10:46 AM
hedingarcia requested review of this revision.Jul 27 2021, 10:46 AM
aeubanks added inline comments.Jul 27 2021, 10:51 AM
libc/cmake/modules/LLVMLibCCheckMPFR.cmake
2

can you add an extra line after this?
and also at the very end of this file

4

just curious, what was the issue and fix to make ${LIBC_TESTS_CAN_USE_MPFR} work?

libc/utils/MPFRWrapper/CMakeLists.txt
13

message

[libc] Added newline at end of files

[libc] Fixed message statement

hedingarcia marked 3 inline comments as done.Jul 27 2021, 11:12 AM
hedingarcia added inline comments.
libc/cmake/modules/LLVMLibCCheckMPFR.cmake
4

The issue with it was that once set to true, if the path to MPFR was given, that value was not saved and evaluated False in libc/test/src/CMakeLists.txt and messages displayed that "WARNING: Math test...will be skipped as MPFR library is not available"; the test that needed MPFR did not ran. A way around it was to set it to the value in a cmake file separately and that variable will be carried to the subdirectorys utils and test. Hence carrying the True value in both the CMakeLists.txt in MPFRWrapper and on the one in test/src. Value was not being shared between two CMakeLists.txt siblings but they were passed from parent to child.

hedingarcia marked an inline comment as done.Jul 27 2021, 11:13 AM
sivachandra added inline comments.Jul 27 2021, 11:17 AM
libc/cmake/modules/LLVMLibCCheckMPFR.cmake
2

... where MPFR _is_ installed ... ?

Also, it would be nice if you can add something about this flag to step #9 here: https://github.com/llvm/llvm-project/tree/main/libc/config/windows#readme

[libc] Fixed the summary of LLVM_LIBC_MPFR_INSTALL_PATH and explained this option in README.md

A suggestion on the wording but good to go.

libc/config/windows/README.md
65

You don't have line breaks in this big chunk of text! I have a suggestion wrt the wording:

Some LLVM libc math unittests test correctness/accuracy against results from
the [GNU MPFR library](https://www.mpfr.org/). If you want to run math tests
which use MPFR, and if MPFR on your machine is not installed in the default
include and linker lookup directories, then you can specify the MPFR install
directory by passing an additional CMake option as follows:

-DLLVM_LIBC_MPFR_INSTALL_PATH=<path/mpfr/install/dir>

If the above option is specified, then `${LLVM_LIBC_MPFR_INSTALL_PATH}/include`
will be added to the include directories, and
`${LLVM_LIBC_MPFR_INSTALL_PATH}/lib` will be added to the linker lookup
directories.

NOTE: The GNU MPFR library depends on the
[GNU GMP library](https://gmplib.org/). If you specify the above option, then it
will be assumed that GMP is also installed in the same directory or availabe in
the default paths.
sivachandra accepted this revision.Jul 27 2021, 1:14 PM
This revision is now accepted and ready to land.Jul 27 2021, 1:14 PM

[libc] Changed the description in README.md to suggested message

hedingarcia marked 2 inline comments as done.Jul 27 2021, 1:29 PM
hedingarcia added inline comments.Jul 27 2021, 1:32 PM
libc/config/windows/README.md
65

I agree, the message conveys more clearly the prerequisites for this CMake option.

This revision was landed with ongoing or failed builds.Jul 27 2021, 1:40 PM
This revision was automatically updated to reflect the committed changes.