Skip to content

[SYCL][ESIMD] Add test proxy for move constructors #5132

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.

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

Merged
merged 7 commits into from
Jan 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sycl/ext/intel/experimental/esimd/detail/intrin.hpp>
#include <sycl/ext/intel/experimental/esimd/detail/memory_intrin.hpp>
#include <sycl/ext/intel/experimental/esimd/detail/sycl_util.hpp>
#include <sycl/ext/intel/experimental/esimd/detail/test_proxy.hpp>
#include <sycl/ext/intel/experimental/esimd/detail/type_format.hpp>
#include <sycl/ext/intel/experimental/esimd/simd_view.hpp>

Expand Down Expand Up @@ -729,11 +730,17 @@ class simd_obj_impl {
__ESIMD_DEF_SIMD_OBJ_IMPL_OPASSIGN(/, /=, __ESIMD_ARITH_OP_FILTER)
#undef __ESIMD_ARITH_OP_FILTER

// Getter for the test proxy member, if enabled
__ESIMD_DECLARE_TEST_PROXY_ACCESS

private:
// The underlying data for this vector.
raw_vector_type M_data;

protected:
// The test proxy if enabled
__ESIMD_DECLARE_TEST_PROXY

void set(const raw_vector_type &Val) {
#ifndef __SYCL_DEVICE_ONLY__
M_data = Val;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#pragma once

#include <sycl/ext/intel/experimental/esimd/detail/intrin.hpp>
#include <sycl/ext/intel/experimental/esimd/detail/test_proxy.hpp>
#include <sycl/ext/intel/experimental/esimd/detail/type_format.hpp>

__SYCL_INLINE_NAMESPACE(cl) {
Expand Down Expand Up @@ -305,8 +306,12 @@ class simd_view_impl {
Derived &operator=(const value_type &Val) { return write(Val); }

/// Move assignment operator.
Derived &operator=(Derived &&Other) { return write(Other.read()); }
Derived &operator=(Derived &&Other) {
__esimd_move_test_proxy(Other);
return write(Other.read());
}
simd_view_impl &operator=(simd_view_impl &&Other) {
__esimd_move_test_proxy(Other);
return write(Other.read());
}

Expand Down Expand Up @@ -495,10 +500,17 @@ class simd_view_impl {
return read().all();
}

public:
// Getter for the test proxy member, if enabled
__ESIMD_DECLARE_TEST_PROXY_ACCESS

protected:
// The reference to the base object, which must be a simd object
BaseTy &M_base;

// The test proxy if enabled
__ESIMD_DECLARE_TEST_PROXY

// The region applied on the base object. Its type could be
// - region1d_t
// - region2d_t
Expand Down
105 changes: 105 additions & 0 deletions sycl/include/sycl/ext/intel/experimental/esimd/detail/test_proxy.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
//==-------------- test_proxy.hpp - DPC++ Explicit SIMD API ----------------==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// Test proxy to differentiate move and copy constructor calls
//===----------------------------------------------------------------------===//

#pragma once

// The test proxy if solely for the test purposes, so it's off by default, with
// no any code generated. It is enabled only if the __ESIMD_ENABLE_TEST_PROXY
// macro is defined.
// It's expected for the proxy class to be available in device code, so that it
// could be incorporated into the ESIMD API classes. Though there is no reason
// to limit it to the __SYCL_DEVICE_ONLY__.
#ifndef __ESIMD_ENABLE_TEST_PROXY
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not quite clear if the test proxy is going to be used in device code - please clarify, and if yes - how. Otherwise, the above #ifndef can be changed to #if !defined(__ESIMD_ENABLE_TEST_PROXY) || defined (__SYCL_DEVICE_ONLY__)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's expected for the proxy class to be available in device code, so that it could be incorporated into the ESIMD API classes. See __ESIMD_DECLARE_TEST_PROXY_ACCESS usage.
I've added a comment to the code, please let me know if there are any other concerns


// No code generation by default
#define __ESIMD_DECLARE_TEST_PROXY
#define __ESIMD_DECLARE_TEST_PROXY_ACCESS
#define __esimd_move_test_proxy(other)

#else

// Declare the class attribute
//
// We are using non static data member initialization approach to force
// the value required. Initialization will take place even if no
// default/copy/move constructor of the test_proxy class was explcitly
// called by any of the user-defined constructors of the proxy target
#define __ESIMD_DECLARE_TEST_PROXY \
esimd::detail::test::test_proxy M_testProxy = \
esimd::detail::test::test_proxy();

// Declare the getter to access the proxy from the tests
#define __ESIMD_DECLARE_TEST_PROXY_ACCESS \
const auto &get_test_proxy() const { return M_testProxy; }

// Test proxy will be handled in a proper way by default/implicit move
// constructors and move operators.
// Still the user-defined constructors or move operators should explicitly state
// what to do with each of class atributes, so a proper wrapper required
//
// We are using a simple do-while trick to make sure no code breakage could
// possibly occur in case macro becomes multistatement (PRE10-C in SEI CERT C)
#define __esimd_move_test_proxy(other) \
do { \
M_testProxy = std::move(other.M_testProxy); \
} while (false)

__SYCL_INLINE_NAMESPACE(cl) {
namespace sycl::ext::intel::experimental::esimd::detail::test {

// The test_proxy class.
// Being intended solely for the test purposes, it is enabled only if the
// __ESIMD_ENABLE_TEST_PROXY macro is defined, which is off by default.
//
// This is a helper class for tests to differentiate between the copy
// constructor/assignment and the move constructor/assignment calls,
// as the copy constructor works as the default fallback for every case with
// move constructor disabled or not provided
//
// It is expected for the class with the test proxy (the class under test) to:
// - provide the get_test_proxy() method
// - properly handle moving the test_proxy member in user-defined move
// constructors and user-defined assignment operators
//
// Therefore the following expression is expected to return `true` only if the
// move constructor or move operator was called for the instance of the class
// under test:
// instance.get_test_proxy().was_move_destination()
//
class test_proxy {
// Define the default value to use for every constructor
bool M_move_destination = false;

public:
test_proxy() { __esimd_dbg_print(test_proxy()); }

test_proxy(const test_proxy &) {
__esimd_dbg_print(test_proxy(const test_proxy &other));
}
test_proxy(test_proxy &&) {
__esimd_dbg_print(test_proxy(test_proxy && other));
M_move_destination = true;
}
test_proxy &operator=(const test_proxy &) {
__esimd_dbg_print(test_proxy::operator=(const test_proxy &other));
return *this;
}
test_proxy &operator=(test_proxy &&) {
__esimd_dbg_print(test_proxy::operator=(test_proxy &&other));
M_move_destination = true;
return *this;
}
bool was_move_destination() const { return M_move_destination; }
};

} // namespace sycl::ext::intel::experimental::esimd::detail::test
} // __SYCL_INLINE_NAMESPACE(cl)

#endif // __ESIMD_ENABLE_TEST_PROXY