Optimize and vectorize TOMS748 #5578

nilsdeppe · 2023-10-20T13:30:50Z

Proposed changes

I've had this on a branch for a long time, decided to clean it up. This is necessary for vectorizing FixConservatives and primitive recovery. FixConservatives was ~10x after vectorization, primitive recovery I don't remember if I did as thorough of a comparison, but I remember at least a 5x speedup. Our GRMHD simulations (haven't checked GHMHD) spend ~50% of their time in primitive recovery, so hopefully once that's all in we should see a big reduction in runtime there.

Upgrade instructions

Code review checklist

The code is documented and the documentation renders correctly. Run
make doc to generate the documentation locally into BUILD_DIR/docs/html.
Then open index.html.
The code follows the stylistic and code quality guidelines listed in the
code review guide.
The PR lists upgrade instructions and is labeled bugfix or
new feature if appropriate.

Further comments

wthrowe

And as a bonus, this should get rid of most of the stupid clang-tidy warnings from Boost.

wthrowe · 2023-10-20T18:56:39Z

cmake/SetupXsimd.cmake

@@ -6,6 +6,10 @@ option(USE_XSIMD "Use xsimd if it is available" OFF)
 if(USE_XSIMD)
  find_package(xsimd REQUIRED)

+  if(xsimd_VERSION VERSION_LESS 11.0.1)
+    message(FATAL_ERROR "xsimd must be at least 11.0.1, got ${xsimd_VERSION}")


Update installation requirements documentation.

wthrowe · 2023-10-20T18:58:57Z

cmake/SetupXsimd.cmake

-    xsimd
-    )
+  # target_link_libraries(
+  #   Blaze


Just want to make sure leaving this was intentional.

Yes, I'll add a comment. Basically, I haven't got all bugs out when doing Blaze+XSIMD in our case. I'm not sure if it's some padding assumption or what.

wthrowe · 2023-10-20T19:11:40Z

src/DataStructures/Tensor/Tensor.hpp

@@ -108,7 +109,8 @@ class Tensor<X, Symm, IndexList<Indices...>> {
          std::is_same_v<X, ComplexModalVector> or
          std::is_same_v<X, DataVector> or std::is_same_v<X, ModalVector> or
          is_spin_weighted_of_v<ComplexDataVector, X> or
-          is_spin_weighted_of_v<ComplexModalVector, X>,
+          is_spin_weighted_of_v<ComplexModalVector, X> or
+          simd::is_batch<X>::value,


Update the error message. (Maybe remove the list of types from it so the information isn't duplicated.)

wthrowe · 2023-10-20T19:12:19Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

@@ -6,15 +6,465 @@

 #pragma once

-#include <boost/math/tools/roots.hpp>


[arbitrary line] This is derived from boost code, so you need to include the boost license and copyright statement somewhere.

wthrowe · 2023-10-20T19:24:24Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

+    // `max() * (1. + value)`
+    const auto mask = fabs(simd::select(mask0, denom, static_cast<T>(0)) *
+                           std::numeric_limits<T>::max()) <= fabs(num);
+    return simd::select(mask0 and mask, r, num / denom);


Does num / denom not FPE here?

I have not had an issue, but I'll add a comment with code that could be changed to prevent that. This might have something to do with retiring instructions, etc. but SIMD things like that is something I don't have a complete grasp on. If we have issues, it'll be easy to guard against :)

Edit: I just stuck it in... It's one mask, not super expensive to compute.

wthrowe · 2023-10-20T19:56:26Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

+  // Main entry point and logic for Toms Algorithm 748
+  // root finder.
+  if (UNLIKELY(simd::any(ax >= bx))) {
+    throw std::domain_error("Lower bound is larger than upper bound");


Would allowing ax == bx cause any problems with the implementation or need special handling? If it would it's probably not worth allowing, but if not it seems mathematically OK (as long as the single-point bracket is a root, of course).

I'm assuming something subtle breaks in the implementation. The Boost implementation doesn't allow it so I just did that rather than possibly creating some weird edge case

wthrowe · 2023-10-20T20:05:31Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

+    u = simd::select(fabs_fa_less_fabs_fb_mask, a, b);
+    fu = simd::select(fabs_fa_less_fabs_fb_mask, fa, fb);
+    const T b_minus_a = b - a;
+    // need to mask out fb==fa case


Is this a TODO? I don't see any code handling this.

wthrowe · 2023-10-20T20:06:15Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

+    const T b_minus_a = b - a;
+    // need to mask out fb==fa case
+    c = simd::fnma(static_cast<T>(2) * (fu / (fb - fa)), b_minus_a, u);
+    // c = u - static_cast<T>(2) * (fu / (fb - fa)) * b_minus_a;


Remove comment or move before the line it's documenting.

wthrowe · 2023-10-20T20:13:23Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

 * \throws `convergence_error` if, for any index, the requested tolerance is not
- * met after `max_iterations` iterations.
+p * met after `max_iterations` iterations.


cmake/SetupXsimd.cmake

nilsvu · 2023-10-20T20:26:04Z

src/PointwiseFunctions/AnalyticSolutions/Xcts/CMakeLists.txt

@@ -42,6 +42,7 @@ target_link_libraries(
  Options
  RelativisticEulerSolutions
  Serialization
+  Simd


Make PRIVATE and add PRIVATE RootFinding please.

tests/Unit/NumericalAlgorithms/RootFinding/Test_TOMS748.cpp

nilsdeppe · 2023-10-21T03:55:15Z

Pushed fixups. Thanks for the reviews!

wthrowe

Other changes look good.

wthrowe · 2023-10-22T17:01:31Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

+//  (C) Copyright John Maddock 2006.
+//  Use, modification and distribution are subject to the
+//  Boost Software License, Version 1.0. (See accompanying file
+//  LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)


You're missing the actual license file.

I added the file. Much to my surprise it looks like installed versions of Boost also don't necessarily include it, but I agree we should include the license for it.

nilsdeppe · 2023-10-23T17:06:24Z

I added a couple more fixups that change the way the size() function works and also adds a make_sequence function

nilsdeppe · 2023-10-24T14:04:36Z

@nilsvu could you please take a look at the fixups? @wthrowe I changed a couple small features in the simd lib in the last fixups (and a linking issue) if you have a chance to take a look. The linking issue was what was causing CI to fail.

wthrowe · 2023-10-24T17:57:35Z

Looks good.

nilsvu

you can squash

src/Evolution/Systems/GrMhd/ValenciaDivClean/LICENSE_1_0.txt

src/PointwiseFunctions/AnalyticSolutions/Xcts/Schwarzschild.cpp

tests/Unit/NumericalAlgorithms/RootFinding/Test_TOMS748.cpp

nilsvu · 2023-10-24T18:19:34Z

Any idea why the root finder in the tabulated EOS test failed to converge in the macOS CI test?

nilsdeppe · 2023-10-24T20:19:11Z

No, I have no idea why it doesn't work on macOS. I tried reproducing on my machine (Linux) with the same seeds, etc. and can't.

Edit: I'm pushing a fixup commit that should print out more info (tolerances and result) so we can get some idea of how well it did.

nilsdeppe · 2023-10-26T14:50:27Z

@nilsvu @wthrowe this should be ready now...

wthrowe

Can you give a (very short) summary of what you changed since the last time we looked at this? Was it just adjusting tolerances or similar?

src/Evolution/Systems/GrMhd/ValenciaDivClean/LICENSE_1_0.txt

nilsdeppe · 2023-10-26T18:33:39Z

src/NumericalAlgorithms/RootFinding/TOMS748.hpp

      f, lower_bound, upper_bound, f_at_lower_bound, f_at_upper_bound, tol,
-      max_iters);
+      ignore_filter, max_iters);
  if (max_iters >= max_iterations) {
    throw convergence_error(


I only change this throw to have extra info in it.

nilsdeppe · 2023-10-27T15:19:55Z

Okay, I moved the file. Sorry, this was a goof on my end. I don't know why I put it in the wrong directory. I then interpreted @nilsvu's comment as "Why not put it inside the header file?" which is clearly not what it asked. Sorry for the confusion!

nilsvu · 2023-10-30T16:50:16Z

I ran the tabulated EOS test 1e4 times without fail on macOS (Apple Silicon though, not GitHub's Intel chips). Not sure what caused the convergence issue on CI but I can't reproduce it so can't do anything about it :)

nilsvu · 2023-10-30T16:50:58Z

Edit: GitHub isn't surfacing this: #5578 (comment)

nilsdeppe · 2023-11-07T03:11:51Z

Rebased on latest develop since this was a bit old

nilsdeppe · 2023-11-11T23:25:47Z

I've pushed a fixup that resolves the random failures in some of the tests like Test_Tabulated3D.cpp. I also pushed a commit that adds several CAPTUREs to test_Tabulated3D.cpp. I rebased as well.

wthrowe

Looks good. Squash.

There are still some quirks that need to be ironed out before this is ready to go.

The goal is again to wrap the external library so that we are somewhat agnostic to the specific simd library we are using.

nilsdeppe · 2023-11-13T19:21:54Z

Rebased and squashed. Thanks for the reviews!

nilsdeppe · 2023-11-14T20:13:20Z

CI failures are:

2x executable build timeouts
1x Burgers Step failure

nilsdeppe requested a review from kidder October 20, 2023 13:30

wthrowe requested changes Oct 20, 2023

View reviewed changes

nilsvu requested changes Oct 20, 2023

View reviewed changes

nilsdeppe force-pushed the simd_toms748 branch from fe9d0fa to 2f1af9b Compare October 21, 2023 14:52

wthrowe requested changes Oct 22, 2023

View reviewed changes

nilsdeppe force-pushed the simd_toms748 branch from 2f1af9b to f74e1f9 Compare October 23, 2023 16:48

nilsdeppe force-pushed the simd_toms748 branch 3 times, most recently from 0f17a3a to db87fc6 Compare October 24, 2023 14:03

nilsvu requested changes Oct 24, 2023

View reviewed changes

src/Evolution/Systems/GrMhd/ValenciaDivClean/LICENSE_1_0.txt Outdated Show resolved Hide resolved

src/PointwiseFunctions/AnalyticSolutions/Xcts/Schwarzschild.cpp Show resolved Hide resolved

tests/Unit/NumericalAlgorithms/RootFinding/Test_TOMS748.cpp Show resolved Hide resolved

nilsdeppe force-pushed the simd_toms748 branch 2 times, most recently from 8735fed to eeb8ead Compare October 26, 2023 02:08

wthrowe requested changes Oct 26, 2023

View reviewed changes

src/Evolution/Systems/GrMhd/ValenciaDivClean/LICENSE_1_0.txt Outdated Show resolved Hide resolved

nilsdeppe commented Oct 26, 2023

View reviewed changes

nilsdeppe force-pushed the simd_toms748 branch from eeb8ead to a5d2e1b Compare October 27, 2023 15:13

wthrowe previously approved these changes Oct 27, 2023

View reviewed changes

nilsdeppe dismissed wthrowe’s stale review via fa8f5a3 November 7, 2023 03:11

nilsdeppe force-pushed the simd_toms748 branch from a5d2e1b to fa8f5a3 Compare November 7, 2023 03:11

wthrowe previously approved these changes Nov 8, 2023

View reviewed changes

nilsdeppe force-pushed the simd_toms748 branch from fa8f5a3 to 4174ebf Compare November 9, 2023 20:46

nilsdeppe dismissed wthrowe’s stale review via 7179c6f November 10, 2023 15:59

nilsdeppe force-pushed the simd_toms748 branch 4 times, most recently from 99b91fb to ad1f0c1 Compare November 11, 2023 23:25

nilsvu approved these changes Nov 12, 2023

View reviewed changes

wthrowe approved these changes Nov 13, 2023

View reviewed changes

nilsdeppe added 11 commits November 13, 2023 11:19

Require xsimd 11.0.1 or newer

9b83d79

Do not interface Blaze and xsimd

b4b31e6

There are still some quirks that need to be ironed out before this is ready to go.

Add missing include to UniformCylindricalEndcap

6158684

Set scale in map tests

ea05d7f

Use target_compile_options for xsimd

86472be

Add missing link dependency: spectral on root finding

71a9e1b

Add simd library wrapping xsimd

a6b8725

The goal is again to wrap the external library so that we are somewhat agnostic to the specific simd library we are using.

Allow Tensor<xsimd::batch>

328e309

Add vectorized TOMS748 root finder

b0f8da1

Make RootFinding a private dependency of CoordinateMaps

aab4bd0

Add CAPTUREs to Test_Tabulated3D.cpp

72b67da

nilsdeppe force-pushed the simd_toms748 branch from 72fb502 to 72b67da Compare November 13, 2023 19:21

nilsdeppe merged commit 0945e8c into sxs-collaboration:develop Nov 14, 2023
19 of 22 checks passed

nilsdeppe deleted the simd_toms748 branch November 14, 2023 20:13

		@@ -6,15 +6,465 @@

		#pragma once

		#include <boost/math/tools/roots.hpp>

Optimize and vectorize TOMS748 #5578

Optimize and vectorize TOMS748 #5578

Conversation

nilsdeppe commented Oct 20, 2023

Proposed changes

Upgrade instructions

Code review checklist

Further comments

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe commented Oct 21, 2023

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe commented Oct 23, 2023

nilsdeppe commented Oct 24, 2023

wthrowe commented Oct 24, 2023

nilsvu left a comment

Choose a reason for hiding this comment

nilsvu commented Oct 24, 2023

nilsdeppe commented Oct 24, 2023 • edited Loading

nilsdeppe commented Oct 26, 2023

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe commented Oct 27, 2023

nilsvu commented Oct 30, 2023

nilsvu commented Oct 30, 2023

nilsdeppe commented Nov 7, 2023

nilsdeppe commented Nov 11, 2023

wthrowe left a comment

Choose a reason for hiding this comment

nilsdeppe commented Nov 13, 2023

nilsdeppe commented Nov 14, 2023

nilsdeppe Oct 21, 2023 •

edited

Loading

nilsdeppe commented Oct 24, 2023 •

edited

Loading