[BUG] we should not use the __ldcg, __ldcv, etc intrinsics?

[teju85]

I have put in a question mark in the issue as I wasn't sure what's the right approach here! Let me describe the current issue.

There are a couple of places where we are using these intrinsics in RAFT:

1. Usage of __ldcv in linewise_op.cuh
2. Usage of __ldcg in reduce_rows_by_key.cuh

Unfortunately, clang doesn't support(?) these intrinsics, unlike our CTK. Because of this, clang-tidy fails to compile such source files, thereby causing trouble in enabling tidy checks in our repo's. Thus, we currently have an explicit definition of these functions in a separate header file which our run-clang-tidy.py refers to.

There are a few possible ways to solve this issue:

1. Send a PR to clang to add these intrinsics support.
2. Continue to maintain the above mentioned intrinsics header file just for clang-tidy purposes. We'll have to continue to maintain and keep this updated and depending on how other RAPIDS projects adapt clang-tidy checks, they too might have to copy this around!
3. Do not use these intrinsics in our source files and instead implement explicit definitions of such in our device_loads_stores.cuh.

I honestly don't know what's the right approach, and hence would like thoughts from others in this regard. This is NOT a priority item as atleast for now, there's a workaround.

Cc += @MatthiasKohl, @dantegd and @cjnolet

[MatthiasKohl]

I replied on a thread within cugraph-ops, so I'll add my thoughts here:

1. Sending PR to clang: this is likely to take quite a while and will only make it into clang 15 at the very earliest. Currently, RAPIDS uses clang 11 tools, so we'd have to update a few things.
2. Keeping additional header only for clang: In any case, I think that this has the advantage of being able to adapt to other things clang might miss from CUDA, especially newer features. Clang often lags behind a bit in terms of newer CUDA features, so we might simply use this header as a kind of "staging" thing for anything that Clang does not yet support
3. Add additional intrinsics to device_loads_stores.cuh: One thing to consider here is that this is different to the other loads/stores utilities in that file. If we want to be compatible with clang, we would have to force the usage of these utility functions, rather than just add them as a should-have/nice-to-have. Right now, anyone can write auto x = address[0]; instead of float x; raft::ldg(x, address); and the first version is probably acceptable. With cache hints, the first version is auto x = __ldcv(address); and the second float x; raft::ldcv(x, address); but the first version must not be used.

This is basically why I went with option 2 for loads/stores with cache hints, but I agree that the "nicer" approach would be option 3 and the nicest option 1. In fact, I first proposed something similar to option 3, but @achirkin wasn't so sure about it (granted, what I proposed at the time simply removed cache hints altogether for clang, while we could use the appropriate PTX instead).
I do understand the concern about forcing the usage of the utility functions though, and not being able to use the CUDA functions as in the programming guide.

[achirkin]

I'd be fine with option 3, if we could have the interface of raft::xxx versions matching exactly the __xxx intrinsics. e.g. I'm happy with using raft::shfl_xxx functions in place of __shfl_xx, since it's a no-brainer substitution.

[github-actions]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. 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 labeled inactive-90d if there is no activity in the next 60 days.

[github-actions]

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. 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.