Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for modular build structure. #1160

Merged
merged 47 commits into from
Sep 6, 2024

Conversation

grafikrobot
Copy link
Member

@grafikrobot grafikrobot commented Jul 20, 2024

This is part of the effort to make the Boost libraries "modular" for build and consumption. See https://lists.boost.org/Archives/boost/2024/01/255704.php and https://github.com/grafikrobot/boost-b2-modular/blob/b2-modular/README.adoc for more information.

This PR depends on the following other PRs being merged to both develop and master branches of the respective repos:

This PR will be changed to ready for review, i.e. not draft, when the above are merged. Do not merge this one until that time.

@grafikrobot grafikrobot marked this pull request as ready for review August 18, 2024 14:12
@grafikrobot
Copy link
Member Author

Please review and merge this PR at your earliest convenience.

@jzmaddock
Copy link
Collaborator

@grafikrobot There's a lot of failures here, for example:

gcc.compile.c++ ../../../bin.v2/libs/math/test/test_zeta_floatmax_t.test/gcc-14/debug/x86_64/link-static/threading-multi/visibility-hidden/float128/test_zeta.o
In file included from float128/test_zeta.cpp:6:
float128/setup.hpp:19:62: error: static assertion failed: These tests should only be run for 128-bit floating point types.
   19 | static_assert(std::numeric_limits<boost::floatmax_t>::digits == 113, "These tests should only be run for 128-bit floating point types.");
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
float128/setup.hpp:19:62: note: the comparison reduces to ‘(64 == 113)’

