This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add multithreading support for exhaustive testing and MPFRUtils.
ClosedPublic

Authored by lntue on Jan 11 2022, 8:22 AM.

Details

Summary

Add threading support for exhaustive testing and MPFRUtils.

Diff Detail

Event Timeline

lntue created this revision.Jan 11 2022, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 8:22 AM
lntue requested review of this revision.Jan 11 2022, 8:22 AM
michaelrj added inline comments.Jan 11 2022, 4:32 PM
libc/utils/UnitTest/FPMatcher.cpp
70–71

do these templates still work with the new code? It looks like you replaced the function they were using.

sivachandra added inline comments.Jan 12 2022, 12:00 AM
libc/test/src/math/exhaustive/exhaustive_test.hpp
1 ↗(On Diff #398959)

The LLVM style is to use a .h extension and C++ in the license header.

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

15 ↗(On Diff #398959)

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.

24 ↗(On Diff #398959)

Instead of taking an argument for number of threads, can we may be use std::thread::hardware_concurrency?

24 ↗(On Diff #398959)

testFullRange should be named test_full_range.

libc/test/src/math/exhaustive/logf_test.cpp
10

We cannot include C++ headers in a test also. See below for a possible solution for your current use case.

23

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.

libc/utils/MPFRWrapper/MPFRUtils.cpp
590–591

It's not clear to me as to why you need a stringstream. Can you explain why?

lntue updated this revision to Diff 399392.Jan 12 2022, 11:21 AM
lntue marked 2 inline comments as done.

Move exhaustive_test implementation to .cpp file.

lntue marked an inline comment as done.Jan 12 2022, 11:29 AM
lntue added inline comments.
libc/test/src/math/exhaustive/exhaustive_test.hpp
15 ↗(On Diff #398959)

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].

24 ↗(On Diff #398959)

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.

libc/test/src/math/exhaustive/logf_test.cpp
23

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.

libc/utils/MPFRWrapper/MPFRUtils.cpp
590–591

stringstream is needed so that the error messages are printed without interleaving each other when running concurrently.

libc/utils/UnitTest/FPMatcher.cpp
70–71

They are still being used by the default FPMatcher::explainError at libc/utils/UnitTest/FPMatcher.hpp:53-54.

sivachandra added inline comments.Jan 12 2022, 11:51 AM
libc/test/src/math/exhaustive/exhaustive_test.h
27

Why is using a method pointer over an overridable virtual method with an additional rounding mode arg better? FWIW, method pointers are like virtual functions.

libc/test/src/math/exhaustive/logf_test.cpp
23

I am not sure what "including" rounding mode information in test_full_range would me. But, with the virtual function setup, it will be like this:

test_full_range(..., RoundingMode rounding_mode) {
  check(..., rounding_mode);
}

And it will avoid all the function pointers in the tests below. Also, the template arg is not deducible so there is not benefit to making it a template.

An additional note: Tests which don't need rounding mode will just ignore that arg - so there has to be a default.

libc/utils/MPFRWrapper/MPFRUtils.cpp
590–591

Ah, thanks. Can you add a note at a suitable place in this file?

lntue updated this revision to Diff 399546.Jan 12 2022, 10:42 PM

[libc] Add multithreading support for exhaustive testing and MPFRUtils.

lntue updated this revision to Diff 399548.Jan 12 2022, 10:55 PM

Add comments about the usage of std::stringstream.

lntue marked an inline comment as done.Jan 12 2022, 10:56 PM
lntue added inline comments.
libc/test/src/math/exhaustive/exhaustive_test.h
27

See the comment below in logf_test.cpp.

libc/test/src/math/exhaustive/logf_test.cpp
23

It's exactly passing rounding_mode directly to test_full_range that I didn't like and preferred method pointer over virtual function. Since if other tests need different parameters to execute, similar to rounding_mode, then they will have to pass to test_full_range, and all other overridden functions would have to be updated to carry more unused parameters.

Without template parameters (since its implementations is not in the header) and std::function, I found method pointers is the next easiest way to allow each test to add their own required parameters individually without affecting other tests.

Nonetheless, I updated to use virtual function in here, and we can change to method pointers later if more tests need different parameters.

sivachandra accepted this revision.Jan 12 2022, 11:22 PM
sivachandra added inline comments.
libc/test/src/math/exhaustive/logf_test.cpp
23

Ah - this sounds like, if you really need more parameterization in future, then we should parameterize the test class and not the test method? As in,

template <mpfr::RoundingMode Rounding>
class LlvmLibcLogfExhaustiveTest : public LlvmLibcExhaustiveTest<uint32_t> {
  ...
};

Virtual methods are there essentially there to avoid such "poor man's C++" one can setup using member function pointers. So, I think this virtual function based solution is more idiomatic.

This revision is now accepted and ready to land.Jan 12 2022, 11:22 PM