Automatically select apropriate parallel slice #3094

dschwoerer · 2025-03-18T15:31:18Z

This allows to replace f.yup()[i.yp()] with f[i.yp()].

That should allow to simplify some code, and will make more code fci-correct.

Useful for Field3D with parallel slices, so we can write `f[i.yp()]` instead of `f.yup()[i.yp()]`

This is needed wo we can both write `f[i.yp()]` while allowing old code to call `f.yup()[i.yp()]` as well.

github-actions

clang-tidy made some suggestions

include/bout/field3d.hxx

src/field/generated_fieldops.cxx

Interpolation methods might need that, but in general it is unlike to be of use.

The tests are probably not meant to work with parallel slices.

ZedThree · 2025-06-09T14:01:41Z

This is really neat, but my major worry is having any conditionals in operator[] will completely kill vectorisation. What if adding a yoffset to an Ind made a new type, and we add a new overload of operator[]? Then at least most indexing wouldn't have these conditionals in.

dschwoerer · 2025-06-12T06:49:49Z

Adding a new type would require a lot of new code. Do you have some benchmarks that suggest that this a significant overhead?

ZedThree · 2025-06-12T09:15:29Z

We did a lot of benchmarking when we first made Region and Ind, and vectorisation was quite temperamental and made a lot of difference to performance. operator[] is going to be at the bottom of a lot of tight loops, so we do have to be careful here

dschwoerer · 2025-06-29T19:37:48Z

Do you have a suggestion for a performance test case?
I do agree we should make sure it does not cause (significant) performance issues ...

dschwoerer · 2025-07-09T11:21:34Z

I just realised with this and Field3DParallel

void floor_all(Field3D &f, BoutReal val) {
  alloc_all(f);
  BOUT_FOR(i, f.getRegion("RGN_ALL")) {
    f[i] = floor(f,val);
    f.yup()[i] = floor(f,val);
    f.ydown()[i] = floor(f,val);
  }
}

can be simplified to

void floor(Field3DParallel &f, BoutReal val) {
  BOUT_FOR(i, f.getRegion("RGN_ALL")) {
    f[i] = floor(f,val);
  }
}

if getRegion contains also shifted indices.
That can then just be

template <class F>
void floor(F &f, BoutReal val) {
  BOUT_FOR(i, f.getRegion("RGN_ALL")) {
    f[i] = floor(f,val);
  }
}

which we probably have already :-)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

clang-tidy made some suggestions

include/bout/field3d.hxx

github-actions · 2025-07-21T08:24:11Z

include/bout/region.hxx

  }

-  inline SpecificInd xp(int dx = 1) const { return {ind + (dx * ny * nz), ny, nz}; }
+  inline SpecificInd xp(int dx = 1) const {


warning: function 'xp' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]

Suggested change

inline SpecificInd xp(int dx = 1) const {

SpecificInd xp(int dx = 1) const {

github-actions · 2025-07-21T08:24:11Z

include/bout/region.hxx

-    return {ind + (dy * nz), ny, nz};
+    return {ind + (dy * nz), ny, nz, yoffset + dy};
  }
+  inline SpecificInd yp_no_parallel_shift(int dy = 1) const {


warning: function 'yp_no_parallel_shift' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]

Suggested change

inline SpecificInd yp_no_parallel_shift(int dy = 1) const {

SpecificInd yp_no_parallel_shift(int dy = 1) const {

github-actions · 2025-07-21T08:24:11Z

src/mesh/mesh.cxx

+  // Add region for parallel slices
+  for (int i = 1; i <= ystart; ++i) {
+    for (const int offset : {-i, i}) {
+      const auto region = fmt::format("RGN_YPAR_{:+d}", offset);


warning: no header providing "fmt::format" is directly included [misc-include-cleaner]

src/mesh/mesh.cxx:12:

- #include "impls/bout/boutmesh.hxx" + #include "fmt/core.h" + #include "impls/bout/boutmesh.hxx"

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-21T08:29:55Z

include/bout/field3d.hxx

 #include "bout/assert.hxx"
 #include "bout/bout_types.hxx"
+#include "bout/boutexception.hxx"
 #include "bout/field.hxx"


warning: included header boutexception.hxx is not used directly [misc-include-cleaner]

Suggested change

#include "bout/field.hxx"

#include "bout/field.hxx"

include/bout/field3d.hxx

github-actions · 2025-07-21T08:29:57Z

include/bout/field3d.hxx

+      ASSERT2(d.yoffset == yoffset);
+    }
+  }
+  return data[d.ind];


warning: expected member name or ';' after declaration specifiers [clang-diagnostic-error]

return data[d.ind]; ^

github-actions

clang-tidy made some suggestions

include/bout/field3d.hxx

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-21T11:16:32Z

include/bout/field2d.hxx

          DirectionTypes directions_in = {YDirectionType::Standard,
                                          ZDirectionType::Average},
-          std::optional<size_t> region = {});
+          std::optional<size_t> region = {}, int yoffset = 0);


warning: no header providing "size_t" is directly included [misc-include-cleaner]

include/bout/field2d.hxx:26:

- class Field2D; + #include <cstddef> + class Field2D;

github-actions · 2025-07-21T11:16:32Z

include/bout/field2d.hxx

          DirectionTypes directions_in = {YDirectionType::Standard,
                                          ZDirectionType::Average},
-          std::optional<size_t> region = {});
+          std::optional<size_t> region = {}, int yoffset = 0);


