Skip to content

[UR][CUDA] Avoid unnecessary calls to cuFuncSetAttribute #16928

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

Merged
merged 2 commits into from
Feb 27, 2025

Conversation

rafbiels
Copy link
Contributor

@rafbiels rafbiels commented Feb 7, 2025

Calling cuFuncSetAttribute to set CU_FUNC_ATTRIBUTE_MAX_DYNAMIC_SHARED_SIZE_BYTES is required to launch kernels using more than 48 kB of local memory[1] (CUDA dynamic shared memory). Without this, cuLaunchKernel fails with CUDA_ERROR_INVALID_VALUE. However, calling cuFuncSetAttribute introduces synchronisation in the CUDA runtime which blocks its execution until all H2D/D2H memory copies are finished (don't know why), therefore effectively blocking kernel launches from overlapping with memory copies. This introduces significant performance degradation in some workflows, specifically in applications launching overlapping memory copies and kernels from multiple host threads into multiple CUDA streams to the same GPU.

Avoid the CUDA runtime synchronisation causing poor performance by removing the cuFuncSetAttribute call unless it's strictly necessary. Call it only when a specific carveout is requested by user (using env variables) or when the kernel launch would fail without it (local memory size >48kB). Good performance is recovered for default settings with kernels using little or no local memory.

No performance effects were observed for kernel execution time after removing the attribute across a wide range of tested kernels using various amounts of local memory.

[1] Related to the 48 kB static shared memory limit, see the footnote for "Maximum amount of shared memory per thread block" in https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#features-and-technical-specifications-technical-specifications-per-compute-capability

Calling cuFuncSetAttribute to set
CU_FUNC_ATTRIBUTE_MAX_DYNAMIC_SHARED_SIZE_BYTES is required to
launch kernels using more than 48 kB of local memory[1] (CUDA
dynamic shared memory). Without this, cuLaunchKernel fails with
CUDA_ERROR_INVALID_VALUE. However, calling cuFuncSetAttribute
introduces synchronisation in the CUDA runtime which blocks its
execution until all H2D/D2H memory copies are finished (don't know
why), therefore effectively blocking kernel launches from
overlapping with memory copies. This introduces significant
performance degradation in some workflows, specifically in
applications launching overlapping memory copies and kernels from
multiple host threads into multiple CUDA streams to the same GPU.

Avoid the CUDA runtime synchronisation causing poor performance by
removing the cuFuncSetAttribute call unless it's strictly
necessary. Call it only when a specific carveout is requested by
user (using env variables) or when the kernel launch would fail
without it (local memory size >48kB). Good performance is recovered
for default settings with kernels using little or no local memory.

No performance effects were observed for kernel execution time
after removing the attribute across a wide range of tested kernels
using various amounts of local memory.

[1] Related to the 48 kB static shared memory limit, see the
footnote for "Maximum amount of shared memory per thread block" in
https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#features-and-technical-specifications-technical-specifications-per-compute-capability
@rafbiels rafbiels force-pushed the cuda-avoid-cuFuncSetAttribute branch from e682c9b to 6429ab6 Compare February 18, 2025 19:59
@rafbiels rafbiels requested a review from a team as a code owner February 18, 2025 19:59
@rafbiels rafbiels requested a review from Seanst98 February 18, 2025 19:59
@rafbiels
Copy link
Contributor Author

@frasercrmck @Seanst98, I rebased this PR following the UR move. This is now making the same changes as the already-approved UR PR oneapi-src/unified-runtime#2678

@rafbiels
Copy link
Contributor Author

friendly ping @intel/llvm-reviewers-cuda 👋

Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

Sorry, missed this

@rafbiels
Copy link
Contributor Author

Thank you, reviewers! @intel/llvm-gatekeepers this is ready to be merged

@uditagarwal97 uditagarwal97 merged commit 35fba19 into intel:sycl Feb 27, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants