shibatch / sleef

SIMD Library for Evaluating Elementary Functions, vectorized libm and DFT
https://sleef.org
Boost Software License 1.0
659 stars 131 forks source link

Add a new tester that utilizes tlfloat #574

Open shibatch opened 1 month ago

shibatch commented 1 month ago

Why a new feature? Is your feature request related to a problem? Please describe it.

What feature would you like?

How would you like this feature to be implemented?

How portable is the feature across architectures and platforms?

Describe alternative approaches you have considered

Additional context

blapie commented 2 weeks ago

Hello, Sorry for the delay in replying.

Here are a few general comments:

  1. This generally seems like an improvement and the integration of the new tester should be fairly smooth.

  2. As mentioned in the previous issue, the main worry is the addition of code, which means more maintenance work. However, because it solves a portability issue (in particular the posix-one) then I think it is worth the maintenance cost.

  3. I believe the project can greatly benefit from C++ in the long term (e.g. leverage templatisation to reduce the need for macros), in particular when it comes to tests and benchmarks. By the way we are currently reworking the benchmark suite using C++.

  4. I think the existing testers could use a bit of documentation (inline and on website) to facilitate maintenance.

Some answers/reactions to your issue:

TLFloat has fewer portability issues than MPFR and can be used in a wider range of environments.

I still wonder what sort of environment does not have access to MPFR? This did not occur to me that this could be a difficult dependency to satisfy.

However, it will require a C++20 compiler and cannot be compiled with clang 16 or earlier due to compiler bugs.

Restriction to C++20 and clang 16+, might be acceptable although not ideal. Maybe some of these bugs can be addressed?

I will allow the user to choose whether to build the current tester/iut and the new tester by cmake options.

If the user can choose between old and new tester then I suppose the C++ compiler dependency is not a huge issue.

shibatch commented 2 weeks ago

By the way we are currently reworking the benchmark suite using C++.

If so, could you briefly explain the plan in a new issue?

I think the existing testers could use a bit of documentation (inline and on website) to facilitate maintenance.

Yeah. We should add documentation across the board.

I still wonder what sort of environment does not have access to MPFR? This did not occur to me that this could be a difficult dependency to satisfy.

MPFR is quite troublesome to use on Windows. I guess we will have to test our library on Windows on ARM, sooner or later.

blapie commented 2 weeks ago

If so, could you briefly explain the plan in a new issue?

Sure, we will post about it in an issue shortly.

I guess we will have to test our library on Windows on ARM, sooner or later.

We have the ability to test on WoA internally. We work under Msys2 (which a unix-like environment) and it is relatively easy to install gmp, mpc and mpfr using the package manager pacman. The issue on WoA with Msys2 is the POSIX-dependency.

shibatch commented 2 weeks ago

Are you testing building SLEEF with MSVC? MinGW seems to be imcompatible with MSVC.

blapie commented 2 weeks ago

We are building with clang, it's proved simpler on many occasions. Not saying there isn't a way to make it work, just still haven't got round to building successfully.

shibatch commented 2 weeks ago

In any case, using MPFR with MSVC is a bit tricky, and I think it would be ideal to test SLEEF properly with MSVC. The new tester should be useful for this purpose.

blapie commented 2 weeks ago

Sure! Looking forward to the PR.

joanaxcruz commented 2 weeks ago

If so, could you briefly explain the plan in a new issue?

For now, we didn't create a issue as #9 is still open. In this issue, you discuss introducing googlebench framework. I decided to investigate a bit on this, and the following points made it appealing to me:

Should upload a PR introducing this new tool soon - will keep you updated.