WIP: Lazy stencil expressions for kernel fusion - #3401
Conversation
Return BinaryExpr types, perform derivatives in Z using CoordinateAccessor to get dz. Appear to inline in Hasegawa-Wakatani example.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| int nz{0}; | ||
|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, |
There was a problem hiding this comment.
warning: no header providing "BOUT_FORCEINLINE" is directly included [misc-include-cleaner]
BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs,
^|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
| inline bout::stencil::DDZExprC2 DDZ_C2(const Field3D& f) { | ||
| checkData(f); | ||
|
|
||
| const auto region_id = f.getMesh()->getRegionID("RGN_NOBNDRY"); |
There was a problem hiding this comment.
warning: member access into incomplete type 'Mesh' [clang-diagnostic-error]
const auto region_id = f.getMesh()->getRegionID("RGN_NOBNDRY");
^Additional context
include/bout/field_data.hxx:47: forward declaration of 'Mesh'
class Mesh;
^| f.getLocation(), | ||
| f.getDirections(), | ||
| region_id, | ||
| f.getMesh()->getRegion("RGN_NOBNDRY")}; |
There was a problem hiding this comment.
warning: member access into incomplete type 'Mesh' [clang-diagnostic-error]
f.getMesh()->getRegion("RGN_NOBNDRY")};
^Additional context
include/bout/field_data.hxx:47: forward declaration of 'Mesh'
class Mesh;
^| inline bout::stencil::DDZExprC4 DDZ_C4(const Field3D& f) { | ||
| checkData(f); | ||
|
|
||
| const auto region_id = f.getMesh()->getRegionID("RGN_NOBNDRY"); |
There was a problem hiding this comment.
warning: member access into incomplete type 'Mesh' [clang-diagnostic-error]
const auto region_id = f.getMesh()->getRegionID("RGN_NOBNDRY");
^Additional context
include/bout/field_data.hxx:47: forward declaration of 'Mesh'
class Mesh;
^| f.getLocation(), | ||
| f.getDirections(), | ||
| region_id, | ||
| f.getMesh()->getRegion("RGN_NOBNDRY")}; |
There was a problem hiding this comment.
warning: member access into incomplete type 'Mesh' [clang-diagnostic-error]
f.getMesh()->getRegion("RGN_NOBNDRY")};
^Additional context
include/bout/field_data.hxx:47: forward declaration of 'Mesh'
class Mesh;
^Implements Arakawa bracket in X-Z as a BinaryExpr.
Testing the performance impact of runtime dispatch. The hope is to preserve the ability to switch method at runtime. The conditional that selects the method is the same for all iterations, so hopefully good branch prediction and no warp divergence. DDZ_Dispatch(f, method) selects between DDZ_C2 and DDZ_C4 at runtime.
| ddt(n) = | ||
| -bracket(phi, n, bm) + alpha * (nonzonal_phi - nonzonal_n) - kappa * DDZ(phi); | ||
| ddt(n) = -bracket_arakawa(phi, n) + alpha * (nonzonal_phi - nonzonal_n) | ||
| - kappa * DDZ_Dispatch(phi, DIFF_C2); |
There was a problem hiding this comment.
warning: no header providing "DIFF_C2" is directly included [misc-include-cleaner]
- kappa * DDZ_Dispatch(phi, DIFF_C2);
^| struct DDZ_Dispatch_Op { | ||
| CoordinatesAccessor coords; | ||
| int nz{0}; | ||
| DIFF_METHOD method{DIFF_DEFAULT}; |
There was a problem hiding this comment.
warning: no header providing "DIFF_DEFAULT" is directly included [misc-include-cleaner]
DIFF_METHOD method{DIFF_DEFAULT};
^| struct DDZ_Dispatch_Op { | ||
| CoordinatesAccessor coords; | ||
| int nz{0}; | ||
| DIFF_METHOD method{DIFF_DEFAULT}; |
There was a problem hiding this comment.
warning: no header providing "DIFF_METHOD" is directly included [misc-include-cleaner]
DIFF_METHOD method{DIFF_DEFAULT};
^|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { | ||
| switch (method) { | ||
| case DIFF_C2: |
There was a problem hiding this comment.
warning: no header providing "DIFF_C2" is directly included [misc-include-cleaner]
case DIFF_C2:
^| switch (method) { | ||
| case DIFF_C2: | ||
| return apply_c2(idx, lhs); | ||
| case DIFF_C4: |
There was a problem hiding this comment.
warning: no header providing "DIFF_C4" is directly included [misc-include-cleaner]
case DIFF_C4:
^|
|
||
| if ((method != DIFF_C2) && (method != DIFF_C4)) { | ||
| throw BoutException("DDZ_Dispatch only supports DIFF_C2 and DIFF_C4, got {:s}", | ||
| toString(method)); |
There was a problem hiding this comment.
warning: no header providing "toString" is directly included [misc-include-cleaner]
toString(method));
^| const Field3D& g) { | ||
| checkData(f); | ||
| checkData(g); | ||
| ASSERT1_FIELDS_COMPATIBLE(f, g); |
There was a problem hiding this comment.
warning: no header providing "ASSERT1_FIELDS_COMPATIBLE" is directly included [misc-include-cleaner]
include/bout/stencil_expr.hxx:1:
- #ifndef BOUT_STENCIL_EXPR_HXX
+ #include "bout/field2d.hxx"
+ #ifndef BOUT_STENCIL_EXPR_HXXRetains the runtime dispatch of DDZ, while building a BinaryExpr lazy expression.
Stores the default numerical methods as DIFF_METHOD enums. This enables runtime dispatch with minimal overhead in expressions.
Retains runtime choice of method, and handling of staggered inputs or outputs. Only C2 and C4 methods are supported.
| ITERATOR_TEST_BLOCK("DDZ Default", result = DDZ(a);); | ||
|
|
||
| ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, "DIFF_C2");); | ||
| ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, DIFF_C2);); |
There was a problem hiding this comment.
warning: no header providing "CELL_DEFAULT" is directly included [misc-include-cleaner]
ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, DIFF_C2););
^| ITERATOR_TEST_BLOCK("DDZ Default", result = DDZ(a);); | ||
|
|
||
| ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, "DIFF_C2");); | ||
| ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, DIFF_C2);); |
There was a problem hiding this comment.
warning: no header providing "DIFF_C2" is directly included [misc-include-cleaner]
ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, DIFF_C2););
^| ITERATOR_TEST_BLOCK("DDZ Default", result = DDZ(a);); | ||
|
|
||
| ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, "DIFF_C2");); | ||
| ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, DIFF_C2);); |
There was a problem hiding this comment.
warning: variable 'start' of type 'SteadyClock' (aka 'time_pointstd::chrono::steady_clock') can be declared 'const' [misc-const-correctness]
ITERATOR_TEST_BLOCK("DDZ C2", result = DDZ(a, CELL_DEFAULT, DIFF_C2););
^Additional context
examples/performance/ddz/ddz.cxx:27: expanded from macro 'ITERATOR_TEST_BLOCK'
SteadyClock start = steady_clock::now(); \
^| ITERATOR_TEST_BLOCK("DDZ W3", result = DDZ(a, CELL_DEFAULT, "DIFF_W3");); | ||
|
|
||
| ITERATOR_TEST_BLOCK("DDZ FFT", result = DDZ(a, CELL_DEFAULT, "DIFF_FFT");); | ||
| ITERATOR_TEST_BLOCK("DDZ C4", result = DDZ(a, CELL_DEFAULT, DIFF_C4);); |
There was a problem hiding this comment.
warning: no header providing "DIFF_C4" is directly included [misc-include-cleaner]
ITERATOR_TEST_BLOCK("DDZ C4", result = DDZ(a, CELL_DEFAULT, DIFF_C4););
^| ITERATOR_TEST_BLOCK("DDZ W3", result = DDZ(a, CELL_DEFAULT, "DIFF_W3");); | ||
|
|
||
| ITERATOR_TEST_BLOCK("DDZ FFT", result = DDZ(a, CELL_DEFAULT, "DIFF_FFT");); | ||
| ITERATOR_TEST_BLOCK("DDZ C4", result = DDZ(a, CELL_DEFAULT, DIFF_C4);); |
There was a problem hiding this comment.
warning: variable 'start' of type 'SteadyClock' (aka 'time_pointstd::chrono::steady_clock') can be declared 'const' [misc-const-correctness]
ITERATOR_TEST_BLOCK("DDZ C4", result = DDZ(a, CELL_DEFAULT, DIFF_C4););
^Additional context
examples/performance/ddz/ddz.cxx:27: expanded from macro 'ITERATOR_TEST_BLOCK'
SteadyClock start = steady_clock::now(); \
^| static constexpr int num_staggers = 3; | ||
| static constexpr int num_deriv_kinds = 5; | ||
|
|
||
| std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{}; |
There was a problem hiding this comment.
warning: performing an implicit widening conversion to type 'std::size_t' (aka 'unsigned long') of a multiplication performed in type 'int' [bugprone-implicit-widening-of-multiplication-result]
std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{};
^Additional context
include/bout/mesh.hxx:103: make conversion explicit to silence this warning
std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{};
^include/bout/mesh.hxx:103: perform multiplication in a wider type
std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{};
^|
|
||
| DIFF_METHOD get(DIRECTION direction, DERIV deriv, | ||
| STAGGER stagger = STAGGER::None) const { | ||
| return values[(directionIndex(direction) * num_staggers + staggerIndex(stagger)) |
There was a problem hiding this comment.
warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
return values[(directionIndex(direction) * num_staggers + staggerIndex(stagger))
^There was a problem hiding this comment.
I think values.at() suppresses this warning
| DIFF_METHOD get(DIRECTION direction, DERIV deriv, | ||
| STAGGER stagger = STAGGER::None) const { | ||
| return values[(directionIndex(direction) * num_staggers + staggerIndex(stagger)) | ||
| * num_deriv_kinds |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| * num_deriv_kinds | |
| return values[((directionIndex(direction) * num_staggers + staggerIndex(stagger)) | |
| * num_deriv_kinds) |
|
|
||
| void set(DIRECTION direction, DERIV deriv, DIFF_METHOD method, | ||
| STAGGER stagger = STAGGER::None) { | ||
| values[(directionIndex(direction) * num_staggers + staggerIndex(stagger)) |
There was a problem hiding this comment.
warning: do not use array subscript when the index is not an integer constant expression [cppcoreguidelines-pro-bounds-constant-array-index]
values[(directionIndex(direction) * num_staggers + staggerIndex(stagger))
^| void set(DIRECTION direction, DERIV deriv, DIFF_METHOD method, | ||
| STAGGER stagger = STAGGER::None) { | ||
| values[(directionIndex(direction) * num_staggers + staggerIndex(stagger)) | ||
| * num_deriv_kinds |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| * num_deriv_kinds | |
| values[((directionIndex(direction) * num_staggers + staggerIndex(stagger)) | |
| * num_deriv_kinds) |
ZedThree
left a comment
ZedThree
left a comment
There was a problem hiding this comment.
Is this intended as a replacement for the existing derivatives generation machinery?
| template <typename ResT, typename L, typename R, typename Func> | ||
| struct expression_result<BinaryExpr<ResT, L, R, Func>> { | ||
| using type = ResT; | ||
| }; |
There was a problem hiding this comment.
Might be clearer to consistently just use Result instead of ResT? It took me a minute to decipher that
| static constexpr int num_directions = 3; | ||
| static constexpr int num_staggers = 3; | ||
| static constexpr int num_deriv_kinds = 5; |
There was a problem hiding this comment.
Making these std::size_t might fix a bunch of the clang-tidy warnings?
| } | ||
| } | ||
|
|
||
| static DIFF_METHOD builtinDefaultMethod(DERIV deriv) { |
There was a problem hiding this comment.
I think these can be also be constexpr
| case DERIV::Flux: | ||
| return DIFF_U1; | ||
| } | ||
| throw BoutException("Unhandled derivative kind in builtinDefaultMethod"); |
There was a problem hiding this comment.
We could use something like C++23's std::unreachable instead here
|
|
||
| DIFF_METHOD get(DIRECTION direction, DERIV deriv, | ||
| STAGGER stagger = STAGGER::None) const { | ||
| return values[(directionIndex(direction) * num_staggers + staggerIndex(stagger)) |
There was a problem hiding this comment.
I think values.at() suppresses this warning
| case STAGGER::L2C: | ||
| return apply_c2_l2c(idx, lhs); | ||
| } | ||
| return 0.0; |
There was a problem hiding this comment.
This could also use an unreachable() function
Hi @ZedThree ! Yes, this would replace the index_derivs machinery. The current kernels are optimized for CPUs but are too small to run efficiently on GPUs. To have any chance of running on GPUs efficiently, kernels need to be merged into larger expressions. The single index operators were one way, but these template expressions hide a lot of the ceremony of creating field accessors, capturing variables etc. The interface stays essentially the same, apart from explicitly converting BinaryExpr to fields in some places. |
Merge the lazy `pow` implementation. Add a `DDZ_FFT` function for when FFT is needed.
| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector3D DDZ(const Vector3D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:113: the definition seen here
Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, DIFF_METHOD method,
^include/bout/derivs.hxx:150: differing parameters are named here: ('f'), in definition: ('v')
^| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector2D DDZ(const Vector2D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:149: the definition seen here
Vector2D DDZ(const Vector2D& v, CELL_LOC UNUSED(outloc), DIFF_METHOD UNUSED(method),
^include/bout/derivs.hxx:171: differing parameters are named here: ('f'), in definition: ('v')
^| BinaryExpr<FieldPerp, L, R, bout::op::Pow>> | ||
| pow(const L& lhs, const R& rhs) { | ||
| ASSERT1_EXPR_COMPATIBLE(lhs, rhs); | ||
| ASSERT1(lhs.getIndex() == rhs.getIndex()); |
There was a problem hiding this comment.
warning: no header providing "ASSERT1" is directly included [misc-include-cleaner]
;
^| static constexpr int num_staggers = 3; | ||
| static constexpr int num_deriv_kinds = 5; | ||
|
|
||
| std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{}; |
There was a problem hiding this comment.
warning: performing an implicit widening conversion to type 'std::size_t' (aka 'unsigned long') of a multiplication performed in type 'int' [bugprone-implicit-widening-of-multiplication-result]
std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{};
^Additional context
include/bout/mesh.hxx:104: make conversion explicit to silence this warning
std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{};
^include/bout/mesh.hxx:104: perform multiplication in a wider type
std::array<DIFF_METHOD, num_directions * num_staggers * num_deriv_kinds> values{};
^| BoutReal fft_derivs_filter{0.0}; | ||
|
|
||
| /// Concrete default methods initialised from options in derivs_init | ||
| DerivativeDefaults derivative_defaults{}; |
There was a problem hiding this comment.
warning: initializer for member 'derivative_defaults' is redundant [readability-redundant-member-init]
| DerivativeDefaults derivative_defaults{}; | |
| DerivativeDefaults derivative_defaults; |
| return DIFF_S2; | ||
| } | ||
| if (method == "DEFAULT") { | ||
| throw BoutException("Default derivative options must resolve to a concrete method"); |
There was a problem hiding this comment.
warning: no header providing "BoutException" is directly included [misc-include-cleaner]
src/mesh/index_derivs.cxx:22:
- #include "bout/build_defines.hxx"
+ #include "bout/boutexception.hxx"
+ #include "bout/build_defines.hxx"| DerivativeStore<Field3D>::getInstance().initialise(options); | ||
| DerivativeStore<Field2D>::getInstance().initialise(options); | ||
|
|
||
| auto backup_section = options->getSection("diff"); |
There was a problem hiding this comment.
warning: 'auto backup_section' can be declared as 'auto *backup_section' [readability-qualified-auto]
| auto backup_section = options->getSection("diff"); | |
| auto *backup_section = options->getSection("diff"); |
| DerivativeStore<Field2D>::getInstance().initialise(options); | ||
|
|
||
| auto backup_section = options->getSection("diff"); | ||
| const std::array<std::pair<DIRECTION, std::string>, 3> directions{{ |
There was a problem hiding this comment.
warning: no header providing "DIRECTION" is directly included [misc-include-cleaner]
const std::array<std::pair<DIRECTION, std::string>, 3> directions{{
^| DerivativeStore<Field2D>::getInstance().initialise(options); | ||
|
|
||
| auto backup_section = options->getSection("diff"); | ||
| const std::array<std::pair<DIRECTION, std::string>, 3> directions{{ |
There was a problem hiding this comment.
warning: no header providing "std::pair" is directly included [misc-include-cleaner]
src/mesh/index_derivs.cxx:32:
+ #include <utility>| {DIRECTION::Y, "ddy"}, | ||
| {DIRECTION::Z, "ddz"}, | ||
| }}; | ||
| const std::array<std::pair<DERIV, std::string>, 5> deriv_types{{ |
There was a problem hiding this comment.
warning: no header providing "DERIV" is directly included [misc-include-cleaner]
const std::array<std::pair<DERIV, std::string>, 5> deriv_types{{
^Fixes failing serial tests
| backupSection->get(derivName, theDefault, ""); | ||
| } | ||
|
|
||
| if (uppercase(theDefault) == toString(DIFF_DEFAULT)) { |
There was a problem hiding this comment.
warning: no header providing "uppercase" is directly included [misc-include-cleaner]
include/bout/deriv_store.hxx:39:
- #include <bout/scorepwrapper.hxx>
+ #include "bout/utils.hxx"
+ #include <bout/scorepwrapper.hxx>| derivative_defaults = DerivativeDefaults{}; | ||
|
|
||
| for (const auto& [direction, section_name] : directions) { | ||
| auto specific_section = options->getSection(section_name); |
There was a problem hiding this comment.
warning: 'auto specific_section' can be declared as 'auto *specific_section' [readability-qualified-auto]
| auto specific_section = options->getSection(section_name); | |
| auto *specific_section = options->getSection(section_name); |
|
|
||
| for (const auto& [direction, section_name] : directions) { | ||
| auto specific_section = options->getSection(section_name); | ||
| auto staggered_section = options->getSection(section_name + "stag"); |
There was a problem hiding this comment.
warning: 'auto staggered_section' can be declared as 'auto *staggered_section' [readability-qualified-auto]
| auto staggered_section = options->getSection(section_name + "stag"); | |
| auto *staggered_section = options->getSection(section_name + "stag"); |
|
|
||
| for (const auto& [deriv, option_name] : deriv_types) { | ||
| auto default_method = DerivativeDefaults::builtinDefaultMethod(deriv); | ||
| auto default_name = toString(default_method); |
There was a problem hiding this comment.
warning: no header providing "toString" is directly included [misc-include-cleaner]
auto default_name = toString(default_method);
^| } | ||
|
|
||
| default_method = parseConcreteDiffMethod(default_name); | ||
| derivative_defaults.set(direction, deriv, default_method, STAGGER::None); |
There was a problem hiding this comment.
warning: no header providing "STAGGER" is directly included [misc-include-cleaner]
derivative_defaults.set(direction, deriv, default_method, STAGGER::None);
^| const std::string& UNUSED(method), const std::string& UNUSED(region)) { | ||
| Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, const std::string& method, | ||
| const std::string& region) { | ||
| return DDZ(v, outloc, parseDDZMethodString(method), region); |
There was a problem hiding this comment.
warning: no header providing "(anonymous namespace)::parseDDZMethodString" is directly included [misc-include-cleaner]
return DDZ(v, outloc, parseDDZMethodString(method), region);
^
| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector3D DDZ(const Vector3D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:99: the definition seen here
Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, DIFF_METHOD method,
^include/bout/derivs.hxx:146: differing parameters are named here: ('f'), in definition: ('v')
^| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector2D DDZ(const Vector2D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:135: the definition seen here
Vector2D DDZ(const Vector2D& v, CELL_LOC UNUSED(outloc), DIFF_METHOD UNUSED(method),
^include/bout/derivs.hxx:167: differing parameters are named here: ('f'), in definition: ('v')
^|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
| const BoutReal sm_p = ma - lhs(ixp); | ||
| const BoutReal sm_pp = ma - lhs(ixp2); | ||
|
|
||
| BoutReal r = (WENO_SMALL + SQ(sp_c - 2.0 * sp_m + sp_mm)) |
There was a problem hiding this comment.
warning: '*' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
| BoutReal r = (WENO_SMALL + SQ(sp_c - 2.0 * sp_m + sp_mm)) | |
| BoutReal r = (WENO_SMALL + SQ(sp_c - (2.0 * sp_m) + sp_mm)) |
|
|
||
| // z-component of 1./(C1*D) * Grad_perp(C2) | ||
| Field3D coef_z = DDZ(C2coef, location, "FFT") / C1TimesD; | ||
| Field3D coef_z = DDZ(C2coef, location, DIFF_C4) / C1TimesD; |
There was a problem hiding this comment.
warning: no header providing "DDZ" is directly included [misc-include-cleaner]
Field3D coef_z = DDZ(C2coef, location, DIFF_C4) / C1TimesD;
^| for (const auto& ind : coords->dx().getRegion("RGN_ALL")) { | ||
| COPY_STRIPE(dx, dy, dz); | ||
| COPY_STRIPE(d1_dx, d1_dy, d1_dz); | ||
| COPY_STRIPE(IntShiftTorsion); |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
COPY_STRIPE(IntShiftTorsion);
^Additional context
src/mesh/coordinates_accessor.cxx:51: expanded from macro 'COPY_STRIPE'
MACRO_FOR_EACH(COPY_STRIPE1, __VA_ARGS__) \
^include/bout/macro_for_each.hxx:99: expanded from macro 'MACRO_FOR_EACH'
_me_1)(mac, __VA_ARGS__))
^include/bout/macro_for_each.hxx:19: expanded from macro '_me_1'
#define _me_1(_call, x) _call(x)
^src/mesh/coordinates_accessor.cxx:44: expanded from macro 'COPY_STRIPE1'
data[stripe_size * ind.ind + static_cast<int>(Offset::symbol)] = \
^include/bout/macro_for_each.hxx:16: expanded from macro 'BOUT_EXPAND'
#define BOUT_EXPAND(x) x
^|
|
||
| for (const auto& ind : coords->dx().getRegion("RGN_ALL")) { | ||
| if (G1.isAllocated()) { | ||
| COPY_STRIPE(G1); |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
COPY_STRIPE(G1);
^Additional context
src/mesh/coordinates_accessor.cxx:51: expanded from macro 'COPY_STRIPE'
MACRO_FOR_EACH(COPY_STRIPE1, __VA_ARGS__) \
^include/bout/macro_for_each.hxx:99: expanded from macro 'MACRO_FOR_EACH'
_me_1)(mac, __VA_ARGS__))
^include/bout/macro_for_each.hxx:19: expanded from macro '_me_1'
#define _me_1(_call, x) _call(x)
^src/mesh/coordinates_accessor.cxx:44: expanded from macro 'COPY_STRIPE1'
data[stripe_size * ind.ind + static_cast<int>(Offset::symbol)] = \
^include/bout/macro_for_each.hxx:16: expanded from macro 'BOUT_EXPAND'
#define BOUT_EXPAND(x) x
^| COPY_STRIPE(G1); | ||
| } | ||
| if (G3.isAllocated()) { | ||
| COPY_STRIPE(G3); |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
COPY_STRIPE(G3);
^Additional context
src/mesh/coordinates_accessor.cxx:51: expanded from macro 'COPY_STRIPE'
MACRO_FOR_EACH(COPY_STRIPE1, __VA_ARGS__) \
^include/bout/macro_for_each.hxx:99: expanded from macro 'MACRO_FOR_EACH'
_me_1)(mac, __VA_ARGS__))
^include/bout/macro_for_each.hxx:19: expanded from macro '_me_1'
#define _me_1(_call, x) _call(x)
^src/mesh/coordinates_accessor.cxx:44: expanded from macro 'COPY_STRIPE1'
data[stripe_size * ind.ind + static_cast<int>(Offset::symbol)] = \
^include/bout/macro_for_each.hxx:16: expanded from macro 'BOUT_EXPAND'
#define BOUT_EXPAND(x) x
^| const std::string& UNUSED(method), const std::string& UNUSED(region)) { | ||
| Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, const std::string& method, | ||
| const std::string& region) { | ||
| return DDZ(v, outloc, parseField3DMethodString(method), region); |
There was a problem hiding this comment.
warning: no header providing "(anonymous namespace)::parseField3DMethodString" is directly included [misc-include-cleaner]
return DDZ(v, outloc, parseField3DMethodString(method), region);
^DDZ is restored to the previous (eager) implementation that can dispatch to the FFT method. The new lazy implementation is `DDZ_stencil` because it is limited to stencil-based methods.
| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector3D DDZ(const Vector3D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:114: the definition seen here
Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, DIFF_METHOD method,
^include/bout/derivs.hxx:166: differing parameters are named here: ('f'), in definition: ('v')
^| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector2D DDZ(const Vector2D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:150: the definition seen here
Vector2D DDZ(const Vector2D& v, CELL_LOC UNUSED(outloc), DIFF_METHOD UNUSED(method),
^include/bout/derivs.hxx:187: differing parameters are named here: ('f'), in definition: ('v')
^| const std::string& UNUSED(method), const std::string& UNUSED(region)) { | ||
| Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, const std::string& method, | ||
| const std::string& region) { | ||
| return DDZ(v, outloc, parseField3DMethodString(method), region); |
There was a problem hiding this comment.
warning: no header providing "(anonymous namespace)::parseField3DMethodString" is directly included [misc-include-cleaner]
src/sys/derivs.cxx:40:
- #include <bout/assert.hxx>
+ #include "bout/stencil_expr.hxx"
+ #include <bout/assert.hxx>
| #include <bout/options.hxx> | ||
| #include <bout/output.hxx> | ||
| #include <bout/scorepwrapper.hxx> | ||
| #include <bout/utils.hxx> |
There was a problem hiding this comment.
warning: included header scorepwrapper.hxx is not used directly [misc-include-cleaner]
| #include <bout/utils.hxx> | |
| #include <bout/utils.hxx> |
| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector3D DDZ(const Vector3D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:115: the definition seen here
Vector3D DDZ(const Vector3D& v, CELL_LOC outloc, DIFF_METHOD method,
^include/bout/derivs.hxx:166: differing parameters are named here: ('f'), in definition: ('v')
^| /// If not given, defaults to DIFF_DEFAULT | ||
| /// @param[in] region What region is expected to be calculated | ||
| /// If not given, defaults to RGN_NOBNDRY | ||
| Vector2D DDZ(const Vector2D& f, CELL_LOC outloc = CELL_DEFAULT, |
There was a problem hiding this comment.
warning: function 'DDZ' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
^Additional context
src/sys/derivs.cxx:151: the definition seen here
Vector2D DDZ(const Vector2D& v, CELL_LOC UNUSED(outloc), DIFF_METHOD UNUSED(method),
^include/bout/derivs.hxx:187: differing parameters are named here: ('f'), in definition: ('v')
^| if (coords->symbol().isAllocated()) { \ | ||
| data[stripe_size * ind.ind + static_cast<int>(Offset::symbol)] = \ | ||
| coords->symbol()[ind]; \ | ||
| #define COPY_STRIPE1(symbol) \ |
There was a problem hiding this comment.
warning: function-like macro 'COPY_STRIPE1' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define COPY_STRIPE1(symbol) \
^Ensure that regions are propagated through index derivatives, and ensure that Bxy has yup/down slices before trying to access them.
| @@ -85,7 +85,9 @@ T flowDerivative(const T& vel, const T& f, CELL_LOC outloc, const std::string& m | |||
| const int nPoint = localmesh->getNpoints(direction); | |||
There was a problem hiding this comment.
warning: member access into incomplete type 'Mesh' [clang-diagnostic-error]
const int nPoint = localmesh->getNpoints(direction);
^Additional context
include/bout/boundary_region.hxx:9: forward declaration of 'Mesh'
class Mesh;
^| @@ -146,7 +149,9 @@ T standardDerivative(const T& f, CELL_LOC outloc, const std::string& method, | |||
| const int nPoint = localmesh->getNpoints(direction); | |||
There was a problem hiding this comment.
warning: member access into incomplete type 'Mesh' [clang-diagnostic-error]
const int nPoint = localmesh->getNpoints(direction);
^Additional context
include/bout/boundary_region.hxx:9: forward declaration of 'Mesh'
class Mesh;
^Requires yup/ydown fields so that the to/fromFieldAligned transformations are not needed.
Alternative that takes Field3D and checks hasParallelSlices().
Issue arises because only one guard cell is set in toField.
|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
|
|
||
| template <typename LView, typename RView> | ||
| BOUT_HOST_DEVICE BOUT_FORCEINLINE BoutReal operator()(int idx, const LView& lhs, | ||
| const RView&) const { |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| const RView&) const { | |
| const RView& /*unused*/) const { |
| "boundaries before calling DDY_stencil."); | ||
| } | ||
|
|
||
| ASSERT1(f.getDirectionY() == YDirectionType::Standard); |
There was a problem hiding this comment.
warning: no header providing "ASSERT1" is directly included [misc-include-cleaner]
ASSERT1(f.getDirectionY() == YDirectionType::Standard);
^2 participants
Operators return BinaryExpr types that perform derivatives 'lazily'. Like the single index operators they capture a CoordinatesAccessor to provide access to metrics etc. The difference is that the user code doesn't need to create field accessors or explicitly write the loop, capture variables etc. All that is done automatically if the compiler is smart/sufficiently aggressively optimizing.
Implemented DDZ_C2 and DDZ_C4 operators for Z derivatives. These appear to inline as expected in the Hasegawa-Wakatani example.
The DDZ(Field3D) operator now returns a lazy BinaryExpr type. It still allows runtime choice of numerical method (C2 / C4) and staggered fields.