Support BOUT_FOR_RAJA GPU field operators by ggeorgakoudis · Pull Request #3078 · boutproject/BOUT-dev

ggeorgakoudis · 2025-02-25T22:23:33Z

Generate BOUT_FOR_RAJA code for GPU execution of field operators

github-actions

clang-tidy made some suggestions

github-actions · 2025-02-25T22:26:42Z

include/bout/coordinates_accessor.hxx

 ///    by calling CoordinatesAccessor::clear()
 struct CoordinatesAccessor {
-  CoordinatesAccessor() = delete;
+  CoordinatesAccessor() {}


warning: constructor does not initialize these fields: data, mesh_nz [cppcoreguidelines-pro-type-member-init]

include/bout/coordinates_accessor.hxx:87:

- BoutReal* data; - int mesh_nz; ///< For converting from 3D to 2D index + BoutReal* data{}; + int mesh_nz{}; ///< For converting from 3D to 2D index

github-actions · 2025-02-25T22:26:42Z

include/bout/field_accessor.hxx

    ddt = BoutRealArray{&(f.timeDeriv()->operator()(0, 0, 0))};
  }

+  explicit FieldAccessor(const FieldType& f) : FieldAccessor(const_cast<FieldType&>(f)) {}


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

explicit FieldAccessor(const FieldType& f) : FieldAccessor(const_cast<FieldType&>(f)) {} ^

github-actions · 2025-02-25T22:26:42Z

include/bout/field_accessor.hxx

  /// so fa[i] is equivalent to fa.data[i].
  ///
  BOUT_HOST_DEVICE inline const BoutReal& operator[](int ind) const { return data[ind]; }
+  BOUT_HOST_DEVICE inline BoutReal& operator[](int ind) { return data[ind]; }


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

Suggested change

BOUT_HOST_DEVICE inline BoutReal& operator[](int ind) { return data[ind]; }

BOUT_HOST_DEVICE BoutReal& operator[](int ind) { return data[ind]; }

github-actions · 2025-02-25T22:26:42Z

include/bout/field_accessor.hxx

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


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

Suggested change

BOUT_HOST_DEVICE inline BoutReal& operator[](const Ind3D& ind) { return data[ind.ind]; }

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

github-actions · 2025-02-25T22:26:42Z

include/bout/field_accessor.hxx

+  explicit FieldPerpAccessor(const FieldPerp& f) {
+    ASSERT0(f.isAllocated());
+
+    data = BoutRealArray{const_cast<BoutReal*>(&f(0, 0, 0))};


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

data = BoutRealArray{const_cast<BoutReal*>(&f(0, 0, 0))}; ^

Why do we need to take a const field?
If we would take a FieldPerp, we would not need the const_cast.

Do we need a ConstFieldPerpAccessor?

github-actions · 2025-02-25T22:26:43Z

include/bout/mesh.hxx

    return {(indPerp.ind - jz) * LocalNy + LocalNz * jy + jz, LocalNy, LocalNz};
  }

+  BOUT_HOST_DEVICE int flatIndPerpto3D(const int& flatIndPerp, const int nz, int jy = 0) const {


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

include/bout/mesh.hxx:40:

- class Mesh; + #include "bout/build_config.hxx" + class Mesh;

github-actions · 2025-02-25T22:26:43Z

include/bout/mesh.hxx

  }

