[IMP] move core CUDA RT macros to cuda_rt_essentials.hpp #1584
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MatthiasKohl
added
3 - Ready for Review
improvement
ahendriksen
requested changes
Jun 9, 2023
| do { \ | ||
| cudaError_t const status = call; \ | ||
| if (cudaSuccess != status) { \ | ||
| printf("CUDA call='%s' at file=%s line=%d failed with %s\n", \ |
There was a problem hiding this comment.
One change request related to the header being self-standing. Can you add an include of cstdio to the top of cuda_rt_essentials.hpp?
This should have been done before, but probably escaped review.
There was a problem hiding this comment.
actually, nothing was directly using cstdio in this file and cstdio was only included transitively through core/error.hpp for things like SET_ERROR_MSG.
So all was actually good before (IMO), and I fixed the include here now since we now use cstdio directly in this file.
Thanks for pointing it out !
ahendriksen
approved these changes
Jun 9, 2023
|
This should be good to be merged |
cjnolet
approved these changes
Jun 10, 2023
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The reasoning behind this PR is as follows:
for now, anyone wanting to use
RAFT_CUDA_TRY_NO_THROWstill needs to includecudart_utils.hppwhich can be costly (compilation) due to the include ofmemory_pool.hpp.By moving the macros to the essentials, we should not break anything for anyone, but allow anyone to improve compilation times by including the essentials only. At the same time, it should add minimal overhead to the compilation time of the essentials file since the pre-processor is (usually) fast compared to the rest of the compilation pipeline.