[RNG] Follow-up improvements for Philox RNG engine by ElenaTyuleneva · Pull Request #2619 · uxlfoundation/oneDPL

ElenaTyuleneva · 2026-03-16T15:53:35Z

Description

The PR addresses some comments received after #2603 was merged.

Done:

Unified get_even_element_array() and get_odd_element_array() functions in the Philox class into a single get_consts_by().
Reduced the register's pressure in the __philox_kernel() and __mulhilo() internal functions.

* Merged odd and even constants arrays generation into one method with parity parameter

… of the engines

Copilot

Pull request overview

Follow-up refinement to oneDPL RNG engines (notably philox_engine) to address post-merge feedback from #2603, focusing on constant handling and deterministic initialization for vector return paths.

Changes:

Refactors philox_engine constant unpacking into a single get_consts_by_indices() helper and cleans up some internal naming.
Ensures deterministic initialization of partially-filled sycl::vec results by explicitly constructing result vectors from 0 in portion-generation paths.
Applies the same explicit-zero construction pattern across several RNG engines’ internal generation helpers.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
include/oneapi/dpl/internal/random_impl/subtract_with_carry_engine.h	Explicitly initializes vector result buffers to zero in internal generation paths.
include/oneapi/dpl/internal/random_impl/philox_engine.h	Unifies constant extraction, renames internal masks, and zero-initializes vector result buffers.
include/oneapi/dpl/internal/random_impl/linear_congruential_engine.h	Zero-initializes partial vector results returned by `result_portion_internal()`.
include/oneapi/dpl/internal/random_impl/discard_block_engine.h	Zero-initializes vector result buffers in internal generation paths.

Comments suppressed due to low confidence (3)

include/oneapi/dpl/internal/random_impl/discard_block_engine.h:231

generate_internal() assigns to every lane of _res in all control-flow paths (either by whole-vector assignment from engine() or by filling each element in the loop). Initializing __res with result_type __res(0) is therefore unnecessary and can be a small performance regression. Consider default-constructing here and reserving explicit zero-initialization for the partial-result overload that doesn’t write all lanes.

        result_type __res(0);
        if (static_cast<::std::size_t>(_N) < (used_block - n_))
        {
            __res = engine_();
            n_ += static_cast<::std::size_t>(_N);
        }
        else
        {
            for (int __i = 0; __i < _N; ++__i)
            {
                __res[__i] = generate_internal_scalar<internal::type_traits_t<result_type>::num_elems>();
            }
        }
        return __res;

include/oneapi/dpl/internal/random_impl/philox_engine.h:334

This generate_internal() overload writes every element of __loc_result in the loop, so initializing it with result_type __loc_result(0) is redundant and adds extra per-call work for sycl::vec outputs. Consider default-constructing here, and keep explicit zero-initialization only in the overload that returns a portion (where unwritten lanes must be defined).

        result_type __loc_result(0);
        for (int __elm_count = 0; __elm_count < _N; ++__elm_count)
        {
            ++state_.idx;

            // check if buffer is empty
            if (state_.idx == word_count)
            {
                philox_kernel();
                increase_counter_internal();
                state_.idx = 0;
            }
            __loc_result[__elm_count] = state_.Y[state_.idx];
        }

        return __loc_result;

include/oneapi/dpl/internal/random_impl/subtract_with_carry_engine.h:220

In generate_internal() for vector outputs, __res is fully assigned in the following loop, so zero-initializing it with result_type __res(0) is redundant and adds extra work (sycl::vec scalar-ctor will write all lanes). Consider reverting this to default construction here and keep explicit zero-initialization only in the “portion” generation path where not all lanes are written.

        result_type __res(0);
        for (int __i = 0; __i < _N; ++__i)
        {
            __res[__i] = generate_internal_scalar();
        }
        return __res;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

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

rarutyun · 2026-03-16T19:45:10Z

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

+        __even = 0,
+        __odd = 1


I would suggest a small renaming to

Suggested change

__even = 0,

__odd = 1

__even_indices = 0,

__odd_indices = 1

together with renaming below

Done by the latest changes.

rarutyun · 2026-03-16T19:45:39Z

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

+    template <std::size_t _Offset, std::size_t... _Is>
    static constexpr auto
-    get_odd_element_array(std::array<scalar_type, _n> __input_array, std::index_sequence<_Is...>)
+    get_consts_by_indices(std::index_sequence<_Is...>)


Suggested change

get_consts_by_indices(std::index_sequence<_Is...>)

get_consts_by(std::index_sequence<_Is...>)

Thanks for the proposal, applied!

rarutyun · 2026-03-16T19:46:53Z

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

-    static constexpr auto
-    get_even_element_array(std::array<scalar_type, _n> __input_array, std::index_sequence<_Is...>)
+    /* Method for unpacking variadic of constants into two arrays - with odd and even elements */
+    enum


Suggested change

enum

enum class __indices_offset : std::size_t

Or whatever syntax is correct.

Thanks, the enum approach was sligthly changed.

andreyfe1 · 2026-03-27T10:28:17Z

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

Spec says:

template<class UIntType, size_t w, size_t n, size_t r, UIntType... consts>

Please add uglification

template <typename _UIntType, std::size_t _W, std::size_t _N, std::size_t _R, oneapi::dpl::internal::element_type_t<_UIntType>... _Consts>

Thanks! The uglification was fixed in 7c2e0cd.

andreyfe1 · 2026-03-27T10:33:54Z

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

Uglify this sth like

template <typename _CharT, typename _Traits, typename _UIntType, std::size_t _W, std::size_t _N, std::size_t _R, oneapi::dpl::internal::element_type_t<_UIntType>... _Consts>

andreyfe1 · 2026-03-27T10:35:01Z

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

+        std::array<scalar_type, word_count> __X;     // counters
+        std::array<scalar_type, word_count / 2> __K; // keys
+        std::array<scalar_type, word_count> __Y;     // results
+        scalar_type __idx;                           // index


Suggested change

std::array<scalar_type, word_count> __X; // counters

std::array<scalar_type, word_count / 2> __K; // keys

std::array<scalar_type, word_count> __Y; // results

scalar_type __idx; // index

std::array<scalar_type, word_count> __x; // counters

std::array<scalar_type, word_count / 2> __k; // keys

std::array<scalar_type, word_count> __y; // results

scalar_type __idx; // index

andreyfe1 · 2026-03-27T10:37:38Z

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

+            scalar_type __V0 = __state.__X[0];
+            scalar_type __V1 = __state.__X[1];
+            scalar_type __K0 = __state.__K[0];


Suggested change

scalar_type __V0 = __state.__X[0];

scalar_type __V1 = __state.__X[1];

scalar_type __K0 = __state.__K[0];

scalar_type __v0 = __state.__x[0];

scalar_type __v1 = __state.__k[1];

scalar_type __k0 = __state.__k[0];

andreyfe1 · 2026-03-27T10:41:40Z

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

-            scalar_type __V1 = state_.X[1];
-            scalar_type __K0 = state_.K[0];
+            scalar_type __V0 = __state.__X[0];
+            scalar_type __V1 = __state.__X[1];


Minor: Consider using aliases to x[0], x[1]

Implemented, thanks for the idea!

andreyfe1 · 2026-03-27T10:42:44Z

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

+            scalar_type __V0 = __state.__X[2];
+            scalar_type __V3 = __state.__X[3];
+            scalar_type __K0 = __state.__K[0];
+            scalar_type __K1 = __state.__K[1];


Same as above

Implemented as part of 94c9da5.

andreyfe1 · 2026-03-27T10:49:31Z

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

if y1, x1, y0 are not used below it can be reused (with aliases if it's better). It can affect performance due to high registers' pressure

Reduced the amount of temporary registers from 11 to 7 in this function (ca2ff49).

Copilot

Pull request overview

This PR is a follow-up cleanup for the Philox RNG engine implementation, addressing review feedback from #2603 by tightening internal naming/uglification, simplifying constants unpacking, and adjusting internal kernels to reduce register pressure.

Changes:

Uglified Philox private members/methods and updated related friend/operator code accordingly.
Unified even/odd constants extraction into a single __get_consts_by() helper.
Refactored __philox_kernel() / __mulhilo() internals to reduce temporary variables/register pressure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

andreyfe1

lgtm. Make sure that tests are passed

rarutyun

Honestly, I would drop all the uglification from the PR. As I mentioned before, I would not "pollute" commits history with meaningless (from the code perspective) changes.

If uglification matters, I would introduce it only for the lines that have meaningful changes.

As an example, we had ::std namespace all over the place. Over time, the design was settled and we found out that we don't need :: before std. Instead of changing everything at once and create big "pollution" in our code base, we change namespace :: prefix only in the lines that have other meaningful changes.

This reverts commit 7c2e0cd.

ElenaTyuleneva · 2026-04-13T11:27:28Z

Honestly, I would drop all the uglification from the PR. As I mentioned before, I would not "pollute" commits history with meaningless (from the code perspective) changes.

If uglification matters, I would introduce it only for the lines that have meaningful changes.

As an example, we had ::std namespace all over the place. Over time, the design was settled and we found out that we >don't need :: before std. Instead of changing everything at once and create big "pollution" in our code base, we change >namespace :: prefix only in the lines that have other meaningful changes.

I agree that mixing large style-only changes with functional updates makes the history harder to follow. At the same time, this part of the code doesn’t change very often, so opportunities to clean it up incrementally are relatively rare and I’m a bit concerned it may take a while to converge to a consistent style.

To keep this PR focused, I’ve moved all uglification-related changes into a separate branch and will proceed with just the functional/refactoring changes here. We can then handle the naming cleanup independently (either incrementally or all at once - let's discuss offline what works best).

* Corrected uglifications

0bb56f4

* Merged odd and even constants arrays generation into one method with parity parameter

ElenaTyuleneva mentioned this pull request Mar 16, 2026

[RNG] Move the Philox engine out of experimental state #2603

Merged

* Zero-initialized constructor is used in the sycl_vec instantiations…

e094ac6

… of the engines

ElenaTyuleneva changed the title ~~[RNG] Follow-up improvements in the Philox engine~~ [RNG] Follow-up improvements in the Philox and other random engines Mar 16, 2026

ElenaTyuleneva changed the title ~~[RNG] Follow-up improvements in the Philox and other random engines~~ [RNG] Follow-up improvements for Philox and other random number engines Mar 16, 2026

ElenaTyuleneva marked this pull request as ready for review March 16, 2026 18:49

ElenaTyuleneva requested review from andreyfe1 and Copilot March 16, 2026 18:49

Copilot started reviewing on behalf of ElenaTyuleneva March 16, 2026 18:50 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

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

timmiesmith requested a review from rarutyun March 16, 2026 19:09

rarutyun reviewed Mar 16, 2026

View reviewed changes

ElenaTyuleneva added 5 commits March 17, 2026 12:29

* Addressed the renaming proposal

975526f

* Uglified all private methods and members

c3f22f6

* Reverted zero-initialization for engines where loop goes to _N

ee67d01

* Finalized the enum class renaming proposal

b221676

* Reverted the zero-initialization of the output sycl::vec

9761ce8

andreyfe1 reviewed Mar 27, 2026

View reviewed changes

ElenaTyuleneva changed the title ~~[RNG] Follow-up improvements for Philox and other random number engines~~ [RNG] Follow-up improvements for Philox RNG engine Mar 27, 2026

ElenaTyuleneva added 3 commits April 7, 2026 05:33

* Addressed the review comments

7c2e0cd

* Reduced the usage of the registers in some of the internal kernels.

94c9da5

* Reduced registers pressure for the __mulhilo function.

ca2ff49

ElenaTyuleneva requested a review from Copilot April 8, 2026 14:40

Copilot started reviewing on behalf of ElenaTyuleneva April 8, 2026 14:40 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

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

andreyfe1 previously approved these changes Apr 8, 2026

View reviewed changes

rarutyun reviewed Apr 10, 2026

View reviewed changes

Revert "* Addressed the review comments"

77d569a

This reverts commit 7c2e0cd.

ElenaTyuleneva dismissed andreyfe1’s stale review via 77d569a April 13, 2026 10:20

	get_consts_by_indices(std::index_sequence<_Is...>)
	get_consts_by(std::index_sequence<_Is...>)

Conversation

ElenaTyuleneva commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyfe1 Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

andreyfe1 left a comment

Choose a reason for hiding this comment

Uh oh!

rarutyun left a comment

Choose a reason for hiding this comment

Uh oh!

ElenaTyuleneva commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ElenaTyuleneva commented Mar 16, 2026 •

edited

Loading

andreyfe1 Mar 27, 2026 •

edited

Loading