Fix is_address_from(shared/local) - Bug #11028 - #11052
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds named CUDA compiler-version conditions and function-template helpers for affected address-space lowerings. Local checks use the workaround on CUDA 13.1. Shared checks use it on CUDA 12.9–13.1. Other supported versions retain native intrinsics. ChangesAddress-space lowering
Assessment against linked issues
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change provides the CUDA compiler compatibility workaround without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libcudacxx/include/cuda/__memory/address_space.h (1)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Repeat the exact preprocessor condition in each added
#elseand#endifcomment. The current comments use summaries such asNVCC/NVRTC 13.1andother NVCC/NVRTC versions, which can drift from the actual compiler gates. Based on learnings: every annotated#elseor#endifmust repeat the exact condition text from its corresponding#if, without inversion or paraphrasing.Also applies to: 78-78, 147-153, 159-159, 223-229
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 05ef8990-8a13-4ca3-b99f-2634db3d9626
📒 Files selected for processing (1)
libcudacxx/include/cuda/__memory/address_space.h
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
This comment has been minimized.
This comment has been minimized.
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
I believe the less invasive approach would be to selectively mark the function as either noinline or forceinline or potentially just drop the inline
That would reduce the impact on the call sites and we should probably check whether its used in other places
| || (_CCCL_CUDA_COMPILER(NVRTC, >=, 12, 9) && _CCCL_CUDA_COMPILER(NVRTC, <, 13, 2)) | ||
| // NVCC/NVRTC 12.9-13.1 pass typed pointers to the i8* __isShared intrinsic after inlining, which breaks the LLVM | ||
| // verifier. Preventing inlining preserves the required generic pointer type. | ||
| [[nodiscard]] _CCCL_DEVICE_API _CCCL_NOINLINE inline bool __is_shared_cuda_12_9_workaround(const void* __ptr) noexcept |
There was a problem hiding this comment.
This will not work, because many compiler balk at noinline inline Als here inline is applied multiple times, we should not use _CCCL_DEVICE_API, because that might include inline.
| [[nodiscard]] _CCCL_DEVICE_API _CCCL_NOINLINE inline bool __is_shared_cuda_12_9_workaround(const void* __ptr) noexcept | |
| [[nodiscard]] _CCCL_DEVICE _CCCL_NOINLINE bool __is_shared_cuda_12_9_workaround(const void* __ptr) noexcept |
There was a problem hiding this comment.
inline is for ODR, noinline is actually to avoid function inlining from the compiler. _CCCL_DEVICE_API looks safe to me. It cannot include inline
There was a problem hiding this comment.
an alternative could be using template<typename = void>, but looks that CI is passing with the current code
There was a problem hiding this comment.
yeah but I am pretty sure old GCC will then break in the nightlies. Please use the template hack
There was a problem hiding this comment.
Even GCC 5 is happy with inline noinline: https://godbolt.org/z/PsMsYb9o6. Compilers have had a good understanding that inline is predominantly an ODR control, not an, well, inlining marker for a rather long time.
griwes
left a comment
griwes
left a comment
There was a problem hiding this comment.
Reviewed throughout.
Nit: since you are re-spelling the same version checking conditions twice, could we have them drive the definition of a macro which you then check in both places? Would make it easier to visually verify that the conditions do, in fact, match, but I leave this up to you.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a68cd768-d33c-4614-833e-7ad2a4a308fe
📒 Files selected for processing (1)
libcudacxx/include/cuda/__memory/address_space.h
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 6h 57m: Pass: 100%/195 | Total: 8d 04h | Max: 4h 32m | Hits: 36%/3295551See results here. |
3 participants
Description
Fixes #11028
The PR provides a workaround for NVCC/NVRTC 12.9–13.1 where typed pointers (even with
void*) are passed to__isSharedand__isLocal. The fix consists of passing the argument to a non-inline function.