warning: no header providing "std::optional" is directly included [misc-include-cleaner]

include/bout/field2d.hxx:26:

- class Field2D; + #include <optional> + class Field2D;

github-actions · 2025-07-21T11:16:32Z

include/bout/field3d.hxx

          DirectionTypes directions_in = {YDirectionType::Standard,
                                          ZDirectionType::Standard},
-          std::optional<size_t> regionID = {});
+          std::optional<size_t> regionID = {}, int yoffset = 0);


warning: no header providing "size_t" is directly included [misc-include-cleaner]

include/bout/field3d.hxx:22:

- class Field3D; + #include <cstddef> + class Field3D;

github-actions · 2025-07-21T11:16:32Z

include/bout/field3d.hxx

+    return data[d.ind];
+  }
+  const BoutReal& operator[](const Ind3D& d) const {
+    return (*const_cast<Field3D*>(this))[d];


warning: do not use const_cast to remove const qualifier [cppcoreguidelines-pro-type-const-cast]

return (*const_cast<Field3D*>(this))[d]; ^

github-actions · 2025-07-21T11:16:32Z

include/bout/field3d.hxx

  Options* track(const BoutReal& change, std::string operation);
+
+public:
+  int yoffset{0};


warning: member variable 'yoffset' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

int yoffset{0}; ^

github-actions · 2025-07-21T11:16:33Z

src/field/fieldperp.cxx


 FieldPerp::FieldPerp(Mesh* localmesh, CELL_LOC location_in, int yindex_in,
-                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID))
+                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID),


warning: no header providing "UNUSED" is directly included [misc-include-cleaner]

src/field/fieldperp.cxx:25:

- #include <bout/boutcomm.hxx> + #include "bout/unused.hxx" + #include <bout/boutcomm.hxx>

github-actions · 2025-07-21T11:16:33Z

src/field/fieldperp.cxx


 FieldPerp::FieldPerp(Mesh* localmesh, CELL_LOC location_in, int yindex_in,
-                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID))
+                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID),


warning: no header providing "size_t" is directly included [misc-include-cleaner]

src/field/fieldperp.cxx:29:

+ #include <cstddef>

github-actions · 2025-07-21T11:16:33Z

src/field/fieldperp.cxx


 FieldPerp::FieldPerp(Mesh* localmesh, CELL_LOC location_in, int yindex_in,
-                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID))
+                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID),


warning: no header providing "std::optional" is directly included [misc-include-cleaner]

src/field/fieldperp.cxx:29:

+ #include <optional>

github-actions · 2025-07-21T11:16:34Z

src/mesh/coordinates_accessor.cxx

