[libc-commits] [PATCH] D117028: [libc] Add multithreading support for exhaustive testing and MPFRUtils.

[Tue Ly via Phabricator via libc-commits]

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