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

Fix issues that came up with building cuDF with main #1643

Merged
merged 9 commits into from
May 6, 2024

Conversation

miscco
Copy link
Collaborator

@miscco miscco commented Apr 17, 2024

We need cuda_runtime.hfor the C++ cudaMallocHost and not cuda_runtime_api.h

@miscco miscco requested review from a team as code owners April 17, 2024 06:46
@miscco miscco added libcu++ For all items related to libcu++ bug Something isn't working right. labels Apr 17, 2024
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from 0330422 to aebae6f Compare April 17, 2024 06:56
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from aebae6f to efc14e7 Compare April 17, 2024 16:03
@miscco miscco requested review from a team as code owners April 17, 2024 16:03
@miscco miscco requested a review from gevtushenko April 17, 2024 16:03
@miscco miscco changed the title Fic incorrect include in cuda_pinned_memory_resource.h Fix issues that came up with building cuDF with main Apr 17, 2024
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from efc14e7 to a94885d Compare April 17, 2024 16:05
cub/cub/util_device.cuh Show resolved Hide resolved
thrust/thrust/system/cuda/detail/copy_if.h Outdated Show resolved Hide resolved
cub/cub/util_vsmem.cuh Outdated Show resolved Hide resolved
cub/cub/util_vsmem.cuh Outdated Show resolved Hide resolved
cub/cub/util_vsmem.cuh Outdated Show resolved Hide resolved
cub/cub/util_device.cuh Outdated Show resolved Hide resolved
cub/cub/util_device.cuh Show resolved Hide resolved
thrust/thrust/system/cuda/detail/copy_if.h Outdated Show resolved Hide resolved
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch 3 times, most recently from bbb4c49 to 3814b65 Compare April 18, 2024 07:00
thrust/thrust/optional.h Outdated Show resolved Hide resolved
@jrhemstad jrhemstad linked an issue Apr 24, 2024 that may be closed by this pull request
1 task
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from f6bf8d5 to ec4e698 Compare April 25, 2024 13:04
libcudacxx/include/cuda/std/__utility/pair.h Show resolved Hide resolved
thrust/thrust/tuple.h Outdated Show resolved Hide resolved
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch 2 times, most recently from 9226cc3 to 31a334a Compare April 30, 2024 17:11
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from be5ddf7 to bc342df Compare May 2, 2024 12:35
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from bc342df to 3cc958f Compare May 3, 2024 08:42
@miscco miscco force-pushed the fix_cuda_pinned_memory_include branch from 3cc958f to fe3a98b Compare May 3, 2024 08:50
@miscco
Copy link
Collaborator Author

miscco commented May 3, 2024

download

libcudacxx/include/cuda/functional Show resolved Hide resolved
thrust/thrust/tuple.h Outdated Show resolved Hide resolved
thrust/thrust/tuple.h Outdated Show resolved Hide resolved
thrust/thrust/tuple.h Outdated Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I don't know the full context for all of these changes but they look fine from what I know. Thanks!

thrust/thrust/tuple.h Outdated Show resolved Hide resolved
Comment on lines +69 to +71
// The maximum amount of static shared memory available per thread block
// Note that in contrast to dynamic shared memory, static shared memory is still limited to 48 KB
static constexpr std::size_t max_smem_per_block = 48 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know that, interesting!
Suggestion: you may want to add a pointer to the documentation confirming that. I found this on the web: https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#shared-memory-7-x
Question: is there a way (and if there was should) we could statically assert that this is still the case?

cub/cub/util_vsmem.cuh Show resolved Hide resolved
cub/cub/util_vsmem.cuh Show resolved Hide resolved
cub/cub/util_vsmem.cuh Show resolved Hide resolved
cub/cub/util_vsmem.cuh Show resolved Hide resolved
thrust/thrust/pair.h Show resolved Hide resolved
thrust/thrust/tuple.h Outdated Show resolved Hide resolved
thrust/thrust/tuple.h Show resolved Hide resolved
thrust/thrust/tuple.h Show resolved Hide resolved
thrust/thrust/tuple.h Show resolved Hide resolved
Comment on lines 75 to -77
template <class T, class U>
using pair = ::cuda::std::pair<T, U>;

Copy link
Contributor

Choose a reason for hiding this comment

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

I just fell over this again because of a doxygen issue and now wonder, why didn't you just pull pair into the thrust namespace without creating an alias? Like:

using _CUDA_VSTD::pair;

This works with CTAD and passes the unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right. libcu++ For all items related to libcu++
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: CTAD is broken for thrust::pair
7 participants