+    {
+      auto indc = ind;
+      indc.yoffset = 1;
+      data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =

data[(stripe_size * ind.ind) + static_cast<int>(Offset::Byup)] = coords->Bxy.yup()[indc];

github-actions · 2025-07-21T11:16:34Z

src/mesh/coordinates_accessor.cxx

+      indc.yoffset = 1;
+      data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =
+          coords->Bxy.yup()[indc];
+      indc.yoffset = -1;


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

indc.yoffset = -1;

data[(stripe_size * ind.ind) + static_cast<int>(Offset::Bydown)] =

github-actions

clang-tidy made some suggestions

github-actions · 2025-07-21T18:34:15Z

src/field/fieldperp.cxx


 FieldPerp::FieldPerp(Mesh* localmesh, CELL_LOC location_in, int yindex_in,
-                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID))
+                     DirectionTypes directions, std::optional<size_t> UNUSED(regionID),


warning: no header providing "DirectionTypes" is directly included [misc-include-cleaner]

DirectionTypes directions, std::optional<size_t> UNUSED(regionID), ^

github-actions · 2025-07-21T18:34:15Z

src/mesh/coordinates_accessor.cxx

+    {
+      auto indc = ind;
+      indc.yoffset = 1;
+      data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =

data[(stripe_size * ind.ind) + static_cast<int>(Offset::Byup)] =

github-actions · 2025-07-21T18:34:16Z

src/mesh/coordinates_accessor.cxx

+      data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =
+          coords->Bxy.yup()[indc];
+      indc.yoffset = -1;
+      data[stripe_size * ind.ind + static_cast<int>(Offset::Bydown)] =


warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change

data[stripe_size * ind.ind + static_cast<int>(Offset::Bydown)] =

data[(stripe_size * ind.ind) + static_cast<int>(Offset::Bydown)] =

tomc271 · 2025-08-20T13:30:02Z

Profiled with hyperfine:
(Minimum time from 50 runs)

Unit tests in Release with CHECKS=0:

Test	Min time (s)	% of next
next	1.03	100%
yup-ydown-auto	1.05	102%
master	1.10	107%

Unit tests in Debug with CHECKS=2:

Test	Min time (s)	% of next
next	5.40	100%
yup-ydown-auto	5.53	102%
master	5.68	105%

test-drift-instability integration test in Release with CHECKS=0:

Test	Min time (s)	% of next
next	1.26	100%
yup-ydown-auto	1.36	108%
master	1.19	95%

test-drift-instability integration test in Debug with CHECKS=2:

Test	Min time (s)	% of next
next	11.08	100%
yup-ydown-auto	12.88	116%
master	10.81	98%

dschwoerer · 2025-08-21T07:39:24Z

Amazing, thanks @tomc271! So it seems there is quite some overhead (8%) without checks. Looking at the absolute numbers for CHECK=2, I would argue that the 16% is perfectly fine.

I am slightly worried about the 8% - that is probably not great.
So probably worth to hold of the merging, and see whether we/I can come up with a better implementation. Templates could be used, then some of the checks could be done at compile time, but that requires more time to think about it ...

It would make the code however more readable, which also a benefit for non-FCI users. Maybe we could make make a compile time switch to enable parallel slices, so that users that do not use it, can keep it disabled. However, then we either cannot start using this feature and clean up the code, or we are kind of forced to stay with it, even if we cannot come up with a faster implementation ...

daviddickinson · 2025-08-21T07:56:15Z

include/bout/field3d.hxx


-  BoutReal& operator[](const Ind3D& d) { return data[d.ind]; }
-  const BoutReal& operator[](const Ind3D& d) const { return data[d.ind]; }
+  BoutReal& operator[](const Ind3D& d) {


It has been a long time since I've looked at this code but could this be return ynext(d.yoffset - yoffset)[d]? One could then still include some of the consistency checking but it would move some of the logic into CHECK regions that disappear in production runs?

return ynext(d.yoffset - yoffset)[d] would always call ynext - even if this already the right field, and thus would fail.

The issue is that both the main field, which contains parallel fields, as well as the parallel fields, are of type Field3D.

We could (together with PR #3149) only enable this for Field3DParallel. There might be subtle bugs, where it depends on the type (Field3DParallel/Field3D) on whether it uses the correct indexing. But we could add those with higher CHECK level, so that should be fine.

daviddickinson · 2025-08-21T07:57:51Z

include/bout/field3d.hxx

+          return ynext(d.yoffset)[d];
+        }
+#if CHECK >= 2
+        if (isFci()) { // We probably should assert here that this is field aligned


Could this be if ( ! hasParallelSlices() && isFci()) and placed before the return line so the return doesn't need to be guarded?

I think it would still need to be guarded - because for FA there are no parallel slices, and thus we need to use the return at the end of the function.

dschwoerer and others added 5 commits March 18, 2025 15:32

Track yoffset in SpecificInd

d26b825

Useful for Field3D with parallel slices, so we can write `f[i.yp()]` instead of `f.yup()[i.yp()]`

Track yoffset in Field3D

b27d19d

This is needed wo we can both write `f[i.yp()]` while allowing old code to call `f.yup()[i.yp()]` as well.

Add some checks to ensure the parallel offset is correct

aaf102d

Automatically select the appropriate parallel slice

feec777

Apply clang-format changes

49bae21

github-actions bot reviewed Mar 18, 2025

View reviewed changes

include/bout/field3d.hxx Outdated Show resolved Hide resolved

include/bout/field3d.hxx Show resolved Hide resolved

include/bout/field3d.hxx Show resolved Hide resolved

include/bout/field3d.hxx Show resolved Hide resolved

src/field/generated_fieldops.cxx Show resolved Hide resolved

dschwoerer and others added 7 commits March 19, 2025 10:52

Fix: preserve yoffset with other operations

247c2fb

Add a version of .i.yp() that does not shift the parallel field

6e834db

Interpolation methods might need that, but in general it is unlike to be of use.

Add y-shifted regions by default

242b9b2

Fix hermite_spline_z for auto-shift

53c9e31

Avoid warning with 3D metrics

fe888ae

Apply clang-format changes

76dc241

Ajust tests for auto parallel slices

eeb7c14

The tests are probably not meant to work with parallel slices.

dschwoerer and others added 4 commits July 21, 2025 10:12

Merge branch 'fci-auto-with-debug-higher-order' into yup-ydown-auto

980f00b

Apply suggestions from code review

fa1bcb4

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Apply clang-format changes

79e2579

Add header explicitly for clang-tidy

b714f7a

github-actions bot reviewed Jul 21, 2025

View reviewed changes

dschwoerer and others added 2 commits July 21, 2025 10:24

Fixup suggestions from code-review

742e6d0

Apply clang-format changes

58caf56

github-actions bot reviewed Jul 21, 2025

View reviewed changes

dschwoerer and others added 2 commits July 21, 2025 10:51

Remove extra } from code review

6f8a4c9

Apply clang-format changes

159c26e

github-actions bot reviewed Jul 21, 2025

View reviewed changes

include/bout/field3d.hxx Show resolved Hide resolved

Add yoffset to emptyFrom

9496211

dschwoerer and others added 3 commits July 21, 2025 12:53

Use correct yoffset for accessing parallel fields

9b052eb

Fix mock fields to have realistic yoffset

085948c

Apply clang-format changes

288eb77

github-actions bot reviewed Jul 21, 2025

View reviewed changes

Disable hypre3d test

f3fd242

github-actions bot reviewed Jul 21, 2025

View reviewed changes

daviddickinson reviewed Aug 21, 2025

View reviewed changes

	inline SpecificInd xp(int dx = 1) const {
	SpecificInd xp(int dx = 1) const {

	inline SpecificInd yp_no_parallel_shift(int dy = 1) const {
	SpecificInd yp_no_parallel_shift(int dy = 1) const {

	data[stripe_size * ind.ind + static_cast<int>(Offset::Byup)] =
	data[(stripe_size * ind.ind) + static_cast<int>(Offset::Byup)] = coords->Bxy.yup()[indc];

	indc.yoffset = -1;
	data[(stripe_size * ind.ind) + static_cast<int>(Offset::Bydown)] =

Automatically select apropriate parallel slice #3094

Are you sure you want to change the base?

Automatically select apropriate parallel slice #3094

Uh oh!

Conversation

dschwoerer commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZedThree commented Jun 9, 2025

Uh oh!

dschwoerer commented Jun 12, 2025

Uh oh!

ZedThree commented Jun 12, 2025

Uh oh!

dschwoerer commented Jun 29, 2025

Uh oh!

dschwoerer commented Jul 9, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 21, 2025

dschwoerer commented Mar 18, 2025 •

edited

Loading