Skip to content

Commit da40932

Browse files
EJainDevCNugteren
EJainDev
and
CNugteren
authored
Clang tidy 1 (#644)
* Fixed a size_t to unsigned int warning in wrapper_cblas.hpp * Update CHANGELOG Co-authored-by: Cedric Nugteren <web@cedricnugteren.nl> * IWYU for src/tuning/* * Revert "IWYU for src/tuning/*" This reverts commit 39bf80f. * Removed unnecessary lambda capture of 'V' * Applied basic clang tidy checks regarding performance. * Formatting * Added clang tidy file to git * Fixed declaration/definition related issues. * Formatting * Removed unnecessary variable. * Added potential clang tidy linter * Changed clang tidy checks to be the ones in the file * Removed Github Action for clang-tidy for another PR. * Change GetArgument default value to be by value instead of reference * Fix argument parser function signature * Formatting * Added a lint for clang tidy * Added a check to fail if there are linter suggestions. * Added messages on failure of linting. * Updated how to pass compile_commands.son to linter * Changed copy to const reference * Fixed remaining clang tidy checks * Formatting * Reverted const std::shared_ptr<Program>& to const std::shared_ptr<Program> --------- Co-authored-by: Cedric Nugteren <web@cedricnugteren.nl>
1 parent d852d71 commit da40932

File tree

14 files changed

+99
-66
lines changed

14 files changed

+99
-66
lines changed

‎.clang-tidy‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Checks: "performance-*"

‎.github/workflows/lint.yml‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,39 @@
11
name: Lint
22

33
on:
4-
pull_request: {}
4+
push:
5+
branches: [ main, master ]
6+
pull_request:
7+
branches: [ main, master ]
58

69
jobs:
7-
ClangFormat:
10+
lint:
811
runs-on: ubuntu-latest
912
steps:
10-
- uses: actions/checkout@v4
11-
- name: clang-format lint
12-
uses: DoozyX/clang-format-lint-action@v0.18.2
13+
- name: Checkout code
14+
uses: actions/checkout@v3
15+
16+
- name: Install dependencies
17+
run: |
18+
sudo apt-get update
19+
sudo apt-get install -yq ninja-build ocl-icd-opencl-dev opencl-c-headers libopenblas-dev --no-install-recommends
20+
21+
- name: Configure CMake
22+
run: |
23+
export CC=${{ matrix.config.c_compiler }}
24+
export CXX=${{ matrix.config.cpp_compiler }}
25+
cmake -S . -B build -G Ninja -DTESTS=ON -DCLIENTS=ON -DSAMPLES=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
26+
27+
- name: Run linter
28+
uses: cpp-linter/cpp-linter-action@v2
29+
id: linter
30+
env:
31+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
1332
with:
14-
clangFormatVersion: 18
33+
tidy-checks: ''
34+
style: 'file'
35+
database: 'build'
36+
37+
- name: Fail fast?!
38+
if: steps.linter.outputs.checks-failed > 0
39+
run: exit 1

‎src/database/database.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ database::Parameters Database::Search(const std::string& this_kernel, const std:
221221
for (auto& db : this_database) {
222222
if ((db.kernel == this_kernel) && (db.precision == this_precision || db.precision == Precision::kAny)) {
223223
// Searches for the right vendor and device type, or selects the default if unavailable
224-
const auto parameters =
224+
auto parameters =
225225
SearchVendorAndType(this_vendor, this_type, this_device, this_architecture, db.vendors, db.parameter_names);
226226
if (parameters.size() != 0) {
227227
return parameters;
@@ -241,7 +241,11 @@ database::Parameters Database::SearchVendorAndType(const std::string& target_ven
241241
const std::vector<std::string>& parameter_names) const {
242242
for (auto& vendor : vendors) {
243243
if ((vendor.name == target_vendor) && (vendor.type == target_type)) {
244-
log_debug("Found architectures of vendor '" + target_vendor + "' and type '" + target_type + "'");
244+
log_debug(std::string("Found architectures of vendor '")
245+
.append(target_vendor)
246+
.append("' and type '")
247+
.append(target_type)
248+
.append("'"));
245249

246250
// Searches the architecture; if unavailable returns the vendor's default parameters
247251
auto parameters = SearchArchitecture(this_architecture, this_device, vendor.architectures, parameter_names);

‎src/kernel_preprocessor.cpp‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ bool HasOnlyDigits(const std::string& str) {
7070
// Simple unsigned integer math parser
7171
int ParseMath(const std::string& str) {
7272
// Handles brackets
73-
if (str.find(")") != std::string::npos) {
73+
if (str.find(')') != std::string::npos) {
7474
const auto split_close = split(str, ')');
7575
const auto split_end = split(split_close[0], '(');
7676
if (split_end.size() < 2) {
@@ -184,7 +184,7 @@ bool EvaluateCondition(std::string condition, const DefinesIntMap& defines, cons
184184
if (not_defined_pos != std::string::npos) {
185185
const auto contents = condition.substr(not_defined_pos + 9);
186186
const auto not_defined_split = split(contents, ')');
187-
const auto not_defined_val = not_defined_split[0];
187+
const auto& not_defined_val = not_defined_split[0];
188188
return (defines_string.find(not_defined_val) == defines_string.end());
189189
}
190190

@@ -193,7 +193,7 @@ bool EvaluateCondition(std::string condition, const DefinesIntMap& defines, cons
193193
if (defined_pos != std::string::npos) {
194194
const auto contents = condition.substr(defined_pos + 8);
195195
const auto defined_split = split(contents, ')');
196-
const auto defined_val = defined_split[0];
196+
const auto& defined_val = defined_split[0];
197197
return (defines_string.find(defined_val) != defines_string.end());
198198
}
199199

@@ -264,7 +264,7 @@ void ArrayToRegister(std::string& source_line, const DefinesIntMap& defines,
264264
if (right_square_split.size() < 1) {
265265
RaiseError(source_line, "Mis-formatted array declaration #B");
266266
}
267-
auto array_index_string = right_square_split[0];
267+
auto& array_index_string = right_square_split[0];
268268
const auto array_index = StringToDigit(array_index_string, source_line);
269269

270270
// Creates the new string
@@ -398,7 +398,7 @@ std::vector<std::string> PreprocessDefinesAndComments(const std::string& source,
398398
const auto define_pos = line.find("#define ");
399399
if (define_pos != std::string::npos) {
400400
const auto define = line.substr(define_pos + 8); // length of "#define "
401-
const auto value_pos = define.find(" ");
401+
const auto value_pos = define.find(' ');
402402
auto value = define.substr(value_pos + 1);
403403
const auto name = define.substr(0, value_pos);
404404
SubstituteDefines(defines_int, value);
@@ -526,7 +526,7 @@ std::vector<std::string> PreprocessUnrollLoops(const std::vector<std::string>& s
526526
if (array_name_split.size() < 2) {
527527
RaiseError(line, "Mis-formatted array declaration #2");
528528
}
529-
const auto array_name = array_name_split[array_name_split.size() - 1];
529+
const auto& array_name = array_name_split[array_name_split.size() - 1];
530530
arrays_to_registers[array_name] = array_size;
531531
// TODO: bracket count not used currently for scope checking
532532
continue;
@@ -535,6 +535,9 @@ std::vector<std::string> PreprocessUnrollLoops(const std::vector<std::string>& s
535535
// Regular line
536536
lines.emplace_back(line);
537537
}
538+
539+
(void)brackets; // Suppresses variable unused warning, to be used in TODO above
540+
538541
return lines;
539542
}
540543

@@ -580,8 +583,7 @@ std::vector<std::string> PreprocessUnrollLoops(const std::vector<std::string>& s
580583
}
581584

582585
// Retrieves loop information (and checks for assumptions)
583-
const auto variable_type = line_split[0];
584-
const auto variable_name = line_split[1];
586+
const auto& variable_name = line_split[1];
585587
if (variable_name != line_split[4]) {
586588
RaiseError(line, "Mis-formatted for-loop #2");
587589
}

‎src/routines/common.hpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ void PadCopyTransposeMatrix(Queue& queue, const Device& device, const Databases&
5151
const std::vector<Event>& waitForEvents, const size_t src_one, const size_t src_two,
5252
const size_t src_ld, const size_t src_offset, const Buffer<T>& src, const size_t dest_one,
5353
const size_t dest_two, const size_t dest_ld, const size_t dest_offset,
54-
const Buffer<T>& dest, const T alpha, const std::shared_ptr<Program> program,
54+
const Buffer<T>& dest, const T alpha, const std::shared_ptr<Program>& program,
5555
const bool do_pad, const bool do_transpose, const bool do_conjugate,
5656
const bool upper = false, const bool lower = false, const bool diagonal_imag_zero = false) {
5757
// Determines whether or not the fast-version could potentially be used
@@ -146,7 +146,7 @@ void PadCopyTransposeMatrixBatched(Queue& queue, const Device& device, const Dat
146146
const size_t src_ld, const Buffer<int>& src_offsets, const Buffer<T>& src,
147147
const size_t dest_one, const size_t dest_two, const size_t dest_ld,
148148
const Buffer<int>& dest_offsets, const Buffer<T>& dest,
149-
const std::shared_ptr<Program> program, const bool do_pad, const bool do_transpose,
149+
const std::shared_ptr<Program>& program, const bool do_pad, const bool do_transpose,
150150
const bool do_conjugate, const size_t batch_count) {
151151
// Determines the right kernel
152152
auto kernel_name = std::string{};
@@ -197,7 +197,7 @@ void PadCopyTransposeMatrixStridedBatched(Queue& queue, const Device& device, co
197197
const size_t src_stride, const Buffer<T>& src, const size_t dest_one,
198198
const size_t dest_two, const size_t dest_ld, const size_t dest_offset,
199199
const size_t dest_stride, const Buffer<T>& dest,
200-
const std::shared_ptr<Program> program, const bool do_pad,
200+
const std::shared_ptr<Program>& program, const bool do_pad,
201201
const bool do_transpose, const bool do_conjugate, const size_t batch_count) {
202202
// Determines the right kernel
203203
auto kernel_name = std::string{};

‎src/routines/level3/xherk.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ void Xherk<T, U>::DoHerk(const Layout layout, const Triangle triangle, const Tra
5252
const size_t a_ld, const U beta, const Buffer<T>& c_buffer, const size_t c_offset,
5353
const size_t c_ld) {
5454
const auto b_transpose = (a_transpose != Transpose::kNo) ? Transpose::kNo : Transpose::kYes;
55-
const auto b_buffer = a_buffer;
55+
const auto& b_buffer = a_buffer;
5656
const auto b_offset = a_offset;
5757
const auto b_ld = a_ld;
5858
const auto complex_alpha = T{alpha, static_cast<U>(0.0)};

‎src/routines/level3/xsyrk.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ void Xsyrk<T>::DoSyrk(const Layout layout, const Triangle triangle, const Transp
5252
const size_t a_ld, const T beta, const Buffer<T>& c_buffer, const size_t c_offset,
5353
const size_t c_ld) {
5454
const auto b_transpose = (a_transpose != Transpose::kNo) ? Transpose::kNo : Transpose::kYes;
55-
const auto b_buffer = a_buffer;
55+
const auto& b_buffer = a_buffer;
5656
const auto b_offset = a_offset;
5757
const auto b_ld = a_ld;
5858
SyrkAB(layout, triangle, a_transpose, b_transpose, n, k, alpha, a_buffer, a_offset, a_ld, b_buffer, b_offset, b_ld,

‎src/tuning/configurations.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ namespace clblast {
1919
// =================================================================================================
2020

2121
// Finds all configurations. It also applies the user-defined constraints within.
22-
std::vector<Configuration> SetConfigurations(const Device& device, const std::vector<Parameter> parameters,
22+
std::vector<Configuration> SetConfigurations(const Device& device, const std::vector<Parameter>& parameters,
2323
const std::vector<size_t>& local_size_base,
2424
const TransformVector& mul_local_config,
2525
const TransformVector& div_local_config, const Constraints& constraints,
@@ -35,7 +35,7 @@ std::vector<Configuration> SetConfigurations(const Device& device, const std::ve
3535
}
3636

3737
// Iterates recursively over all permutations of the user-defined parameters
38-
void PopulateConfigurations(const std::vector<Parameter>& parameters, const std::vector<size_t> local_size_base,
38+
void PopulateConfigurations(const std::vector<Parameter>& parameters, const std::vector<size_t>& local_size_base,
3939
const TransformVector& mul_local_config, const TransformVector& div_local_config,
4040
const size_t index, const Configuration& config, std::vector<Configuration>& configuration,
4141
const size_t local_mem_max, const Constraints& constraints,
@@ -64,7 +64,7 @@ void PopulateConfigurations(const std::vector<Parameter>& parameters, const std:
6464

6565
// Loops over all user-defined constraints to check whether or not the configuration is valid
6666
bool ValidConfiguration(const Configuration& config, const size_t local_mem_max, const Constraints& constraints,
67-
const LocalMemSizeInfo& local_mem_size_info, const std::vector<size_t> local_size_base,
67+
const LocalMemSizeInfo& local_mem_size_info, const std::vector<size_t>& local_size_base,
6868
const TransformVector& mul_local_config, const TransformVector& div_local_config,
6969
const std::vector<size_t>& max_work_item_sizes, const size_t max_work_group_size) {
7070
// Iterates over all constraints
@@ -112,7 +112,7 @@ bool ValidConfiguration(const Configuration& config, const size_t local_mem_max,
112112
}
113113

114114
// Multiplies and/or dividers a thread configuration (local/global)
115-
std::vector<size_t> SetThreadConfiguration(const Configuration& config, const std::vector<size_t> base,
115+
std::vector<size_t> SetThreadConfiguration(const Configuration& config, const std::vector<size_t>& base,
116116
const TransformVector& mul_config, const TransformVector& div_config) {
117117
auto result = base;
118118
for (const auto& multipliers : mul_config) {

‎src/tuning/configurations.hpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ struct LocalMemSizeInfo {
4747

4848
// Initializes an empty configuration (vector of name/value pairs) and kicks-off the recursive
4949
// function to find all configurations. It also applies the user-defined constraints within.
50-
std::vector<Configuration> SetConfigurations(const Device& device, const std::vector<Parameter> parameters,
50+
std::vector<Configuration> SetConfigurations(const Device& device, const std::vector<Parameter>& parameters,
5151
const std::vector<size_t>& local_size_base,
5252
const TransformVector& mul_local_config,
5353
const TransformVector& div_local_config, const Constraints& constraints,
@@ -57,7 +57,7 @@ std::vector<Configuration> SetConfigurations(const Device& device, const std::ve
5757
// multiple chains, in which each chain selects a unique combination of values for all parameters.
5858
// At the end of each chain (when all parameters are considered), the function stores the result
5959
// into the configuration list.
60-
void PopulateConfigurations(const std::vector<Parameter>& parameters, const std::vector<size_t> local_size_base,
60+
void PopulateConfigurations(const std::vector<Parameter>& parameters, const std::vector<size_t>& local_size_base,
6161
const TransformVector& mul_local_config, const TransformVector& div_local_config,
6262
const size_t index, const Configuration& config, std::vector<Configuration>& configuration,
6363
const size_t local_mem_max, const Constraints& constraints,
@@ -69,12 +69,12 @@ void PopulateConfigurations(const std::vector<Parameter>& parameters, const std:
6969
// not been met. Constraints consist of a user-defined function and a list of parameter names, which
7070
// are replaced by parameter values in this function.
7171
bool ValidConfiguration(const Configuration& config, const size_t local_mem_max, const Constraints& constraints,
72-
const LocalMemSizeInfo& local_mem_size_info, const std::vector<size_t> local_size_base,
72+
const LocalMemSizeInfo& local_mem_size_info, const std::vector<size_t>& local_size_base,
7373
const TransformVector& mul_local_config, const TransformVector& div_local_config,
7474
const std::vector<size_t>& max_work_item_sizes, const size_t max_work_group_size);
7575

7676
// Processes multipliers and dividers to obtain the final thread configuration
77-
std::vector<size_t> SetThreadConfiguration(const Configuration& config, const std::vector<size_t> base,
77+
std::vector<size_t> SetThreadConfiguration(const Configuration& config, const std::vector<size_t>& base,
7878
const TransformVector& mul_config, const TransformVector& div_config);
7979

8080
// =================================================================================================

‎src/tuning/kernels/xgemv.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,10 @@ std::vector<Constraint> XgemvSetConstraints(const int V) {
111111
template <typename T>
112112
LocalMemSizeInfo XgemvComputeLocalMemSize(const int V) {
113113
if (V == 1 || V == 2) {
114-
return {[V](std::vector<size_t> v) -> size_t { return GetBytes(PrecisionValue<T>()) * v[0]; },
114+
return {[](std::vector<size_t> v) -> size_t { return GetBytes(PrecisionValue<T>()) * v[0]; },
115115
{"WGS" + std::to_string(V)}};
116116
}
117-
return {[V](std::vector<size_t> v) -> size_t { return GetBytes(PrecisionValue<T>()) * (v[0] + v[1] * v[2]); },
117+
return {[](std::vector<size_t> v) -> size_t { return GetBytes(PrecisionValue<T>()) * (v[0] + v[1] * v[2]); },
118118
{"WGS3", "WPT3", "WGS3"}};
119119
}
120120

0 commit comments

Comments
 (0)