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.

Sign up for GitHub

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

Jump to bottom

[RNG] Added experimental support of Philox counter-based PRNG #1785

Merged

ElenaTyuleneva merged 55 commits into main from dev/etyulene/eng_philox_engine

Sep 4, 2024

Contributor

ElenaTyuleneva commented Aug 20, 2024 •

edited

Loading

C++ draft proposal was used as a design for public API - p2075r6.pdf, the latest correction by the time of PR creation is issue4134

Things to make before the merge:

Add testing for parameter n = 2 <- added statistics test philox_uniform_real_distribution_test.pass.cpp to check that we are generating a statistically valid sequence.
Add uglification
Mark the feature as experimental since C++ std proposal has not yet got the final revision
Add statistics test for scalar-input Philox for W!=32 and W!=64
Apply clang-format

ElenaTyuleneva added 25 commits

July 4, 2024 02:22


          * Initial changes for Philox support

1b685d5


          * c param removed

cc88633


          Stuff functions added

0ce5e38


          Implementation is made simplier. v.1

1115e29


          * Operator () returns the correct result

d8cd85f


          * discard() method is almost working

29eff6d


          * discard() works correctly

83d3f7b


          * Initial changes for Philox support

d8cc9de


          * c param removed

5cc7358


          Stuff functions added

a46c755


          Implementation is made simplier. v.1

37c902d


          * Operator () returns the correct result

ac35a03


          * discard() method is almost working

4381b8e


          * discard() works correctly

d3e6359


          Merge branch 'dev/etyulene/eng_philox_engine' of https://github.com/o…

a014fdc

…neapi-src/oneDPL into dev/etyulene/eng_philox_engine


          * Sources clean-up

f2c8c97


          * State structure changed

4ba638a


          * Small refactoring

d70a34e


          * Operators (==, !=, >> and <<) added, as well as test for them

0f4899e


          Merge remote-tracking branch 'origin/main' into dev/etyulene/eng_phil…

0c200d4

…ox_engine


          * Interface tests fixed

e20f8b1


          * Implemented increase_counter_internal() with a increment as an inpu…

d299ad7

…t param


          * class API was extended to support sycl::vec

7edd1b0


          * Refactoring done[1]

98314aa


          * Refactoring[2]

a4ea837

ElenaTyuleneva requested review from paveldyakov and aelizaro

August 20, 2024 12:38

aelizaro reviewed

View reviewed changes

include/oneapi/dpl/internal/random_impl/counter_based_engines_stuff.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/counter_based_engines_stuff.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/counter_based_engines_stuff.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/counter_based_engines_stuff.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

test/xpu_api/random/conformance_tests/common_for_conformance_tests.hpp Outdated Show resolved Hide resolved

test/xpu_api/random/conformance_tests/philox_test.pass.cpp Outdated Show resolved Hide resolved

aelizaro reviewed

View reviewed changes

documentation/library_guide/api_for_sycl_kernels/random.rst Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved


          * Corrected review comments

9b3a174

ElenaTyuleneva added 9 commits

September 2, 2024 07:11


          * Constants order was also changed in documentation and test logging

3fd3a9f


          * Removed conversion to tuple

091819d


          * Removed :: before std according to the latest standards

4102ef3


          * Stuff from detail was moved to the class

2faf49c

* Uglified word_mask


          * class=>typename in templates

30bb75e

* Aligned the number of _ in templates
* Corrected max method(added -1)
* Scalar generate_internal() is now working in-place without extra memory buffer
* Applied mask in mulhilo calculations
* Added necessary headers


          * philox_test.pass was extended with precise corner-case testing

ccc0f1f

* small corrections in philox_uniform_real_distribution_test.pass.cpp


          * set_counter() a bit simplified

c7ee5a5

* Make philox eqation more obvious in the code


          * The logic of idx iteration is changed

0afacfe


          * Statistics tests were extended testing for cases bitsize(result_typ…

bdb501e

…e) != word_size

aelizaro reviewed

View reviewed changes

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

aelizaro reviewed

View reviewed changes

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

test/xpu_api/random/interface_tests/engines_methods.pass.cpp Outdated Show resolved Hide resolved

ElenaTyuleneva added 2 commits

September 3, 2024 08:59


          * Added a separate unit testing with corner-case testing functions

d4f9186


          Merge remote-tracking branch 'origin/main' into dev/etyulene/eng_phil…

59b2209

…ox_engine

aelizaro reviewed

View reviewed changes

test/xpu_api/random/unit_tests/philox_unit_test.pass.cpp Outdated Show resolved Hide resolved


          * applied review comments

d0edaa8

aelizaro reviewed

View reviewed changes

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

ElenaTyuleneva and others added 2 commits

September 3, 2024 12:01


          * Applied clang-format

08cbed5


          Update filename in philox_engine.h

3fd8887

Co-authored-by: Alina Elizarova <[email protected]>

aelizaro reviewed

View reviewed changes

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated

+                  static constexpr ::std::size_t word_count = _n;
+                  static constexpr ::std::size_t round_count = _r;
+                  static_assert(_n == 2 || _n == 4, "_n must be 2 or 4");

Contributor

aelizaro Sep 3, 2024

"parameter n must be 2 or 4"

parameter name is just aligned with spec

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Show resolved Hide resolved

include/oneapi/dpl/internal/random_impl/philox_engine.h Outdated Show resolved Hide resolved

test/xpu_api/random/unit_tests/philox_unit_test.pass.cpp Show resolved Hide resolved


          * Review comments partly addressed

b0bd54b

timmiesmith added this to the 2022.7.0 milestone

ElenaTyuleneva added 3 commits

September 4, 2024 08:39


          * Applied cosmetic comments

e969ee4

* Overflow testing was added for w!=32 and w!=64


          * Changed max() method

e99667f


          * Changed max value in test and applied clang-format

1ff549c

Contributor Author

ElenaTyuleneva commented Sep 4, 2024

@aelizaro , the PR is ready for the final review, I've pushed all planned commits here.

aelizaro approved these changes

View reviewed changes

Contributor

aelizaro left a comment

Thank you for the huge work @ElenaTyuleneva!

The changes looks good to me.
Reviewed offline with @rarutyun. All the major design comments are resolved; we agreed to merge the feature as experimental. The algorithmic part of the implementation wasn't checked too thoroughly, but it relies on good test coverage, which is done in this PR.

ElenaTyuleneva merged commit 1072629 into main

22 checks passed

ElenaTyuleneva deleted the dev/etyulene/eng_philox_engine branch

September 4, 2024 19:29

mmichel11 pushed a commit that referenced this pull request


          [RNG] Added experimental support of Philox counter-based PRNG (#1785)

19d3f93

ElenaTyuleneva mentioned this pull request

[RNG] Split up some tests to prevent out-of-memory and device_lost crashes #1923

Merged

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

Labels

None yet