Optimize FixConservatives #5633

nilsdeppe · 2023-11-20T02:08:13Z

Proposed changes

note: I've left fixup commits that I will squash with the goal of making the PR easier to review.

Optimize dot_product for magnitude computation
SIMD FixConservatives
Optimize FixConservatives
Simplify FixConservatives if there's no magnetic field

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 · 2023-12-01T19:38:14Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

+    if (const auto tilde_d_mask =
+            rest_mass_density_times_lorentz_factor <
+            rest_mass_density_times_lorentz_factor_cutoff_;
+        simd::any(tilde_d_mask)) {
      needed_fixing = true;


I believe this block should be masking modifications with tilde_d_mask.

I think this is done mostly correctly in " fixup: FixConservatives simd " (I'll squash this into the other commit where it is wrong). However, the rest_mass_density_times_lorentz_factor was not set correctly with a mask, like you said. I'm doing that in a fixup. Would you mind just double checking in the final code to make sure?

This looks good now.

wthrowe · 2023-12-01T19:43:47Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

        b_tilde_squared * one_over_sqrt_det_g * one_over_d_tilde;
+    const SimdType s_dot_tilde_b_tilde = get(tilde_s_dot_tilde_b)[s];


It seems like s_tilde_dot_b_tilde matches the other names better.

wthrowe · 2023-12-01T19:57:37Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

+  static std::array<T, 4> nonzero_bound(
+      const T& b_squared_over_d, const T& tau_over_d,
+      const T& normalized_s_dot_b, const T& lower_bound,
+      const T& sqr_normalized_s_dot_b_times_b_squared_over_d) {
    return std::array{
        -0.5 * b_squared_over_d *
            (1.0 +
             square(normalized_s_dot_b) * tau_over_d * (tau_over_d + 2.0)),


This is probably better? Unless there are significant speed differences in operations that I don't know about.

-0.5 * (b_squared_over_d + sqr_normalized_s_dot_b_times_b_squared_over_d * tau_over_d * (tau_over_d + 2.0))

wthrowe · 2023-12-01T20:06:23Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

+  const bool non_zero_mag =
+      magnetic_field_treatment_ == hydro::MagneticFieldTreatment::AssumeNonZero
+          ? true
+          : (magnetic_field_treatment_ !=
+             hydro::MagneticFieldTreatment::AssumeZero);


Equivalent: const bool non_zero_mag = magnetic_field_treatment_ != hydro::MagneticFieldTreatment::AssumeZero (or == AssumeNonZero since you overwrite it in the third case).

wthrowe · 2023-12-01T20:13:53Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

+            return square(1.0 + local_lorentz_factor_minus_one) *
+                   local_lorentz_factor_minus_one *
+                   (2.0 + local_lorentz_factor_minus_one) /
+                   square(1.0 + local_lorentz_factor_minus_one) *
+                   square(d_tilde);


The square(1.0 + local_lorentz_factor_minus_one) factors cancel.

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

nilsdeppe

Thanks for the reviews and sorry for the delay! I pushed a couple fixups. I'll squash all the SIMD stuff once you're happy :)

nilsdeppe · 2024-01-16T15:40:06Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

+    if (const auto tilde_d_mask =
+            rest_mass_density_times_lorentz_factor <
+            rest_mass_density_times_lorentz_factor_cutoff_;
+        simd::any(tilde_d_mask)) {
      needed_fixing = true;


I think this is done mostly correctly in " fixup: FixConservatives simd " (I'll squash this into the other commit where it is wrong). However, the rest_mass_density_times_lorentz_factor was not set correctly with a mask, like you said. I'm doing that in a fixup. Would you mind just double checking in the final code to make sure?

wthrowe

Looks good.

wthrowe · 2024-01-17T20:10:41Z

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp

+    if (const auto tilde_d_mask =
+            rest_mass_density_times_lorentz_factor <
+            rest_mass_density_times_lorentz_factor_cutoff_;
+        simd::any(tilde_d_mask)) {
      needed_fixing = true;


This looks good now.

nilsdeppe · 2024-01-18T21:58:28Z

This is now rebased and squashed :) Thanks for the reviews!

nilsdeppe requested a review from kidder November 20, 2023 02:08

nilsdeppe requested a review from wthrowe December 1, 2023 02:08

wthrowe requested changes Dec 1, 2023

View reviewed changes

kidder reviewed Dec 11, 2023

View reviewed changes

src/Evolution/Systems/GrMhd/ValenciaDivClean/FixConservatives.cpp Outdated Show resolved Hide resolved

nilsdeppe force-pushed the optimize_fix_conservatives branch from 25015c5 to 12e21cb Compare January 16, 2024 15:51

nilsdeppe commented Jan 16, 2024

View reviewed changes

nilsdeppe force-pushed the optimize_fix_conservatives branch from 12e21cb to aed9250 Compare January 16, 2024 23:10

wthrowe previously approved these changes Jan 17, 2024

View reviewed changes

nilsdeppe dismissed wthrowe’s stale review via 0caab91 January 17, 2024 22:39

nilsdeppe force-pushed the optimize_fix_conservatives branch 2 times, most recently from 0caab91 to 269b890 Compare January 18, 2024 15:03

nilsdeppe added 6 commits January 18, 2024 09:21

Optimize dot_product for magnitude squared

06a1c8d

Reduce divisions in FixConservatives

122fd8d

Rewrite FixConservatives to use SIMD explicitly

7823ae0

FixConservatives reduce FLOPs

4f6ed79

Add MagneticFieldTreatment enum

061b2fc

FixConservatives ignore magnetic field when zero

b3bdfe6

nilsdeppe force-pushed the optimize_fix_conservatives branch from 269b890 to b3bdfe6 Compare January 18, 2024 17:21

wthrowe approved these changes Jan 18, 2024

View reviewed changes

kidder merged commit 22cad5d into sxs-collaboration:develop Jan 19, 2024
22 checks passed

nilsdeppe deleted the optimize_fix_conservatives branch April 11, 2024 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize FixConservatives #5633

Optimize FixConservatives #5633

nilsdeppe commented Nov 20, 2023 •

edited

Loading

wthrowe Dec 1, 2023

nilsdeppe Jan 16, 2024

wthrowe Jan 17, 2024

wthrowe Dec 1, 2023

wthrowe Dec 1, 2023

wthrowe Dec 1, 2023

wthrowe Dec 1, 2023

nilsdeppe left a comment

nilsdeppe Jan 16, 2024

wthrowe left a comment

wthrowe Jan 17, 2024

nilsdeppe commented Jan 18, 2024

		b_tilde_squared * one_over_sqrt_det_g * one_over_d_tilde;
		const SimdType s_dot_tilde_b_tilde = get(tilde_s_dot_tilde_b)[s];

Optimize FixConservatives #5633

Optimize FixConservatives #5633

Conversation

nilsdeppe commented Nov 20, 2023 • edited Loading

Proposed changes

Upgrade instructions

Code review checklist

Further comments

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe commented Jan 18, 2024

nilsdeppe commented Nov 20, 2023 •

edited

Loading