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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.hpp:1
+//===-- Exhaustive test template for math functions -----------------------===//
+//
----------------
The LLVM style is to use a `.h` extension and `C++` in the license header.

https://llvm.org/docs/CodingStandards.html#self-contained-headers


================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.hpp:15
+#include <thread>
+#include <vector>
+
----------------
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`.


================
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);
----------------
Instead of taking an argument for number of threads, can we may be use `std::thread::hardware_concurrency`?


================
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:
> 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`. 


================
Comment at: libc/test/src/math/exhaustive/logf_test.cpp:10
+#include <fenv.h>
+#include <functional>
+
----------------
We cannot include C++ headers in a test also. See below for a possible solution for your current use case.


================
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();
----------------
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.


================
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"
----------------
It's not clear to me as to why you need a `stringstream`. Can you explain why?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117028/new/

https://reviews.llvm.org/D117028