Skip to content
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

Lazy initialization of queue inside dpcpp_default policy #1154

Closed
wants to merge 8 commits into from
Closed

Lazy initialization of queue inside dpcpp_default policy #1154

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3c95aca
Lazy initialization of queue inside dpcpp_default policy
adamfidel Sep 6, 2023
a3aea24
Fix clang-format
adamfidel Sep 6, 2023
5699334
Update fpga_policy to use lazy initialization as well
adamfidel Sep 6, 2023
8b6bd6d
Clang-format
adamfidel Sep 6, 2023
888e0cb
Make device_policy thread-safe
adamfidel Sep 6, 2023
2c3096d
clang-format
adamfidel Sep 6, 2023
9d6b47a
Changes to device_policy ctors for thread-safety
adamfidel Sep 6, 2023
fee82e0
Check for self-assignment, use static lock method for copy/move ctors…
adamfidel Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 76 additions & 18 deletions include/oneapi/dpl/pstl/hetero/dpcpp/execution_sycl_defs.h
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add a test that fails before this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might be able to add a test that just includes oneDPL but doesn't call anything, and then invoke it with a device filter that disables all devices (e.g., ONEAPI_DEVICE_SELECTOR='!*:*'). In this case, the old code should throw an exception whereas this PR should not.

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 have manually confirmed that this test works in principle. I created the following test:

#include <oneapi/dpl/execution>

int main()
{
    return 0;
}

Running it as ONEAPI_DEVICE_SELECTOR='!*:*' ./build/test/header_inclusion_only.pass crashes on main but does not crash with this branch.

I feel that there is benefit in adding this test, but it will require some special changes to the CMake to modify the environment specifically for this test. It is further complicated by the fact that the CMake files currently already do something similar with the variable $ONEAPI_DEVICE_SELECTOR, which is either set to ONEAPI_DEVICE_SELECTOR=gpu/acc/fpga or SYCL_DEVICE_FILTER=gpu/acc/fpga depending on the compiler version and device type.

The dynamic selection branch also makes changes to this variable. In all, I believe that there is value for adding this test but the risks are high to unintentionally break something with this being so close to the code freeze.

Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

#include "sycl_defs.h"

#include <mutex>
#include <optional>
#include <type_traits>

namespace oneapi
Expand All @@ -42,21 +44,66 @@ struct DefaultKernelName;
template <typename KernelName = DefaultKernelName>
class device_policy
{
// Needed for the copy constructor that rebinds the kernel name
template <typename>
friend class device_policy;

template <typename T>
static auto
lock_and_forward(T&& t, std::mutex& mtx)
adamfidel marked this conversation as resolved.
Show resolved Hide resolved
{
::std::scoped_lock lock{mtx};
return std::forward<T>(t);
}

public:
using kernel_name = KernelName;

device_policy() = default;
explicit device_policy(sycl::queue q_) : q(q_) {}
explicit device_policy(sycl::device d_) { q.emplace(d_); }

template <typename OtherName>
device_policy(const device_policy<OtherName>& other) : q(other.queue())
device_policy(const device_policy<OtherName>& other) : q(device_policy::lock_and_forward(other.q, other.mtx))
{
}
explicit device_policy(sycl::queue q_) : q(q_) {}
explicit device_policy(sycl::device d_) : q(d_) {}
operator sycl::queue() const { return q; }

device_policy(const device_policy& other) : q(device_policy::lock_and_forward(other.q, other.mtx)) {}

device_policy(device_policy&& other) : q(device_policy::lock_and_forward(::std::move(other.q), other.mtx)) {}

device_policy&
operator=(const device_policy& other)
SergeyKopienko marked this conversation as resolved.
Show resolved Hide resolved
{
if (this != &other)
{
::std::scoped_lock lock{mtx, other.mtx};
q = other.q;
}
return *this;
}

device_policy&
operator=(device_policy&& other)
{
if (this != &other)
{
::std::scoped_lock lock{mtx, other.mtx};
q = ::std::move(other.q);
}
return *this;
}

operator sycl::queue() const { return queue(); }
sycl::queue
queue() const
{
return q;
::std::scoped_lock lock{mtx};
if (!q)
SergeyKopienko marked this conversation as resolved.
Show resolved Hide resolved
{
q.emplace();
}
return *q;
}

// For internal use only
Expand All @@ -77,8 +124,9 @@ class device_policy
return ::std::true_type{};
}

private:
sycl::queue q;
protected:
mutable ::std::mutex mtx;
SergeyKopienko marked this conversation as resolved.
Show resolved Hide resolved
mutable ::std::optional<sycl::queue> q;
mmichel11 marked this conversation as resolved.
Show resolved Hide resolved
};

#if _ONEDPL_FPGA_DEVICE
Expand All @@ -91,21 +139,31 @@ class fpga_policy : public device_policy<KernelName>
public:
static constexpr unsigned int unroll_factor = factor;

fpga_policy()
: base(sycl::queue(
# if _ONEDPL_FPGA_EMU
__dpl_sycl::__fpga_emulator_selector()
# else
__dpl_sycl::__fpga_selector()
# endif // _ONEDPL_FPGA_EMU
))
fpga_policy() = default;
template <unsigned int other_factor, typename OtherName>
fpga_policy(const fpga_policy<other_factor, OtherName>& other) : base(other.queue())
{
}

template <unsigned int other_factor, typename OtherName>
fpga_policy(const fpga_policy<other_factor, OtherName>& other) : base(other.queue()){};
explicit fpga_policy(sycl::queue q) : base(q) {}
explicit fpga_policy(sycl::device d) : base(d) {}

operator sycl::queue() const { return queue(); }
sycl::queue
queue() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct that this function sycl::queue queue() should be declared as virtual now in this and base class?

  1. Let's assume we have created the policy instance of fpga_policy
    After this PR this instance doesn't have created sycl::queue instance inside as it's lazy initialized.
  2. In some code we save result of static_cast<device_policy&>(policy) into auto& d_policy:
    fpga_policy policy;
    auto& d_policy = static_cast<device_policy&>(policy);
  1. and after that we call queue() function from reference to the base class:
    fpga_policy policy;
    auto& d_policy = static_cast<device_policy&>(policy);
    auto q = d_policy.queue();                   // call device_policy::queue() !?

Looks like we will call queue() method of the base class and creation of sycl::queue instance will be executed incorrectly.

Copy link
Contributor

@SergeyKopienko SergeyKopienko Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or we need some another approach to resolve this issue: may be we are able to pack sycl::queue creation into someone predicates: one predicate for the base class and one for derived class and so on.

May be able to apply something like #1158
but this PR doesn't resolve the problem of broken lazy initialization in constructor of

    template <unsigned int other_factor, typename OtherName>
    fpga_policy(const fpga_policy<other_factor, OtherName>& other) : base(other.queue()){};

See my comment https://github.com/oneapi-src/oneDPL/pull/1154/files#r1318324154
Need to come up with something more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like that:

  • If we do that, this is not the design intent we want
  • If we don't do that - this is the API breaking change

{
::std::scoped_lock lock{this->mtx};
if (!this->q)
{
this->q.emplace(
# if _ONEDPL_FPGA_EMU
__dpl_sycl::__fpga_emulator_selector()
# else
__dpl_sycl::__fpga_selector()
# endif // _ONEDPL_FPGA_EMU
);
}
return *this->q;
}
};

#endif // _ONEDPL_FPGA_DEVICE
Expand Down