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] Add mcg59 #152

Open

kcenia4010 wants to merge 7 commits into oneapi-src:develop from kcenia4010:dev/kzaitsev/mcg59

kcenia4010 commented Dec 28, 2021 •

edited

Add mcg59 engine


          [RNG] Add mcg59

722c312

aelizaro reviewed

View reviewed changes

tests/unit_tests/rng/include/engines_api_tests.hpp Outdated

+                  }
+                  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)

Contributor

aelizaro Jan 11, 2022

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)

Author

kcenia4010 Jan 12, 2022

done

src/rng/backends/mklgpu/mcg59.cpp Outdated

		@@ -0,0 +1,586 @@
		/*******************************************************************************
		* Copyright 2020-2021 Intel Corporation

Contributor

aelizaro Jan 11, 2022

Old copyright

Author

kcenia4010 Jan 12, 2022

done

kcenia4010 added 2 commits

January 12, 2022 15:05


          [RNG] comment fix

7382d38


          [RNG] Clang-format

28f8bde

andreyfe1 reviewed

View reviewed changes

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

Contributor

andreyfe1 Jan 14, 2022

Should here be MCG59, not Mcg59?

Author

kcenia4010 Jan 20, 2022

done

src/rng/backends/curand/mcg59.cpp Outdated

		@@ -0,0 +1,110 @@
		/*******************************************************************************
		* cuRAND back-end Copyright (c) 2021, The Regents of the University of

Contributor

andreyfe1 Jan 14, 2022

Am I right that we should have 2022?

Author

kcenia4010 Jan 20, 2022

done

src/rng/backends/mklcpu/mcg59.cpp Outdated

		@@ -0,0 +1,579 @@
		/*******************************************************************************
		* Copyright 2021 Intel Corporation

Contributor

andreyfe1 Jan 14, 2022

Am I right that we should have 2022?

Author

kcenia4010 Jan 20, 2022

done

src/rng/backends/mklcpu/mcg59.cpp Outdated

+              namespace oneapi {
+              namespace mkl {
+              namespace rng {
+              namespace mklcpu {

Contributor

andreyfe1 Jan 14, 2022

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

Author

kcenia4010 Jan 20, 2022

done

src/rng/backends/mklcpu/mcg59.cpp Outdated

+              namespace rng {
+              namespace mklcpu {
+              using namespace cl;

Contributor

andreyfe1 Jan 14, 2022

I'd suggest to avoid using namespace according to C++ guidelines: https://google.github.io/styleguide/cppguide.html#Namespaces

Author

kcenia4010 Jan 20, 2022

done

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);

Contributor

andreyfe1 Jan 14, 2022

Can we use SYCL 2020 accessor interfaces here (maybe under the macro if the code should work with SYCL 1.2.1)?

Author

kcenia4010 Jan 20, 2022

done

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 {

Contributor

andreyfe1 Jan 14, 2022

1 is the default dimension value for sycl::buffer for SYCL 2020 and SYCL 1.2.1 both. Can we avoid 1?

Author

kcenia4010 Jan 20, 2022

done

include/oneapi/mkl/rng/detail/curand/onemkl_rng_curand.hpp

               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,

Contributor

andreyfe1 Jan 14, 2022

cl::sycl:: is for SYCL 1.2.1. For SYCL 2020 we can use just sycl::

Contributor

sbalint98 Jan 17, 2022

I think, as described in issue #149, using the sycl:: namespace is only SYCL conformant when the sycl/sycl.hpp header is included.

Author

kcenia4010 Jan 20, 2022

There is a PR from ROCK RAND. Will change after them merge

tests/unit_tests/rng/include/engines_api_tests.hpp Outdated

-                          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);

Contributor

andreyfe1 Jan 14, 2022

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

Author

kcenia4010 Jan 20, 2022

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.

tests/unit_tests/rng/include/rng_test_common.hpp

+                          for (int p = 0; p < 2; p++) {
+                              if (!check_equal(r2[j++], r1[k * n_engines + i * 2 + p])) {
+                                  good = false;
+                                  break;

Contributor

andreyfe1 Jan 14, 2022

I think we can replace these 2 lines with return false and have return true at the end of the function

Author

kcenia4010 Jan 20, 2022

done

kcenia4010 added 4 commits

January 20, 2022 22:38


          [RNG] code style fix

a04b09d


          [RNG] clang format

f50b2c2


          [RNG] change accessor

cd6a555


          [RNG] delete namespace

a8861e7

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