Skip to content
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.

Sign up for GitHub

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

ci: clang-tidy: speed-up clang-tidy #2758

Merged
merged 61 commits into from
Mar 11, 2025
Merged

ci: clang-tidy: speed-up clang-tidy #2758

Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
68d1925
ci: clang-tidy: v1 of speed up
hmaciak Feb 26, 2025
ede13f2
x64: test ci
hmaciak Feb 26, 2025
5759354
ci: clang-tidy: check headers
hmaciak Feb 26, 2025
d8e9d77
ci: clang-tidy: check headers command
hmaciak Feb 26, 2025
762295d
ci: clang-tidy: debug
hmaciak Feb 26, 2025
111ddf1
ci: clang-tidy: no main merge
hmaciak Feb 26, 2025
831a7e2
ci: clang-tidy: refactor
hmaciak Feb 26, 2025
5f11485
ci: automation: full scan headers
hmaciak Feb 26, 2025
38805df
ci: clang-tidy: list build files
hmaciak Feb 27, 2025
686f8b8
ci: clang-tidy: debug
hmaciak Feb 27, 2025
d912a5b
ci: clang-tidy: check files
hmaciak Feb 27, 2025
9d0ab18
ci: clang-tidy: check comp commands
hmaciak Feb 27, 2025
2fa2839
ci: clang-tidy: check comp file
hmaciak Feb 27, 2025
e3a065c
ci: clang-tidy: files scope
hmaciak Feb 27, 2025
412085c
ci: clang-tidy: redundant done
hmaciak Feb 27, 2025
3f24538
aarch64: test changes
hmaciak Feb 27, 2025
372e8f5
ci: clang-tidy: header filter
hmaciak Feb 27, 2025
5e1eefd
ci: clang-tidy: remove done
hmaciak Feb 27, 2025
a07a413
ci: clang-tidy: file...
hmaciak Feb 27, 2025
856d4c5
ci: clang-tidy: refactor
hmaciak Feb 27, 2025
f92024d
ci: clang-tidy: no brackets
hmaciak Feb 27, 2025
1c095c5
ci: clang-tidy: regex for include
hmaciak Feb 27, 2025
8b9aa69
include: test changes
hmaciak Feb 27, 2025
17dc79b
ci: clang-tidy: debug regex
hmaciak Feb 27, 2025
01a1969
ci: clang-tidy: debug if
hmaciak Feb 27, 2025
0a8577a
ci: clang-tidy: fix command
hmaciak Feb 27, 2025
6b5c370
ci: clang-tidy: fix quote...
hmaciak Feb 27, 2025
9fa4861
ci: clang-tidy: debug
hmaciak Feb 27, 2025
15041b4
ci: clang-tidy: regex fix
hmaciak Feb 27, 2025
c09a5a7
ci: clang-tidy: regex
hmaciak Feb 27, 2025
2cebba2
ci: clang-tidy: loop debug
hmaciak Feb 27, 2025
0d562c9
ci: clang-tidy: loop debug..
hmaciak Feb 27, 2025
ee2fe63
ci: clang-tidy: quotes
hmaciak Mar 3, 2025
06e6eca
ci: clang-tidy: debug header check
hmaciak Mar 3, 2025
e4d61f1
ci: clang-tidy: debug grep hardcode
hmaciak Mar 3, 2025
1b5e910
ci: clang-tidy: change if statement
hmaciak Mar 3, 2025
bdb1c9b
aarch64: test hpp changes
hmaciak Mar 3, 2025
233d96b
aarch64: test cont on err
hmaciak Mar 3, 2025
8691ca1
ci: clang-tidy: no header changes
hmaciak Mar 3, 2025
2e1599c
ci: clang-tidy: no changes
hmaciak Mar 3, 2025
61a2973
ci: clang-tidy: no changes in aarch
hmaciak Mar 3, 2025
9704533
ci: clang-tidy: prevent undefined behaviour
hmaciak Mar 3, 2025
15b7bc8
ci: clang-tidy: garbage enter
hmaciak Mar 3, 2025
09c76b5
ci: clang-tidy: eol
hmaciak Mar 4, 2025
40cde4f
ci: clang-tidy: eol
hmaciak Mar 4, 2025
3909f95
ci: clang-tidy: eol
hmaciak Mar 4, 2025
8acd1bd
ci: clang-tidy: e at the end
hmaciak Mar 4, 2025
b11a0d5
ci: clang-tidy: eol
hmaciak Mar 4, 2025
d82f70e
ci: clang-tidy: test removed options
hmaciak Mar 6, 2025
25749ca
ci: clang-tidy: check file content
hmaciak Mar 6, 2025
bc3590a
ci: clang-tidy: test no header filter
hmaciak Mar 6, 2025
f02d978
ci: clang-tidy: no tests
hmaciak Mar 6, 2025
fdeb823
ci: clang: diagnostics info
hmaciak Mar 7, 2025
7fd7f9b
ci: clang: check included headers
hmaciak Mar 7, 2025
4ad9720
ci: clang: test check
hmaciak Mar 7, 2025
313ca65
ci: clang: mark as red
hmaciak Mar 7, 2025
bc55e45
ci: clang: no test
hmaciak Mar 7, 2025
42f7ddf
ci: clang: minor improvements
hmaciak Mar 10, 2025
1609c06
ci: clang: test
hmaciak Mar 10, 2025
1f0ca7a
ci: clang: more tests
hmaciak Mar 10, 2025
3a9599d
ci: clang: no tests
hmaciak Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/automation/x64/build_linters.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ if [[ "$ONEDNN_ACTION" == "configure" ]]; then
-DDNNL_EXPERIMENTAL_PROFILING=ON \
-DDNNL_EXPERIMENTAL_UKERNEL=ON \
-DONEDNN_EXPERIMENTAL_LOGGING=ON \
-DDNNL_USE_CLANG_TIDY=CHECK \
-DDNNL_CPU_RUNTIME=OMP \
-DDNNL_GPU_RUNTIME=OCL \
-DDNNL_WERROR=ON \
-DDNNL_BUILD_FOR_CI=ON
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON
set +x
elif [[ "$GITHUB_JOB" == "pr-format-tags" ]]; then
set -x
Expand Down
33 changes: 27 additions & 6 deletions .github/workflows/clang-tidy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ jobs:
steps:
- name: Checkout oneDNN
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ github.event.pull_request.head.ref }}
fetch-depth: 0

