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

Add host/device compatible RNG #41

Merged
merged 2 commits into from
Sep 22, 2020
Merged

Add host/device compatible RNG #41

merged 2 commits into from
Sep 22, 2020

Conversation

whokion
Copy link
Contributor

@whokion whokion commented Sep 10, 2020

Make RngEngine work on both host and device. This PR addresses and replaces the issue #19.
After merged, need to add options to use other generators/random states such as
MRG32k3a/curandStateMRG32k3a_t, Random123/curandStatePhilox4_32_10_t and etc, which
should work out of box. Also, generalize RngEngine.test.cu which currently works only for curandStateXORWOW.

@whokion whokion requested a review from pcanal September 10, 2020 17:59
@whokion whokion self-assigned this Sep 10, 2020
pcanal
pcanal previously approved these changes Sep 10, 2020
@sethrj sethrj self-requested a review September 15, 2020 13:40
@sethrj sethrj assigned sethrj and unassigned whokion Sep 15, 2020
@sethrj sethrj added the core Software engineering infrastructure label Sep 15, 2020
@sethrj sethrj changed the title Syjun/crng Add host/device compatible RNG Sep 15, 2020
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Two minor comments, and please add a very small unit test (as simple as just another TEST( inside of RngEngine.test.cu) showing that the initialization and sampling works for the RNG and generate_canonical functions. Thanks!

src/random/cuda/RngEngine.cuh Outdated Show resolved Hide resolved
src/random/cuda/RngStatePointers.hh Outdated Show resolved Hide resolved
int num_samples = 1024*1000;
unsigned long seed = 12345u;

RngEngine rng(new RngState);
Copy link
Member

Choose a reason for hiding this comment

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

This is a memory leak. For consistency with the other host tests, I suggest making a local RngState host_rng_state; and passing it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed accordingly.

@sethrj
Copy link
Member

sethrj commented Sep 22, 2020

@whokion Aside from the single comment, there are two minor metadata-type changes:

  1. The RngEngine test isn't formatted properly with clang-format. If you have clang-format installed, there's a script at scripts/dev/install-commit-hooks.sh that will install a git hook to auto-format it. Once that's installed, you can rebase interactively on master and it should auto-apply formatting to your commits.
  2. Very minor comment but for consistency it would be good to captialize the first letter of commit messages 😅

If you don't have clang-format installet, let me know and I can help reformat the commits.

@whokion
Copy link
Contributor Author

whokion commented Sep 22, 2020

Tried to apply the clang-format. Please confirm.

@sethrj
Copy link
Member

sethrj commented Sep 22, 2020

@whokion Something's still off... I'll just reformat on my end.

By the way, what's the advantage of the

CELER_FUNCTION RngEngine(RngState *state) : state_(*state) {}

constructor? Is that only defined for the host unit test?

@whokion
Copy link
Contributor Author

whokion commented Sep 22, 2020

Yes, the constructor is for the purpose of tests (for now), but may be served as a generic constructor used for both host and device depending on how RngEngine is instantiated. The current approach which is binding
a set of pre-initialized states (along with tid) and RngEngine itself is a bit less flexible if we really want to
use codes for both host and device as initialization APIs may be different between host (for either sequential or multithreading workflow) and device SIMT execution. We may revisit this discussion later, but I do not see any
downside to have this constructor either.

@sethrj
Copy link
Member

sethrj commented Sep 22, 2020

That's fair, but I'd prefer to keep the view construction homogenous (even across potential future architectures) and minimizing the code base (at the expense of adding one extra line to the CPU-only unit test). The downside to having the raw pointer constructor is making the interface more complex and easy to accidentally misuse (there's a missing explicit for the construct-from-pointer).

I've clang-formatted the files, removed the raw-pointer constructor, and squashed the commits into a new branch sethrj/crng -- if those look ok, could you git reset --hard sethrj/crng and git push -f to update your branch to match?

@whokion
Copy link
Contributor Author

whokion commented Sep 22, 2020

Okay. Fine with me. I will consolidate your update and push the branch again.

@sethrj sethrj merged commit 5dd64ca into master Sep 22, 2020
@sethrj sethrj deleted the syjun/crng branch September 22, 2020 23:23
@sethrj sethrj linked an issue Oct 24, 2020 that may be closed by this pull request
3 tasks
@sethrj sethrj added the enhancement New feature or request label Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Software engineering infrastructure enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add interface for common RNGs
3 participants