From: Artem Zhmurov Date: Fri, 23 Apr 2021 09:05:24 +0000 (+0000) Subject: Clang-11 fixes for PME and FFT X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=e9d9588e470fc1928bb1e989197e72457abf1076;p=alexxy%2Fgromacs.git Clang-11 fixes for PME and FFT --- diff --git a/src/gromacs/ewald/ewald_utils.h b/src/gromacs/ewald/ewald_utils.h index 9f77967c37..4639d1571b 100644 --- a/src/gromacs/ewald/ewald_utils.h +++ b/src/gromacs/ewald/ewald_utils.h @@ -3,7 +3,7 @@ * * Copyright (c) 1991-2000, University of Groningen, The Netherlands. * Copyright (c) 2001-2004, The GROMACS development team. - * Copyright (c) 2013,2014,2017,2019, by the GROMACS development team, led by + * Copyright (c) 2013,2014,2017,2019,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -122,7 +122,7 @@ public: * \param[in] box The current box matrix * \param[out] scaledBox Scaled copy of the box matrix. */ - void scaleBox(const matrix box, matrix scaledBox) + void scaleBox(const matrix box, matrix scaledBox) const { GMX_ASSERT(box, "invalid source box pointer"); GMX_ASSERT(scaledBox, "invalid target box pointer"); diff --git a/src/gromacs/ewald/pme_gpu_program_impl.cu b/src/gromacs/ewald/pme_gpu_program_impl.cu index 6a6196fd5c..b7830fa60b 100644 --- a/src/gromacs/ewald/pme_gpu_program_impl.cu +++ b/src/gromacs/ewald/pme_gpu_program_impl.cu @@ -179,4 +179,4 @@ PmeGpuProgramImpl::PmeGpuProgramImpl(const DeviceContext& deviceContext) : // clang-format on } -PmeGpuProgramImpl::~PmeGpuProgramImpl() {} +PmeGpuProgramImpl::~PmeGpuProgramImpl() = default; diff --git a/src/gromacs/ewald/pme_gpu_program_impl.h b/src/gromacs/ewald/pme_gpu_program_impl.h index 4bb3ec1a5b..8a1e8afaf1 100644 --- a/src/gromacs/ewald/pme_gpu_program_impl.h +++ b/src/gromacs/ewald/pme_gpu_program_impl.h @@ -170,6 +170,7 @@ struct PmeGpuProgramImpl PmeGpuProgramImpl() = delete; //! Constructor for the given device explicit PmeGpuProgramImpl(const DeviceContext& deviceContext); + // NOLINTNEXTLINE(performance-trivially-destructible) ~PmeGpuProgramImpl(); GMX_DISALLOW_COPY_AND_ASSIGN(PmeGpuProgramImpl); diff --git a/src/gromacs/ewald/pme_load_balancing.cpp b/src/gromacs/ewald/pme_load_balancing.cpp index 2e900d3bd4..f554e43827 100644 --- a/src/gromacs/ewald/pme_load_balancing.cpp +++ b/src/gromacs/ewald/pme_load_balancing.cpp @@ -44,6 +44,7 @@ */ #include "gmxpre.h" +#include "gromacs/utility/enumerationhelpers.h" #include "pme_load_balancing.h" #include @@ -142,22 +143,28 @@ const int c_numPostSwitchTuningIntervalSkip = 1; const double c_startupTimeDelay = 5.0; /*! \brief Enumeration whose values describe the effect limiting the load balancing */ -enum epmelb +enum class PmeLoadBalancingLimit : int { - epmelblimNO, - epmelblimBOX, - epmelblimDD, - epmelblimPMEGRID, - epmelblimMAXSCALING, - epmelblimNR + No, + Box, + DD, + PmeGrid, + MaxScaling, + Count }; -/*! \brief Descriptive strings matching ::epmelb */ -static const char* pmelblim_str[epmelblimNR] = { "no", - "box size", - "domain decompostion", - "PME grid restriction", - "maximum allowed grid scaling" }; +/*! \brief Descriptive strings for PmeLoadBalancingLimit \c enumValue */ +static const char* enumValueToString(PmeLoadBalancingLimit enumValue) +{ + constexpr gmx::EnumerationArray pmeLoadBalancingLimitNames = { + "no", + "box size", + "domain decompostion", + "PME grid restriction", + "maximum allowed grid scaling" + }; + return pmeLoadBalancingLimitNames[enumValue]; +} struct pme_load_balancing_t { @@ -183,7 +190,7 @@ struct pme_load_balancing_t int lower_limit; /**< don't go below this setup index */ int start; /**< start of setup index range to consider in stage>0 */ int end; /**< end of setup index range to consider in stage>0 */ - int elimited; /**< was the balancing limited, uses enum above */ + PmeLoadBalancingLimit elimited; /**< was the balancing limited, uses enum above */ CutoffScheme cutoff_scheme; /**< Verlet or group cut-offs */ int stage; /**< the current stage */ @@ -294,7 +301,7 @@ void pme_loadbal_init(pme_load_balancing_t** pme_lb_p, pme_lb->lower_limit = 0; pme_lb->start = 0; pme_lb->end = 0; - pme_lb->elimited = epmelblimNO; + pme_lb->elimited = PmeLoadBalancingLimit::No; pme_lb->cycles_n = 0; pme_lb->cycles_c = 0; @@ -500,7 +507,7 @@ static void print_loadbal_limited(FILE* fp_err, FILE* fp_log, int64_t step, pme_ auto buf = gmx::formatString( "step %4s: the %s limits the PME load balancing to a coulomb cut-off of %.3f", gmx::int64ToString(step).c_str(), - pmelblim_str[pme_lb->elimited], + enumValueToString(pme_lb->elimited), pme_lb->setup[pme_loadbal_end(pme_lb) - 1].rcut_coulomb); if (fp_err != nullptr) { @@ -694,7 +701,7 @@ static void pme_load_balance(pme_load_balancing_t* pme_lb, if (!OK) { - pme_lb->elimited = epmelblimPMEGRID; + pme_lb->elimited = PmeLoadBalancingLimit::PmeGrid; } } @@ -702,7 +709,7 @@ static void pme_load_balance(pme_load_balancing_t* pme_lb, && pme_lb->setup[pme_lb->cur + 1].spacing > c_maxSpacingScaling * pme_lb->setup[0].spacing) { OK = FALSE; - pme_lb->elimited = epmelblimMAXSCALING; + pme_lb->elimited = PmeLoadBalancingLimit::MaxScaling; } if (OK && ir.pbcType != PbcType::No) @@ -711,7 +718,7 @@ static void pme_load_balance(pme_load_balancing_t* pme_lb, <= max_cutoff2(ir.pbcType, box)); if (!OK) { - pme_lb->elimited = epmelblimBOX; + pme_lb->elimited = PmeLoadBalancingLimit::Box; } } @@ -726,7 +733,7 @@ static void pme_load_balance(pme_load_balancing_t* pme_lb, { /* Failed: do not use this setup */ pme_lb->cur--; - pme_lb->elimited = epmelblimDD; + pme_lb->elimited = PmeLoadBalancingLimit::DD; } } } @@ -818,7 +825,7 @@ static void pme_load_balance(pme_load_balancing_t* pme_lb, /* Limit the range to below the current cut-off, scan from start */ pme_lb->end = pme_lb->cur; pme_lb->cur = pme_lb->start; - pme_lb->elimited = epmelblimDD; + pme_lb->elimited = PmeLoadBalancingLimit::DD; print_loadbal_limited(fp_err, fp_log, step, pme_lb); } } @@ -1140,13 +1147,13 @@ static void print_pme_loadbal_settings(pme_load_balancing_t* pme_lb, fprintf(fplog, " P P - P M E L O A D B A L A N C I N G\n"); fprintf(fplog, "\n"); /* Here we only warn when the optimal setting is the last one */ - if (pme_lb->elimited != epmelblimNO && pme_lb->cur == pme_loadbal_end(pme_lb) - 1) + if (pme_lb->elimited != PmeLoadBalancingLimit::No && pme_lb->cur == pme_loadbal_end(pme_lb) - 1) { fprintf(fplog, " NOTE: The PP/PME load balancing was limited by the %s,\n", - pmelblim_str[pme_lb->elimited]); + enumValueToString(pme_lb->elimited)); fprintf(fplog, " you might not have reached a good load balance.\n"); - if (pme_lb->elimited == epmelblimDD) + if (pme_lb->elimited == PmeLoadBalancingLimit::DD) { fprintf(fplog, " Try different mdrun -dd settings or lower the -dds value.\n"); } @@ -1179,7 +1186,7 @@ static void print_pme_loadbal_settings(pme_load_balancing_t* pme_lb, void pme_loadbal_done(pme_load_balancing_t* pme_lb, FILE* fplog, const gmx::MDLogger& mdlog, gmx_bool bNonBondedOnGPU) { - if (fplog != nullptr && (pme_lb->cur > 0 || pme_lb->elimited != epmelblimNO)) + if (fplog != nullptr && (pme_lb->cur > 0 || pme_lb->elimited != PmeLoadBalancingLimit::No)) { print_pme_loadbal_settings(pme_lb, fplog, mdlog, bNonBondedOnGPU); } diff --git a/src/gromacs/ewald/tests/.clang-tidy b/src/gromacs/ewald/tests/.clang-tidy new file mode 100644 index 0000000000..0adf51e3ee --- /dev/null +++ b/src/gromacs/ewald/tests/.clang-tidy @@ -0,0 +1,91 @@ +# List of rationales for check suppressions (where known). +# This have to precede the list because inline comments are not +# supported by clang-tidy. +# +# -cppcoreguidelines-non-private-member-variables-in-classes, +# -misc-non-private-member-variables-in-classes, +# We intend a gradual transition to conform to this guideline, but it +# is not practical to implement yet. +# +# -readability-isolate-declaration, +# Declarations like "int a, b;" are readable. Some forms are not, and +# those might reasonably be suggested against during code review. +# +# -cppcoreguidelines-avoid-c-arrays, +# C arrays are still necessary in many places with legacy code +# +# -cppcoreguidelines-avoid-magic-numbers, +# -readability-magic-numbers, +# We have many legitimate use cases for magic numbers +# +# -cppcoreguidelines-macro-usage, +# We do use too many macros, and we should fix many of them, but there +# is no reasonable way to suppress the check e.g. in src/config.h and +# configuring the build is a major legitimate use of macros. +# +# -cppcoreguidelines-narrowing-conversions, +# -bugprone-narrowing-conversions +# We have many cases where int is converted to float and we don't care +# enough about such potential loss of precision to use explicit casts +# in large numbers of places. +# +# -google-readability-avoid-underscore-in-googletest-name +# We need to use underscores for readability for our legacy types +# and command-line parameter names +# +# -misc-no-recursion +# We have way too many functions and methods relying on recursion +# +# -cppcoreguidelines-avoid-non-const-global-variables +# There are quite a lot of static variables in the test code that +# can not be replaced. +# +# -modernize-avoid-bind +# Some code needs to use std::bind and can't be modernized quickly. +Checks: clang-diagnostic-*,-clang-analyzer-*,-clang-analyzer-security.insecureAPI.strcpy, + bugprone-*,misc-*,readability-*,performance-*,mpi-*, + -readability-inconsistent-declaration-parameter-name, + -readability-function-size,-readability-else-after-return, + modernize-use-nullptr,modernize-use-emplace, + modernize-make-unique,modernize-make-shared, + modernize-avoid-bind, + modernize-use-override, + modernize-redundant-void-arg,modernize-use-bool-literals, + cppcoreguidelines-*,-cppcoreguidelines-pro-*,-cppcoreguidelines-owning-memory, + -cppcoreguidelines-no-malloc,-cppcoreguidelines-special-member-functions, + -cppcoreguidelines-avoid-goto, + google-*,-google-build-using-namespace,-google-explicit-constructor, + -google-readability-function-size,-google-readability-todo,-google-runtime-int, + -cppcoreguidelines-non-private-member-variables-in-classes, + -misc-non-private-member-variables-in-classes, + -readability-isolate-declaration, + -cppcoreguidelines-avoid-c-arrays, + -cppcoreguidelines-avoid-magic-numbers, + -readability-magic-numbers, + -cppcoreguidelines-macro-usage, + -cppcoreguidelines-narrowing-conversions, + -bugprone-narrowing-conversions, + -google-readability-avoid-underscore-in-googletest-name, + -cppcoreguidelines-init-variables, + -misc-no-recursion, + -cppcoreguidelines-avoid-non-const-global-variables, + -modernize-avoid-bind +HeaderFilterRegex: .* +CheckOptions: + - key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor + value: 1 + - key: modernize-make-unique.IncludeStyle + value: google + - key: modernize-make-shared.IncludeStyle + value: google + - key: readability-implicit-bool-conversion.AllowIntegerConditions + value: 1 + - key: readability-implicit-bool-conversion.AllowPointerConditions + value: 1 + - key: bugprone-dangling-handle.HandleClasses + value: std::basic_string_view; nonstd::sv_lite::basic_string_view +# Permit passing shard pointers by value for sink parameters + - key: performance-unnecessary-copy-initialization.AllowedTypes + value: shared_ptr + - key: performance-unnecessary-value-param.AllowedTypes + value: shared_ptr diff --git a/src/gromacs/fft/fft5d.cpp b/src/gromacs/fft/fft5d.cpp index be74cbaf6d..39fbed3d42 100644 --- a/src/gromacs/fft/fft5d.cpp +++ b/src/gromacs/fft/fft5d.cpp @@ -92,6 +92,7 @@ FILE* debug = 0; /* none of the fftw3 calls, except execute(), are thread-safe, so we need to serialize them with this mutex. */ +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static std::mutex big_fftw_mutex; # define FFTW_LOCK \ try \ diff --git a/src/gromacs/fft/fft_fftw3.cpp b/src/gromacs/fft/fft_fftw3.cpp index 6c704af93b..f525af4026 100644 --- a/src/gromacs/fft/fft_fftw3.cpp +++ b/src/gromacs/fft/fft_fftw3.cpp @@ -57,6 +57,7 @@ /* none of the fftw3 calls, except execute(), are thread-safe, so we need to serialize them with this mutex. */ +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static std::mutex big_fftw_mutex; #define FFTW_LOCK \ try \ diff --git a/src/gromacs/fft/tests/.clang-tidy b/src/gromacs/fft/tests/.clang-tidy new file mode 100644 index 0000000000..0adf51e3ee --- /dev/null +++ b/src/gromacs/fft/tests/.clang-tidy @@ -0,0 +1,91 @@ +# List of rationales for check suppressions (where known). +# This have to precede the list because inline comments are not +# supported by clang-tidy. +# +# -cppcoreguidelines-non-private-member-variables-in-classes, +# -misc-non-private-member-variables-in-classes, +# We intend a gradual transition to conform to this guideline, but it +# is not practical to implement yet. +# +# -readability-isolate-declaration, +# Declarations like "int a, b;" are readable. Some forms are not, and +# those might reasonably be suggested against during code review. +# +# -cppcoreguidelines-avoid-c-arrays, +# C arrays are still necessary in many places with legacy code +# +# -cppcoreguidelines-avoid-magic-numbers, +# -readability-magic-numbers, +# We have many legitimate use cases for magic numbers +# +# -cppcoreguidelines-macro-usage, +# We do use too many macros, and we should fix many of them, but there +# is no reasonable way to suppress the check e.g. in src/config.h and +# configuring the build is a major legitimate use of macros. +# +# -cppcoreguidelines-narrowing-conversions, +# -bugprone-narrowing-conversions +# We have many cases where int is converted to float and we don't care +# enough about such potential loss of precision to use explicit casts +# in large numbers of places. +# +# -google-readability-avoid-underscore-in-googletest-name +# We need to use underscores for readability for our legacy types +# and command-line parameter names +# +# -misc-no-recursion +# We have way too many functions and methods relying on recursion +# +# -cppcoreguidelines-avoid-non-const-global-variables +# There are quite a lot of static variables in the test code that +# can not be replaced. +# +# -modernize-avoid-bind +# Some code needs to use std::bind and can't be modernized quickly. +Checks: clang-diagnostic-*,-clang-analyzer-*,-clang-analyzer-security.insecureAPI.strcpy, + bugprone-*,misc-*,readability-*,performance-*,mpi-*, + -readability-inconsistent-declaration-parameter-name, + -readability-function-size,-readability-else-after-return, + modernize-use-nullptr,modernize-use-emplace, + modernize-make-unique,modernize-make-shared, + modernize-avoid-bind, + modernize-use-override, + modernize-redundant-void-arg,modernize-use-bool-literals, + cppcoreguidelines-*,-cppcoreguidelines-pro-*,-cppcoreguidelines-owning-memory, + -cppcoreguidelines-no-malloc,-cppcoreguidelines-special-member-functions, + -cppcoreguidelines-avoid-goto, + google-*,-google-build-using-namespace,-google-explicit-constructor, + -google-readability-function-size,-google-readability-todo,-google-runtime-int, + -cppcoreguidelines-non-private-member-variables-in-classes, + -misc-non-private-member-variables-in-classes, + -readability-isolate-declaration, + -cppcoreguidelines-avoid-c-arrays, + -cppcoreguidelines-avoid-magic-numbers, + -readability-magic-numbers, + -cppcoreguidelines-macro-usage, + -cppcoreguidelines-narrowing-conversions, + -bugprone-narrowing-conversions, + -google-readability-avoid-underscore-in-googletest-name, + -cppcoreguidelines-init-variables, + -misc-no-recursion, + -cppcoreguidelines-avoid-non-const-global-variables, + -modernize-avoid-bind +HeaderFilterRegex: .* +CheckOptions: + - key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor + value: 1 + - key: modernize-make-unique.IncludeStyle + value: google + - key: modernize-make-shared.IncludeStyle + value: google + - key: readability-implicit-bool-conversion.AllowIntegerConditions + value: 1 + - key: readability-implicit-bool-conversion.AllowPointerConditions + value: 1 + - key: bugprone-dangling-handle.HandleClasses + value: std::basic_string_view; nonstd::sv_lite::basic_string_view +# Permit passing shard pointers by value for sink parameters + - key: performance-unnecessary-copy-initialization.AllowedTypes + value: shared_ptr + - key: performance-unnecessary-value-param.AllowedTypes + value: shared_ptr