That test, and all the float128 tests are predicated on:

            [ check-target-builds ../config//has_128bit_floatmax_t "128-bit floatmax_t" : : <build>no ]

And that configure step is showing "no" and so should NOT be building?

@grafikrobot
Copy link
Member Author

Hm.. Strange.. Will investigate.

@jzmaddock
Copy link
Collaborator

@grafikrobot thanks for the fixes, however there are still configuration related failures, for example if we stick to this job: https://drone.cpp.al/boostorg/math/2055/66/2 then the first failure is a linker error for test_2F0 - it's looking for libquadmath which isn't present. The Jamfile for that target has:

[ check-target-builds ../config//has_float128 "GCC libquadmath and __float128 support" : <define>BOOST_MATH_TEST_FLOAT128 <linkflags>"-Bstatic -lquadmath -Bdynamic" ]

and the configuration output says:

   - GCC libquadmath and __float128 support : no [3]

But it's still trying to link against it.

There are other configuration failings as well, that's just the first one in the log.

Thanks!

mborland and others added 3 commits August 30, 2024 20:52
Fix igamma_large support on device

Add GPU support to toms748

Add GPU support to igamma_inv

Add GPU markers to gamma_inva

Add GPU Markers to lgamma_small

Remove STL usage from gamma

Remove NVRTC workaround

Fix fraction use of STL headers

Mark gamma functions in fwd

Disable declval on all GPU platforms

Disable more unneeded code on device

Add forward decl for NVRTC tgamma

Disable unneeded items for all GPU

Change workaround for missing overloads

Rearrange definition location

Add include path to cuda now that workaround is removed

Fix NVRTC incompatibility with recursion and forward decls

Add tgamma_ratio CUDA and NVRTC testing

Fix NVRTC handling of gamma_p_derivative

Add gamma_p_derivative CUDA and NVRTC testing

Remove recursion from gamma_incomplete_imp

Add SYCL testing of igamma, igamma_inv, and igamma_inva

Ignore literal-range warnings

Remove use of static const char* for function name

Fix missing CUDA header

Remove calls under NVRTC to fwd decl

Add more nvrtc workarounds

Use builtin erfc instead of header cycle

Add CUDA and NVRTC testing of gamma_p_inv

Adjust tolerances

Add GPU support to chi squared dist

Fix static local variable

Add chi squared dist SYCL testing

Add chi squared dist CUDA testing

Add chi squared dist NVRTC testing

Add GPU support to weibull dist

Add weibull dist SYCL testing

Add weibull dist CUDA testing

Add weibull dist NVRTC testing
@grafikrobot
Copy link
Member Author

@jzmaddock I think I failed to understand how you are doing those checks. And actually I'm still failing. :-) What I tried to do was to normalize/abstract the various float18 config checks into one variable for reuse. But I didn't notice you use the checks differently in various places. So let me ask this.. In one place you use:

[ check-target-builds ../config//has_intel_quad "Intel _Quad datatype support" : <cxxflags>-Qoption,cpp,--extended_float_type <define>BOOST_MATH_USE_FLOAT128 ]

And in another you use:

[ check-target-builds ../config//has_intel_quad "Intel _Quad datatype support" : <cxxflags>-Qoption,cpp,--extended_float_type ]

Note the missing <define>BOOST_MATH_USE_FLOAT128. And the other checks has_float18 and has_128bit_floatmax_t also differ in various uses. My questions, to try and fix my attempt:

  1. Are the individual uses of each check meant to the same everywhere?
  2. Should all three of the checks be used together as is done in rule get_float128_tests?

@grafikrobot
Copy link
Member Author

@jzmaddock AFAICT the issues with quadmath are now resolved. But it would be great to get an answer on the consistency question of the trio of float128 checks. Or rather, on the lack of consistency in the many places those are copy+pasted which I fixed?

@jzmaddock
Copy link
Collaborator

Apologies Rene, no they are not consistent, and no they are not really meant to be, each test has it's own set of requirements, some of them are "if this is available link to it because a dependency needs it", some are "if this is available enable testing" and so on.

Plus it's not just the float128 configurations that are failing: I highlighted that because it was the first and most numerous thing to show up, but there are others as well as I think.

@jzmaddock
Copy link
Collaborator

Plus it's not just the float128 configurations that are failing: I highlighted that because it was the first and most numerous thing to show up, but there are others as well as I think.

Or perhaps not as this appears to be green now.

In principle though, shouldn't each conditional usage be able to define it's own set of requirements depending on what that target needs?

@grafikrobot
Copy link
Member Author

grafikrobot commented Sep 1, 2024

Plus it's not just the float128 configurations that are failing: I highlighted that because it was the first and most numerous thing to show up, but there are others as well as I think.

Or perhaps not as this appears to be green now.

In principle though, shouldn't each conditional usage be able to define it's own set of requirements depending on what that target needs?

In principle you can do whatever you like depending on what you need and intended. And it's precisely your INTENT that I'm asking about because I definitely do not know what math testing needs to do. I'm just guessing to what I think you might have intended. So to reformulate the question.. Here are the variations of the use of the has_float128, has_intel_quad, and has_128bit_floatmax_t appearing in the current Jamfile with counts of appearance:

55: [ check-target-builds ../config//has_float128 "GCC libquadmath and __float128 support" : <define>BOOST_MATH_TEST_FLOAT128 <linkflags>"-Bstatic -lquadmath -Bdynamic" ]
01: [ check-target-builds ../config//has_float128 "GCC libquadmath and __float128 support" : <linkflags>"-Bstatic -lquadmath -Bdynamic" : <build>no ]
13: [ check-target-builds ../config//has_float128 "GCC libquadmath and __float128 support" : <linkflags>"-Bstatic -lquadmath -Bdynamic" ]
02: [ check-target-builds ../config//has_float128 "__float128" : : <build>no ]
75: [ check-target-builds ../config//has_float128 "GCC libquadmath and __float128 support" : <linkflags>-lquadmath ]

01: [ check-target-builds ../config//has_intel_quad "Intel _Quad datatype support" : <cxxflags>-Qoption,cpp,--extended_float_type <define>BOOST_MATH_USE_FLOAT128 ]
01: [ check-target-builds ../config//has_intel_quad "Intel _Quad datatype support" : <cxxflags>-Qoption,cpp,--extended_float_type ]

01: [ check-target-builds ../config//has_128bit_floatmax_t "128-bit floatmax_t" : : <build>no ]

Question is.. Did you intend to make all those different? Or should they be uniform and it's a historical mistake that they differ?

Do note that I only rewrote the cases of [ check-target-builds ../config//has_float128 "GCC libquadmath and __float128 support" : <define>BOOST_MATH_TEST_FLOAT128 <linkflags>"-Bstatic -lquadmath -Bdynamic" ]. The rest stayed the same.

Also, generally, the reasons to specify what those checks are in a simple place near the top of the jamfile are:

  • To avoid possible copy+paste+edit errors over time.
  • To minimize Jamfile processing time, as each [ check-target-builds .. ] instance does a bunch of work. There's no optimization of repeated invocations in the interpreted world of B2 jamfiles.

Further note.. Those are not the only copy+pasted check-target-builds cases. Things could be further de-duplicated for the rest.

HTH

@mborland
Copy link
Member

mborland commented Sep 3, 2024

Question is.. Did you intend to make all those different? Or should they be uniform and it's a historical mistake that they differ?

has_float128 and has_intel_quad are separate because they represent two different types that are platform specific. has_128bit_floatmax_t would be if either of the previous are defined. Can b2 differentiate between intel compiler being icpc or icpx? icpx is the new clang based intel compiler, and it does not support all the compiler flags that icpc uses.

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.77%. Comparing base (ab57b20) to head (56e82c0).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1160      +/-   ##
===========================================
- Coverage    94.09%   93.77%   -0.32%     
===========================================
  Files          780      654     -126     
  Lines        65851    53718   -12133     
===========================================
- Hits         61960    50375   -11585     
+ Misses        3891     3343     -548     
Files with missing lines Coverage Δ
test/test_bernoulli_constants.cpp 83.72% <ø> (ø)

... and 180 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab57b20...56e82c0. Read the comment docs.

@mborland
Copy link
Member

mborland commented Sep 4, 2024

This is effectively green now with only Codecov failing. Based on the massive diff stat shown in the codecov comment it looks like something went wrong parsing an upload since all of those runs are green. Anything else @jzmaddock before I squash and merge this?

@mborland mborland merged commit 41f07b0 into boostorg:develop Sep 6, 2024
78 of 79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants