[RNG] Add mcg59 #152
[RNG] Add mcg59 #152
Conversation
| } | ||
|
|
||
| template <> | ||
| void generate<oneapi::mkl::rng::mcg59>(oneapi::mkl::rng::bits<std::uint32_t> distr, oneapi::mkl::rng::mcg59 &engine, std::uint64_t n, cl::sycl::buffer<std::uint32_t, 1>& buf) |
There was a problem hiding this comment.
Please, add a note, why we need to use n\2 here (it would be great if it's documented in the specification, but I am not sure)
src/rng/backends/mklgpu/mcg59.cpp
Outdated
| @@ -0,0 +1,586 @@ | |||
| /******************************************************************************* | |||
| * Copyright 2020-2021 Intel Corporation | |||
include/oneapi/mkl/rng/engines.hpp
Outdated
| @@ -210,6 +210,77 @@ class mrg32k3a { | |||
| const std::vector<sycl::event>& dependencies); | |||
| }; | |||
|
|
|||
| // Class oneapi::mkl::rng::mcg59 | |||
| // | |||
| // Represents Mcg59 counter-based pseudorandom number generator | |||
There was a problem hiding this comment.
Should here be MCG59, not Mcg59?
src/rng/backends/curand/mcg59.cpp
Outdated
| @@ -0,0 +1,110 @@ | |||
| /******************************************************************************* | |||
| * cuRAND back-end Copyright (c) 2021, The Regents of the University of | |||
There was a problem hiding this comment.
Am I right that we should have 2022?
src/rng/backends/mklcpu/mcg59.cpp
Outdated
| @@ -0,0 +1,579 @@ | |||
| /******************************************************************************* | |||
| * Copyright 2021 Intel Corporation | |||
There was a problem hiding this comment.
Am I right that we should have 2022?
src/rng/backends/mklcpu/mcg59.cpp
Outdated
| namespace oneapi { | ||
| namespace mkl { | ||
| namespace rng { | ||
| namespace mklcpu { |
There was a problem hiding this comment.
I believe we can have nested namespace definition if the minimal C++ version is 17: namespace oneapi::mkl::rng::mklcpu { ... } if it doesn't violate any internal coding style
src/rng/backends/mklcpu/mcg59.cpp
Outdated
| namespace rng { | ||
| namespace mklcpu { | ||
|
|
||
| using namespace cl; |
There was a problem hiding this comment.
I'd suggest to avoid using namespace according to C++ guidelines: https://google.github.io/styleguide/cppguide.html#Namespaces
src/rng/backends/mklcpu/mcg59.cpp
Outdated
| sycl::buffer<char, 1> stream_buf(static_cast<char*>(stream_), state_size_); | ||
| queue_.submit([&](sycl::handler& cgh) { | ||
| auto acc_stream = stream_buf.get_access<sycl::access::mode::read_write>(cgh); | ||
| auto acc_r = r.get_access<sycl::access::mode::read_write>(cgh); |
There was a problem hiding this comment.
Can we use SYCL 2020 accessor interfaces here (maybe under the macro if the code should work with SYCL 1.2.1)?
src/rng/backends/mklcpu/mcg59.cpp
Outdated
| // Buffers APIs | ||
|
|
||
| virtual void generate(const uniform<float, uniform_method::standard>& distr, std::int64_t n, | ||
| cl::sycl::buffer<float, 1>& r) override { |
There was a problem hiding this comment.
1 is the default dimension value for sycl::buffer for SYCL 2020 and SYCL 1.2.1 both. Can we avoid 1?
| @@ -82,6 +82,12 @@ ONEMKL_EXPORT oneapi::mkl::rng::detail::engine_impl* create_mrg32k3a(cl::sycl::q | |||
| ONEMKL_EXPORT oneapi::mkl::rng::detail::engine_impl* create_mrg32k3a( | |||
| cl::sycl::queue queue, std::initializer_list<std::uint32_t> seed); | |||
|
|
|||
| ONEMKL_EXPORT oneapi::mkl::rng::detail::engine_impl* create_mcg59(cl::sycl::queue queue, | |||
There was a problem hiding this comment.
cl::sycl:: is for SYCL 1.2.1. For SYCL 2020 we can use just sycl::
There was a problem hiding this comment.
I think, as described in issue #149, using the sycl:: namespace is only SYCL conformant when the sycl/sycl.hpp header is included.
There was a problem hiding this comment.
There is a PR from ROCK RAND. Will change after them merge
| oneapi::mkl::rng::generate(distr, engine2, N_GEN, r2_buffer); | ||
| oneapi::mkl::rng::generate(distr, engine3, N_GEN, r3_buffer); | ||
| oneapi::mkl::rng::generate(distr, engine4, N_GEN, r4_buffer); | ||
| generate(distr, engine1, N_GEN, r1_buffer); |
There was a problem hiding this comment.
Why do you think that fully qualified name is not needed here?
From https://docs.microsoft.com/en-us/cpp/cpp/namespaces-cpp:
Code in header files should always use the fully qualified namespace name.
Of course, the remark is not critical since the code is for tests
There was a problem hiding this comment.
This function isn't from this namespace. I defined it above. But I see that it can be a bit confusing, so I renamed it.
| for (int p = 0; p < 2; p++) { | ||
| if (!check_equal(r2[j++], r1[k * n_engines + i * 2 + p])) { | ||
| good = false; | ||
| break; |
There was a problem hiding this comment.
I think we can replace these 2 lines with return false and have return true at the end of the function
Add mcg59 engine