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

[Kirill Okhotnikov via Phabricator via libc-commits]

orex added inline comments.


================
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;
+        }
----------------
lntue wrote:
> orex wrote:
> > lntue wrote:
> > > 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.
> > That will be a lot of ranges, which is passed. I don't think it will be good to display them. Also you always have a values, which is not passed from testing macros, so you can use them to check problems. 
> Or just display the failed ranges?  So that you can redirect the outputs to a file and find the exact / smallest ranges to skip all the passed one?  Actually with your default option, `2^12` ranges is not too bad for output to a file.
Good proposition. Done.


================
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
----------------
lntue wrote:
> 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?
I can hardly imagine, that developers in general should take care about this. It looks like a technical thing. Anyhow I understand you point. I propose another solution. Make it virtual function, which default value in base class. If you would like to change the parameter, you can overload the function together with check function, which you should overload. What do you think?

About the template parameter of rounding. It is a good idea, but probably, we can think about to do all rounding and ranges with one command? Also, it is good to combine everything in one function like `exec`?
And you pass ranges and rounding as overload functions in `check` implementation class?

So, in derivative class you overload array of rounding, array of ranges and check function, of course. 

But, let's do as you always said several small steps.)))))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128995