Skip to content

Conversation

@matthiasdiener
Copy link
Contributor

@matthiasdiener matthiasdiener commented Jan 16, 2026

Description

Use rocrand random number generation (instead of CPU/OpenMP).

Partly addresses https://github.com/ROCm/frameworks-internal/issues/14746.

Notes:

Type of change

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Infra/Build change
  • Code refactoring

Changes

Please list the changes introduced in this PR:

-Use rocrand random number generation (instead of CPU/OpenMP).

Checklist:

  • I have read and followed the contributing guidelines
  • The functionality is complete
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@matthiasdiener matthiasdiener self-assigned this Jan 16, 2026
@matthiasdiener matthiasdiener marked this pull request as ready for review January 16, 2026 18:37
Copy link
Contributor

@alextmagro alextmagro left a comment

Choose a reason for hiding this comment

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

Awesome work! Do you have any rough estimates of how much faster the entire cpp test suite is?

template <typename T>
void generate_data_uniformly(T* data, const size_t size, std::mt19937* gen) {
#ifdef __HIP_PLATFORM_AMD__
// TODO: Introduce a parallel RNG library (Random123, PCG, rocRAND)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably remove this entire #ifdef guarded section, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, unless we want to keep it around for future reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove, we can always revert if we need it again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole method seems unused on ROCm and can be guarded

#else
T *data = t->columnwise_cpu_dptr<T>();
generate_data_uniformly(data, size, &(t->gen()));
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to use rocRAND for fillCase_special as well? Also, I think there were a few tests that for some reason generate their own data... I might be wrong about that, or it may have been updated.

@matthiasdiener
Copy link
Contributor Author

Awesome work! Do you have any rough estimates of how much faster the entire cpp test suite is?

Thanks! I left some performance numbers here: https://github.com/ROCm/frameworks-internal/issues/14746#issuecomment-3761360289

@ipanfilo
Copy link
Collaborator

Update copyright date of modified files

}

#ifdef __HIP_PLATFORM_AMD__
#include <rocrand/rocrand.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though it does not cause errors, better move #include to the top of the file, out of test namespace

TRANSFORMER_ENGINE_TYPE_SWITCH_ALL(t->dtype(), T,
{
#ifdef __HIP_PLATFORM_AMD__
fillUniformDevice(t);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any test that tests this generation? I think using GPU generation here does not produce correct result because of using t->from_cpu() below in this method

template <typename T>
void generate_data_uniformly(T* data, const size_t size, std::mt19937* gen) {
#ifdef __HIP_PLATFORM_AMD__
// TODO: Introduce a parallel RNG library (Random123, PCG, rocRAND)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole method seems unused on ROCm and can be guarded

rocrand_generate_uniform(gen, tmp, N);

// map to [-2.0, 1.0] (like generate_data_uniformly) and cast into tensor dtype
TRANSFORMER_ENGINE_TYPE_SWITCH_ALL(t->dtype(), T, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

T should either be template parameter and no TRANSFORMER_ENGINE_TYPE_SWITCH_ALL here, or the method calling should be moved out of TRANSFORMER_ENGINE_TYPE_SWITCH_ALL in fillUniform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants