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

[REVIEW] Introducing host API for PCG #1767

Merged
merged 43 commits into from
Sep 1, 2023

Conversation

vinaydes
Copy link
Contributor

@vinaydes vinaydes commented Aug 23, 2023

This PR introduces following changes

  1. Adds support for calling PCG RNG API from the host
  2. Adds a test for verifying host API
  3. Fixes a bug in Normal PDF deviate function for integers (custom_next)
  4. Adds a wide 64-bit multiplication function for both host and device
  5. Adds a test for 64-bit wide multiplication on host and device

Copy link
Contributor

@MatthiasKohl MatthiasKohl left a comment

Choose a reason for hiding this comment

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

Thank you very much @vinaydes , LGTM

@MatthiasKohl
Copy link
Contributor

Just referencing issue #1560 which should be closed by this (I cannot add it to the development dependencies since I don't seem to have privileges for this)

@MatthiasKohl
Copy link
Contributor

I missed the fact that the file was moved, thanks for pointing it out.
However, I don't understand your comment b) @achirkin . The CUDA asm in the functions is protected by pre-processor already. However, the "HDI" macro is an issue, so regardless of CUDA asm, this function cannot be called from a host-compiler. I didn't know that this was the intention here, and it is in a .cuh file, so everything looked good to me.
If we want RAFT to actually care about host compilers, we need to significantly overhaul it.

@vinaydes
Copy link
Contributor Author

vinaydes commented Aug 31, 2023

@MatthiasKohl For now, going with option b) will enable us to call the wide multiply from .cu files for using in the host code.

@MatthiasKohl
Copy link
Contributor

@MatthiasKohl For now, going with option b) will enable us to call the wide multiply from .cu files for using in host code.

Right, I don't understand the second part of Artem's comment though.
Basically, if these functions could go into .hpp then what file should they be in?
My point is that if we wanted them to be in .hpp, it would be very easy to allow them to be there by simply changing out the HDI macro to one that allows a host-compiler to compile as well.

@achirkin
Copy link
Contributor

achirkin commented Aug 31, 2023

@MatthiasKohl , you're probably right. It didn't occur to me that __CUDA_ARCH__ would still be available in *.hpp included by a *.cu file :)
I guess, then it's natural to move it to the existing raft/util/integer_utils.hpp. HDI would need to be replaced with _RAFT_HOST_DEVICE from core/detail/macros.hpp.

@achirkin
Copy link
Contributor

I wonder though. If we start to guard the whole kernels with pre-processor macros, we can end up with replacing all headers in raft with .hpp versions. Do we want that? Where's the boundary between what we want to have in .cuh and .hpp?..

@vinaydes
Copy link
Contributor Author

I guess, then it's natural to move it to the existing raft/util/integer_utils.hpp.

I was going to say we create a integer_utils.cuh but then I took a quick glance at the other .hpp files in the project. Several of them have cuda code. I guess I'll put the function in the integer_utils.hpp.

@vinaydes
Copy link
Contributor Author

@achirkin @MatthiasKohl I have tried to accommodate the changes. It should be non-breaking now 🤞.
Also Artem do you mind doing /ok to test one more time?

@achirkin
Copy link
Contributor

/ok to test

@achirkin
Copy link
Contributor

/ok to test

@vinaydes
Copy link
Contributor Author

@achirkin Can we mark this as non-breaking again?

@achirkin achirkin added non-breaking Non-breaking change and removed breaking Breaking change labels Aug 31, 2023
@vinaydes
Copy link
Contributor Author

vinaydes commented Sep 1, 2023

Can this be merged now?

@achirkin
Copy link
Contributor

achirkin commented Sep 1, 2023

Apparently I don't have enough power to summon the merger bot :)

@achirkin
Copy link
Contributor

achirkin commented Sep 1, 2023

/ok to test

@cjnolet
Copy link
Member

cjnolet commented Sep 1, 2023

/merge

@rapids-bot rapids-bot bot merged commit 02e44ac into rapidsai:branch-23.10 Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants