[libc-commits] [PATCH] D117028: [libc] Add multithreading support for exhaustive testing and MPFRUtils.
Tue Ly via Phabricator via libc-commits
libc-commits at lists.llvm.org
Wed Jan 12 11:29:20 PST 2022
lntue marked an inline comment as done.
lntue added inline comments.
================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.hpp:15
+#include <thread>
+#include <vector>
+
----------------
sivachandra wrote:
> We cannot include any of these C++ headers. The best would be to implement the `testFullRange` method in a `.cpp` file and explicitly instantiate all the variants.
>
> Or, even simpler, exhaustive tests are meaningful only for single-precision functions. So, why not avoid the template and have a simple non-template implementation in a `.cpp` file? You wouldn't need the `start` and `stop` args to `testFullRange`.
Moved to .cpp file.
The start and stop args are needed, even just for single-precision, since different functions might have different range of meaningful inputs. For example, logf can take the whole finite range, but expf take a much smaller range, or asin, acos will only produce finite outputs for inputs from [-1, 1].
================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.hpp:24
+
+ void testFullRange(T start, T stop, int nthreads, Func check) {
+ std::vector<std::thread> thread_list(nthreads);
----------------
sivachandra wrote:
> sivachandra wrote:
> > Instead of taking an argument for number of threads, can we may be use `std::thread::hardware_concurrency`?
> `testFullRange` should be named `test_full_range`.
Sometime I found myself needed to lower the number of threads, or even set it to 1. Since we are not allowed to include <thread> in here and in exhaustive_test.h, I don't think we can default it to std::thread::hardware_concurrency. Leave it hard-coded for now.
================
Comment at: libc/test/src/math/exhaustive/logf_test.cpp:26
+ template <mpfr::RoundingMode Rounding>
+ void check(uint32_t start, uint32_t stop) {
+ int old_rounding = fegetround();
----------------
sivachandra wrote:
> It seems to me that this function need not be a template? As in, the rounding mode can be an arg? You will need to add a rounding mode arg to `testFullRange` also I think.
I don't think rounding mode information should be included in test_full_range, as their sole purpose is to break down the input range and run it in parallel. So in order to move test_full_range implementation to .cpp file, the testing function has to have a fixed signature. That leave template parameter as the cleanest way to pass rounding mode to each testing function.
================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:558
MPFRNumber mpfrMatchValue(matchValue);
- OS << "Match value not within tolerance value of MPFR result:\n"
+ std::stringstream ss;
+ ss << "Match value not within tolerance value of MPFR result:\n"
----------------
sivachandra wrote:
> It's not clear to me as to why you need a `stringstream`. Can you explain why?
stringstream is needed so that the error messages are printed without interleaving each other when running concurrently.
================
Comment at: libc/utils/UnitTest/FPMatcher.cpp:70-71
template void describeValue<float>(const char *, float,
testutils::StreamWrapper &);
template void describeValue<double>(const char *, double,
----------------
michaelrj wrote:
> do these templates still work with the new code? It looks like you replaced the function they were using.
They are still being used by the default FPMatcher::explainError at libc/utils/UnitTest/FPMatcher.hpp:53-54.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117028/new/
https://reviews.llvm.org/D117028
More information about the libc-commits
mailing list