[libc-commits] [PATCH] D128995: [libc][math] Improved ExhaustiveTest performance.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.cpp:39-41
+          range_end = (stop - increment >= current_value)
+                          ? (current_value + increment)
+                          : stop;
----------------
This might be overflow when `stop < increment`, since both are unsigned?


================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.cpp:51
+          std::lock_guard<std::mutex> lock(mx_cout);
+          std::cout << new_percent << "% is in process     \r" << std::flush;
+        }
----------------
It is actually convenient to display the tested range whether it passed or not, so that we can save time by only running the failed ranges again while developing.  Also if you build the message separately before passing all at once to `std::cout`, you will not need this lock, since a single `<<` for `std::cout` is guaranteed to be atomic.


================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.h:20
 struct LlvmLibcExhaustiveTest : public __llvm_libc::testing::Test {
+  static constexpr T increment = (1 << 20);
   // Break [start, stop) into `nthreads` subintervals and apply *check to each
----------------
Probably this should be configurable from the caller side.  Few possibilities:
1) add `increment` to the last parameter of `test_full_range` with default value.
2) move rounding mode to a template parameter of `test_full_range` and add increment to the last parameter of `test_full_range` with default value.
3) add `increment` to as a template parameter of `test_full_range` with default value.
4) add both rounding mode and `increment` to template parameters of `test_full_range` with default value for `increment`.

Among these, I think option 2 will make the caller sides look the best.  What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128995