- name: Install clang
run: |
Expand All @@ -48,10 +51,28 @@ jobs:
env:
ONEDNN_ACTION: configure

- name: Build oneDNN
- name: Check source files
run: |
.github/automation/x64/build_linters.sh 2>&1 | tee build.log
grep -i "warning:" build.log | sort -u
grep -q -i "warning:" build.log && exit 1 || true
env:
ONEDNN_ACTION: build
touch source-check.log
for file in $(git diff --name-only ${{ github.event.pull_request.base.sha }} | grep -E '\.(c|cpp)$');
do
if grep -q "$file" "build/compile_commands.json"; then
clang-tidy -p build $file 2>&1 | tee -a source-check.log
fi
done
grep -i "warning:" source-check.log | sort -u
grep -q -i "warning:" source-check.log && exit 1 || true

- name: Check header files
if: always()
run: |
touch headers-check.log
for file in $(git diff --name-only ${{ github.event.pull_request.base.sha }} | grep -E '\.(h|hpp)$');
do
header_dir=$(dirname "$file")
if [[ "$header_dir" =~ ^include ]] || grep -q "$header_dir" "build/compile_commands.json"; then
Copy link
Contributor

@vpirogov vpirogov Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain this logic? I'm primarily interested in why we need to handle headers separately and what rules you apply to pick the relevant ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is compile_commands.json does not contain header files. It only contains c/cpp files which are used in full build. If we want to ensure somehow that we check the "right" header files we need to check full path without file name. For example:

headers:

  1. src/cpu/file.hpp -> path is src/cpu -> in compile_commands.json for sure some cpp is in this directory, so we should check this header file.
  2. src/cpu/x64/file.hpp -> path is src/cpu/x64 -> in compile_commands.json for sure some cpp is in this directory, so we should check this header file.
  3. src/cpu/aarch64/file.hpp -> path is src/cpu/aarch64 -> in compile_commands.json for sure there is no cpp in this directory, so we do not check this header file.
  4. The only exception are files in include, where for example include/oneapi dir is not present in compile_commands.json.

c/cpp:

  1. src/cpu/file.cpp -> in compile_commands.json this cpp file exists, so we should check this file.
  2. src/cpu/x64/file.cpp -> in compile_commands.json this cpp file exists, so we should check this file.
  3. src/cpu/aarch64/file.cpp -> in compile_commands.json for sure there is no such cpp file, so we do not check it.

This is the main difference between checking c/cpp files and headers. As you can see headers are more tricky because we don't have the list of exact files. Also it seems to provide more visibility when we check c/cpp files only and headers separately, but this is only my opinion :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks. Documentation for clang-tidy indicates that it is expected to run on C/C++ files only. Header files included by sources are analyzed as well if they match the mask defined by HeaderFilterRegex in .clang-tidy or '--header-filter' option. So it looks like we should not be invoking clang-tidy on header files at all.

With that we have a question of how to make sure that the changed headers are analyzed. In ideal world this should be done by finding a source that includes particular header and running clang-tidy on it, though this may not be practical. For starters we can assume that header is not touched in isolation and see how it goes.

Copy link
Contributor

@vpirogov vpirogov Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the options for this part are:

  1. Remove it altogether.
  2. Use the same logic as for source check, but without the headers filter. In that case this check should only print diagnostics, but not fail the job.

Once we are done with addressing hits in headers (2) will become the default behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon experimenting I'm leaning towards option (2).

Copy link
Contributor

@rjoursler rjoursler Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For starters we can assume that header is not touched in isolation and see how it goes.

This doesn't seem like a good assumption to me, in my experience this happens relatively frequently, especially for small changes. If we excluded header checks here, we will just hit issues when we later run a full clang-tidy suite.

With that we have a question of how to make sure that the changed headers are analyzed. In ideal world this should be done by finding a source that includes particular header and running clang-tidy on it

Ideally, this dependency management would be handled by the build system, but last I was looking into it, there are not tools for this. Although there are a few wrappers scripts floating around that try to cache clang-tidy calls. (see https://gitlab.kitware.com/cmake/cmake/-/issues/21092 for some discussions).

indicates that it is expected to run on C/C++ files only

What if we just generate some psuedo-cpp targets which just include the target header via CMake that we can run clang-tidy against for this check? The main thing that clang-tidy needs to know is how to compile the file, which for headers, implies knowing what preprocessor defines are set. Since most of our #include are solely a function of the defines set by the compiler, this will get us 99% of the way to being correct without worrying about dependencies. One other minor advantage here is we can avoid duplicated logging when modified cpp files reference the same header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just generate some psuedo-cpp targets which just include the target header via CMake that we can run clang-tidy against for this check?

That's an interesting idea to explore. For headers clang-tidy need a bit more then defines alone:

  • Include paths to find dependencies
  • Some compile options, like -std that affect behavior
    Effectively this problem boils down to finding a C++ file in compile_commands.json that include particular header and running clang-tidy on it.

I would suggest to not hold this PR though, as it offers material improvement over what we have now.

clang-tidy -p build --header-filter='' $file | tee -a headers-check.log
fi
done
grep -i "warning:" headers-check.log | sort -u
grep -q -i "warning:" headers-check.log && exit 1 || true
6 changes: 3 additions & 3 deletions .github/workflows/pr-linter.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ concurrency:

jobs:
pr-commits:
name: "Commit messages"
name: Commit messages
runs-on: ubuntu-latest
steps:
- name: Checkout
Expand All @@ -43,7 +43,7 @@ jobs:
run: python3 ./.github/automation/commit-msg-check.py "${{ github.event.pull_request.head.sha }}" "${{ github.event.pull_request.base.sha }}"

pr-clang-format:
name: "Clang-Format"
name: Clang-Format
runs-on: ubuntu-22.04
steps:
- name: Checkout
Expand Down Expand Up @@ -90,7 +90,7 @@ jobs:
git diff | grep . && exit 1 || true

pr-status:
name: "Formatting"
name: Formatting
runs-on: ubuntu-latest
needs: [ pr-commits, pr-clang-format, pr-format-tags ]
steps:
Expand Down