+  BOUT_HOST_DEVICE int flatIndPerpto3D(const int& flatIndPerp, const int nz, int jy = 0) const {
+    int jz = flatIndPerp % nz;


warning: variable 'jz' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int jz = flatIndPerp % nz;

int const jz = flatIndPerp % nz;

warning: variable 'jz' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int jz = flatIndPerp % nz;

const int jz = flatIndPerp % nz;

We generally put const in front ...

github-actions · 2025-02-25T22:26:43Z

src/mesh/coordinates_accessor.cxx

  // Uses the symbol to look up the corresponding Offset
-#define COPY_STRIPE1(symbol) \
-  data[stripe_size * ind.ind + static_cast<int>(Offset::symbol)] = coords->symbol[ind];
+#define COPY_STRIPE1(symbol)        \


warning: function-like macro 'COPY_STRIPE1' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define COPY_STRIPE1(symbol) \ ^

github-actions · 2025-02-25T22:26:43Z

src/mesh/coordinates_accessor.cxx

-        coords->Bxy.ydown()[ind];
+    if (coords->Bxy.isAllocated()) {
+      data[stripe_size * ind.ind + static_cast<int>(Offset::B)] = coords->Bxy[ind];
+      if (coords->Bxy.yup().isAllocated())


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (coords->Bxy.yup().isAllocated())

if (coords->Bxy.yup().isAllocated()) {

src/mesh/coordinates_accessor.cxx:61:

- coords->Bxy.yup()[ind]; + coords->Bxy.yup()[ind]; + }

please always use braces for if statements!

github-actions · 2025-02-25T22:26:43Z

src/mesh/coordinates_accessor.cxx

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


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (coords->Bxy.ydown().isAllocated())

if (coords->Bxy.ydown().isAllocated()) {

src/mesh/coordinates_accessor.cxx:64:

- coords->Bxy.ydown()[ind]; + coords->Bxy.ydown()[ind]; + }

dschwoerer

This PR roughly doubles the lines of gen_fieldops.jinja - making it very hard to read.

I think we should either manage to move more to the python file, or split it into two files, which of course makes it more tricky to keep in sync, but might be the lesser of two evils ...

Also look at the clang-tidy comments.

dschwoerer · 2025-02-26T08:43:29Z

include/bout/field_accessor.hxx

+  explicit FieldPerpAccessor(const FieldPerp& f) {
+    ASSERT0(f.isAllocated());
+
+    data = BoutRealArray{const_cast<BoutReal*>(&f(0, 0, 0))};


Why do we need to take a const field?
If we would take a FieldPerp, we would not need the const_cast.

Do we need a ConstFieldPerpAccessor?

dschwoerer · 2025-02-26T08:43:49Z

include/bout/mesh.hxx

  }

+  BOUT_HOST_DEVICE int flatIndPerpto3D(const int& flatIndPerp, const int nz, int jy = 0) const {
+    int jz = flatIndPerp % nz;


warning: variable 'jz' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int jz = flatIndPerp % nz;

const int jz = flatIndPerp % nz;

We generally put const in front ...

dschwoerer · 2025-02-26T08:48:17Z

CMakeLists.txt

+  if (BOUT_ENABLE_RAJA)
+    set(GEN_LOOP_EXEC "raja")
+  elseif (BOUT_ENABLE_OPENMP)
+    set(GEN_LOOP_EXEC "openmp")
+  else()
+    set(GEN_LOOP_EXEC "serial")
+  endif()
  add_custom_command( OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/src/field/generated_fieldops.cxx
-    COMMAND ${Python3_EXECUTABLE} gen_fieldops.py --filename generated_fieldops.cxx.tmp
+    COMMAND ${Python3_EXECUTABLE} gen_fieldops.py --loop-exec ${GEN_LOOP_EXEC} --filename generated_fieldops.cxx.tmp


I think it is a bad idea to generate different code, depending on configure option, as we track the generated code, and we want to do this, so users don't have to generate the code.

If we do not want to generate code full of #if's, we could always generate all 3 versions, and track all files in git. Then we can just #if guard the correct C++ code, and compile 2 empty files.

dschwoerer · 2025-02-26T08:53:18Z

src/field/gen_fieldops.jinja

+    {% if lhs.field_type == "FieldPerp" %}
+      auto {{lhs.name}}_acc = FieldPerpAccessor{ {{lhs.name}} };
+    {% elif lhs.field_type == "BoutReal" %}
+    {% else %}
+      auto {{lhs.name}}_acc = FieldAccessor({{lhs.name}});
+    {% endif %}


Suggested change

{% if lhs.field_type == "FieldPerp" %}

auto {{lhs.name}}_acc = FieldPerpAccessor{ {{lhs.name}} };

{% elif lhs.field_type == "BoutReal" %}

{% else %}

auto {{lhs.name}}_acc = FieldAccessor({{lhs.name}});

{% endif %}

{% if lhs.field_type != "BoutReal" %}

auto {{lhs.name}}_acc = {{ lhs.accessor }}({{lhs.name}});

{% endif %}

Although I think this is nicer, as it is shorter, more clear and no copying is required:

Suggested change

{% if lhs.field_type == "FieldPerp" %}

auto {{lhs.name}}_acc = FieldPerpAccessor{ {{lhs.name}} };

{% elif lhs.field_type == "BoutReal" %}

{% else %}

auto {{lhs.name}}_acc = FieldAccessor({{lhs.name}});

{% endif %}

{% if lhs.field_type != "BoutReal" %}

{{ lhs.accessor }} {{lhs.name}}_acc({{lhs.name}});

{% endif %}

dschwoerer · 2025-02-26T08:54:02Z

src/field/gen_fieldops.jinja


  {% if (out == "Field3D") and ((lhs == "Field2D") or (rhs =="Field2D")) %}
+    {% if (region_loop == "BOUT_FOR_RAJA") %}
+    int mesh_nz = {{lhs.name if lhs.field_type != "BoutReal" else rhs.name}}_acc.mesh_nz;


Suggested change

int mesh_nz = {{lhs.name if lhs.field_type != "BoutReal" else rhs.name}}_acc.mesh_nz;

const int mesh_nz = {{lhs.name if lhs.field_type != "BoutReal" else rhs.name}}_acc.mesh_nz;

dschwoerer · 2025-02-26T11:01:51Z

src/mesh/coordinates_accessor.cxx

-        coords->Bxy.ydown()[ind];
+    if (coords->Bxy.isAllocated()) {
+      data[stripe_size * ind.ind + static_cast<int>(Offset::B)] = coords->Bxy[ind];
+      if (coords->Bxy.yup().isAllocated())


please always use braces for if statements!

dschwoerer · 2025-02-26T11:11:53Z

CMakeLists.txt

+  elseif (BOUT_ENABLE_OPENMP)
+    set(GEN_LOOP_EXEC "openmp")
+  else()
+    set(GEN_LOOP_EXEC "serial")
+  endif()


Note that this changes the default. We used to always generate BOUT_FOR unless the user explicitly requested BOUT_FOR_SERIAL. I am fairly confident we do not want to change the default.

ggeorgakoudis and others added 2 commits February 25, 2025 14:00

Support BOUT_FOR_RAJA GPU field operators

dc932cc

Apply black changes

f0c4b22

ggeorgakoudis requested review from ZedThree, bendudson and dschwoerer February 25, 2025 22:23

github-actions bot reviewed Feb 25, 2025

View reviewed changes

dschwoerer requested changes Feb 26, 2025

View reviewed changes

ggeorgakoudis self-assigned this Mar 10, 2025

	BOUT_HOST_DEVICE inline BoutReal& operator[](int ind) { return data[ind]; }
	BOUT_HOST_DEVICE BoutReal& operator[](int ind) { return data[ind]; }

	BOUT_HOST_DEVICE inline BoutReal& operator[](const Ind3D& ind) { return data[ind.ind]; }
	BOUT_HOST_DEVICE BoutReal& operator[](const Ind3D& ind) { return data[ind.ind]; }

	if (coords->Bxy.yup().isAllocated())
	if (coords->Bxy.yup().isAllocated()) {

	if (coords->Bxy.ydown().isAllocated())
	if (coords->Bxy.ydown().isAllocated()) {

	int mesh_nz = {{lhs.name if lhs.field_type != "BoutReal" else rhs.name}}_acc.mesh_nz;
	const int mesh_nz = {{lhs.name if lhs.field_type != "BoutReal" else rhs.name}}_acc.mesh_nz;

Conversation

ggeorgakoudis commented Feb 25, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

dschwoerer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants