-
Notifications
You must be signed in to change notification settings - Fork 104
Support BOUT_FOR_RAJA GPU field operators #3078
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
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -386,8 +386,15 @@ if (BOUT_GENERATE_FIELDOPS) | |
| if (NOT ClangFormat_FOUND) | ||
| message(FATAL_ERROR "clang-format not found, but you have requested to generate code!") | ||
| endif() | ||
| 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 | ||
|
Comment on lines
+389
to
+397
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| COMMAND ${ClangFormat_BIN} generated_fieldops.cxx.tmp -i | ||
| COMMAND ${CMAKE_COMMAND} -E rename generated_fieldops.cxx.tmp generated_fieldops.cxx | ||
| DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/src/field/gen_fieldops.jinja ${CMAKE_CURRENT_SOURCE_DIR}/src/field/gen_fieldops.py | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ | |
| /// -> If Coordinates data is changed, the cache should be cleared | ||
| /// by calling CoordinatesAccessor::clear() | ||
| struct CoordinatesAccessor { | ||
| CoordinatesAccessor() = delete; | ||
| CoordinatesAccessor() {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| /// Constructor from Coordinates | ||
| /// Copies data from coords, doesn't modify it | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,10 +57,17 @@ struct FieldAccessor { | |||||||||
| /// Constructor from Field3D | ||||||||||
| /// | ||||||||||
| /// @param[in] f The field to access. Must already be allocated | ||||||||||
| explicit FieldAccessor(FieldType& f) : coords(f.getCoordinates()) { | ||||||||||
| explicit FieldAccessor(FieldType& f) { | ||||||||||
| ASSERT0(f.getLocation() == location); | ||||||||||
| ASSERT0(f.isAllocated()); | ||||||||||
|
|
||||||||||
| if (auto* Coords = f.getCoordinates()) { | ||||||||||
| coords = CoordinatesAccessor{Coords}; | ||||||||||
| } | ||||||||||
| else { | ||||||||||
| coords = CoordinatesAccessor{}; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| data = BoutRealArray{&f(0, 0, 0)}; | ||||||||||
|
|
||||||||||
| // Field size | ||||||||||
|
|
@@ -81,15 +88,19 @@ struct FieldAccessor { | |||||||||
| ddt = BoutRealArray{&(f.timeDeriv()->operator()(0, 0, 0))}; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| explicit FieldAccessor(const FieldType& f) : FieldAccessor(const_cast<FieldType&>(f)) {} | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)) {}
^ |
||||||||||
|
|
||||||||||
| /// Provide shorthand for access to field data. | ||||||||||
| /// Does not convert between 3D and 2D indices, | ||||||||||
| /// 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]; } | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'operator[]' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
Suggested change
|
||||||||||
|
|
||||||||||
| 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]; } | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'operator[]' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
Suggested change
|
||||||||||
|
|
||||||||||
| // Pointers to the field data arrays | ||||||||||
| // These are wrapped in BoutRealArray types so they can be indexed with Ind3D or int | ||||||||||
|
|
@@ -115,6 +126,9 @@ struct FieldAccessor { | |||||||||
| template <CELL_LOC location = CELL_CENTRE> | ||||||||||
| using Field2DAccessor = FieldAccessor<location, Field2D>; | ||||||||||
|
|
||||||||||
| template <CELL_LOC location = CELL_CENTRE> | ||||||||||
| using Field3DAccessor = FieldAccessor<location, Field3D>; | ||||||||||
|
|
||||||||||
| /// Syntactic sugar for time derivative of a field | ||||||||||
| /// | ||||||||||
| /// Usage: | ||||||||||
|
|
@@ -130,4 +144,28 @@ BOUT_HOST_DEVICE inline BoutRealArray& ddt(const FieldAccessor<location, FieldTy | |||||||||
| return const_cast<BoutRealArray&>(fa.ddt); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct FieldPerpAccessor { | ||||||||||
| FieldPerpAccessor() = delete; | ||||||||||
|
|
||||||||||
| int nx, nz; | ||||||||||
| int yindex; | ||||||||||
| BoutReal* data; | ||||||||||
|
|
||||||||||
| explicit FieldPerpAccessor(const FieldPerp& f) { | ||||||||||
| ASSERT0(f.isAllocated()); | ||||||||||
|
|
||||||||||
| data = BoutRealArray{const_cast<BoutReal*>(&f(0, 0, 0))}; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: do not use const_cast to remove const qualifier [cppcoreguidelines-pro-type-const-cast] data = BoutRealArray{const_cast<BoutReal*>(&f(0, 0, 0))};
^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to take a const field? Do we need a |
||||||||||
|
|
||||||||||
| // Field size | ||||||||||
| nx = f.getNx(); | ||||||||||
| nz = f.getNz(); | ||||||||||
|
|
||||||||||
| yindex = f.getIndex(); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| BOUT_HOST_DEVICE int getIndex() const { return yindex; } | ||||||||||
| BOUT_HOST_DEVICE inline const BoutReal& operator[](int ind) const { return data[ind]; } | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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]; } | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'operator[]' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: method 'operator[]' can be made const [readability-make-member-function-const]
Suggested change
|
||||||||||
| }; | ||||||||||
|
|
||||||||||
| #endif | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -762,6 +762,11 @@ public: | |||||||||
| 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 { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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; |
||||||||||
| int jz = flatIndPerp % nz; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: variable 'jz' of type 'int' can be declared 'const' [misc-const-correctness]
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We generally put const in front ... |
||||||||||
| return (flatIndPerp - jz) * LocalNy + LocalNz * jy + jz; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// Converts an Ind3D to an Ind2D representing a 2D index using a lookup -- to be used with care | ||||||||||
| Ind2D map3Dto2D(const Ind3D& ind3D) { | ||||||||||
| return {indexLookup3Dto2D[ind3D.ind], LocalNy, 1}; | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this changes the default. We used to always generate
BOUT_FORunless the user explicitly requestedBOUT_FOR_SERIAL. I am fairly confident we do not want to change the default.