Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

cuda/detail/malloc_and_free doesn't include thrust/system/cuda/memory.h #990

Conversation

robertmaynard
Copy link
Collaborator

The comments inside cuda/detail/malloc_and_free.h state that
they don't want to include thrust/system/cuda/memory.h as it
is heavy-weight. It was inadvertently doing so through thrust/memory.h,
and so I have correct this oversight.

@robertmaynard robertmaynard force-pushed the correct_over_includes_in_cuda_malloc_and_free branch from fb874ca to f4f612d Compare May 30, 2019 17:43
@robertmaynard
Copy link
Collaborator Author

@brycelelbach I don't see your review.

As for your concern about calling std::free and std::malloc directly. What is the best approach to resolving this. Is including #include <thrust/detail/malloc_and_free.h> and calling thrust::malloc/free okay or is the insufficient for ADL?

Maybe the better question is can thrust::malloc(thrust::seq, ptr) invoke a user defined function or will it always call the version defined in thrust/system/detail/sequential?

@robertmaynard robertmaynard force-pushed the correct_over_includes_in_cuda_malloc_and_free branch from f4f612d to eb18e47 Compare August 7, 2019 12:23
@brycelelbach
Copy link
Collaborator

@griwes can you take a look at this?

The comments inside cuda/detail/malloc_and_free.h state that
they don't want to include thrust/system/cuda/memory.h as it
is heavy-weight. It was inadvertently doing so through thrust/memory.h,
so I have corrected this oversight.
@alliepiper alliepiper force-pushed the correct_over_includes_in_cuda_malloc_and_free branch from eb18e47 to 006d700 Compare May 27, 2020 22:39
@alliepiper
Copy link
Collaborator

Manually rebased on master (the only conflict was a whitespace change).

@alliepiper alliepiper added the testing: gpuCI passed Passed gpuCI testing. label May 28, 2020
@alliepiper
Copy link
Collaborator

Builds and tests pass locally:

Building thrust.cpp.cuda.cpp11...  ExitCode: 0
Building thrust.cpp.cuda.cpp14...  ExitCode: 0
Building thrust.cpp.cuda.cpp17...  ExitCode: 0
Testing thrust.cpp.cuda.cpp11...   ExitCode: 0   | 100% Pass | 0/273 Fail
Testing thrust.cpp.cuda.cpp14...   ExitCode: 0   | 100% Pass | 0/273 Fail
Testing thrust.cpp.cuda.cpp17...   ExitCode: 0   | 100% Pass | 0/273 Fail

Internal CI started in changelist 28461484.

@alliepiper alliepiper added the testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). label May 28, 2020
@alliepiper
Copy link
Collaborator

As for your concern about calling std::free and std::malloc directly. What is the best approach to resolving this. Is including #include <thrust/detail/malloc_and_free.h> and calling thrust::malloc/free okay or is the insufficient for ADL?

Maybe the better question is can thrust::malloc(thrust::seq, ptr) invoke a user defined function or will it always call the version defined in thrust/system/detail/sequential?

@robertmaynard I'm guessing you've figured something out by now, but if you still have a question about this, please open up a new issue.

This patch looks fine to me, it just reduces the number of includes to match a documented concern. Unless I hear otherwise I'll merge this after the tests finish.

@robertmaynard
Copy link
Collaborator Author

No concerns, thanks for fixing these issues

@alliepiper alliepiper added testing: internal ci passed Passed internal NVIDIA CI (DVS). and removed testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Jun 1, 2020
@alliepiper alliepiper merged commit d26be73 into NVIDIA:master Jun 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
testing: gpuCI passed Passed gpuCI testing. testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants