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.

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

[BUG] Fix clang-tidy errors #84

Closed
teju85 opened this issue Oct 21, 2020 · 3 comments · Fixed by #424
Closed

[BUG] Fix clang-tidy errors #84

teju85 opened this issue Oct 21, 2020 · 3 comments · Fixed by #424
Labels
bug Something isn't working inactive-30d inactive-90d

Comments

@teju85
Copy link
Member

teju85 commented Oct 21, 2020

Describe the bug
In the past we had to disable parsing of .cu files in run-clang-tidy.py due to difficulties in trying to get them working properly. So, we never tidied our cuda source files!

Steps/Code to reproduce bug
Today, if we path this script with the following change:

diff --git a/cpp/scripts/run-clang-tidy.py b/cpp/scripts/run-clang-tidy.py
index 23260d2..fdb0abc 100644
--- a/cpp/scripts/run-clang-tidy.py
+++ b/cpp/scripts/run-clang-tidy.py
@@ -36,7 +36,7 @@ def parse_args():
                            help="Path to cmake-generated compilation database")
     argparser.add_argument("-exe", type=str, default="clang-tidy",
                            help="Path to clang-tidy exe")
-    argparser.add_argument("-ignore", type=str, default="[.]cu$",
+    argparser.add_argument("-ignore", type=str, default=None,
                            help="Regex used to ignore files from checking")
     argparser.add_argument("-select", type=str, default=None,
                            help="Regex used to select files for checking")
@@ -123,6 +123,10 @@ def get_tidy_args(cmd, exe):
         # replace nvcc's "-gencode ..." with clang's "--cuda-gpu-arch ..."
         archs = get_gpu_archs(command)
         command.extend(archs)
+        # clang-tooling 8.0.1 doesn't support linking with greater sm_70 archs
+        # so, we need to disable libdevice linkage, until we are in a position
+        # to upgrade clang-tooling
+        command.extend(["-nocudalib", "--no-cuda-version-check"])
         while True:
             loc = remove_item_plus_one(command, "-gencode")
             if loc < 0:

and then run the script, we get a ton of errors which we never caught before (and thus never fixed). Full list of all the errors in here: run.log

Expected behavior
tidy check should pass on all source files, irrespective of device or host code.

@teju85 teju85 added the bug Something isn't working label Oct 21, 2020
@github-actions
Copy link

This issue has been marked rotten due to no recent activity in the past 90d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be marked rotten if there is no activity in the next 60d.

@MatthiasKohl
Copy link
Contributor

I'd like to come back to this issue and fix clang compilation in raft.
There are several things that I can see after attempting to compile all raft test codes with clang:

  1. A few minor things due to the CUDA/C __host__ / __device__ overloads which nvcc and clang++ treat differently (mainly functions such as min / fmin etc.)
  2. The fact that raft has a dependency on cub which is too old to be compiled by clang: we should update this to >= 1.14
  3. A few issues with static const vs static constexpr : should be easy to handle since raft is C++ >= 11
  4. There seem to be some issues with template parameters in KNN which I still have to figure out

@rapids-bot rapids-bot bot closed this as completed in #424 Feb 16, 2022
rapids-bot bot pushed a commit that referenced this issue Feb 16, 2022
This makes RAFT sources compilable with clang.
It fixes some fragile code (using `static const` instead of `static constexpr` or `%laneid` in PTX relying on quirks in nvcc which make this happen).

RAFT is still not compilable with clang entirely though due to the dependencies:
1. cub has this issue before 1.14: NVIDIA/cub#335
2. libcudacxx has issues with atomic, which should be fixed in >= 1.7.0-ea (wasn't able to verify this yet)
3. libcudacxx has issues with variadic CUDA functions, which is apparently fixed by passing `-Xclang -fcuda-allow-variadic-functions` to clang (wasn't able to verify this yet)
3. cooperative_groups from CUDA does not work with clang 11.0 / 11.1 but only with >= 13

EDIT: this is necessary to close #84

Authors:
  - Matt Joux (https://github.com/MatthiasKohl)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Artem M. Chirkin (https://github.com/achirkin)

URL: #424
dantegd pushed a commit to dantegd/raft that referenced this issue Jul 23, 2024
The rust API docs aren't being generated on docs.rapids.ai/cuvs . While the `build.sh docs` script was including the rust api docs, the `ci/build_docs.sh` wasn't. Fix.

Authors:
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Jake Awe (https://github.com/AyodeAwe)

URL: rapidsai/cuvs#84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working inactive-30d inactive-90d
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants