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.

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

Various smaller bugfixes in tests #870

Merged
merged 20 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0f2a228
Fix: get_multi_ptr() is only defined for device- and local accessors
fknorr Feb 17, 2024
363c209
Fix: legacy accessors do not have member function size()
fknorr Feb 17, 2024
e6aed12
Fix: Typo in function name
fknorr Feb 17, 2024
13b2277
Fix: `id<1> == int` is ambiguous
fknorr Feb 17, 2024
ad599db
Fix: info::device::image_max_array_size is not part of the Spec
fknorr Feb 17, 2024
53fd92f
Fix: info::device::opencl_c_version is not part of the Spec
fknorr Feb 17, 2024
05c1a78
Fix: sycl::runtime_error is not part of the Spec
fknorr Feb 17, 2024
cb520fd
Fix: sycl::exception is not default-constructible
fknorr Feb 17, 2024
9fe6b29
Fix: `sycl::id<1> - 1` is ambiguous
fknorr Feb 17, 2024
52f993b
Fix: remove `template` from non-dependent expression
fknorr Feb 17, 2024
f9f2894
Fix: marray<bool> does not have operator++/--
fknorr Feb 17, 2024
54d8b79
Fix: Avoid multiple definitions of kernel names
fknorr Feb 17, 2024
502a9bf
Fix: Regex typo in scalars/CMakeLists
fknorr Feb 17, 2024
9008312
Fix: sycl::vec cannot load/store from accessor or load from non-const…
fknorr Feb 17, 2024
0fbac0d
Fix: vec<bool> does not provide operator++/--
fknorr Feb 17, 2024
e931257
Fix: Call to erase_if is ambiguous with C++20
fknorr Feb 17, 2024
8764459
Fix: correctly reset array in test_scalars
fknorr Feb 17, 2024
8a68532
Address reviewer comments on fixes
fknorr Jun 13, 2024
004bcbf
Fix formatting in updated tests
fknorr Jun 19, 2024
7d63aba
Merge branch 'SYCL-2020' into various-fixes
bader Jun 19, 2024
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
22 changes: 21 additions & 1 deletion tests/accessor/generic_accessor_api_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,29 @@ void test_accessor_methods(const AccT &accessor,
#endif
}

template <typename Accessor>
extern const sycl::target accessor_target_v;
Copy link
Member

Choose a reason for hiding this comment

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

Why this extern definition?

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 the base template that's being specialized below. For a variable template marking it extern allows me to omit the (invalid) definition and cause a compiler error when it's used in a constexpr context.

Copy link
Member

Choose a reason for hiding this comment

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

Why not. :-)


template <typename T, int Dims, sycl::access_mode Mode, sycl::target Target,
sycl::access::placeholder IsPlaceholder>
inline constexpr sycl::target
accessor_target_v<sycl::accessor<T, Dims, Mode, Target, IsPlaceholder>> =
Target;

template <typename T, int Dims>
inline constexpr sycl::target accessor_target_v<sycl::local_accessor<T, Dims>> =
sycl::target::device;

template <typename T, int Dims, sycl::access_mode Mode>
inline constexpr sycl::target
accessor_target_v<sycl::host_accessor<T, Dims, Mode>> =
sycl::target::host_buffer;

template <typename T, typename AccT>
void test_accessor_ptr_host(AccT &accessor, T expected_data) {
{
// get_multi_ptr is only defined for device accessors and local accessors
if constexpr (accessor_target_v<std::remove_cv_t<AccT>> ==
sycl::target::device) {
INFO("check get_multi_ptr() method");
auto acc_multi_ptr_no =
accessor.template get_multi_ptr<sycl::access::decorated::no>();
Expand Down
5 changes: 4 additions & 1 deletion tests/accessor_legacy/accessor_api_buffer_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ class check_buffer_accessor_api_methods {
"accessor does not properly report placeholder status");
}
}
{
// legacy accessors do not have the size() member function
if constexpr (target != sycl::access::target::constant_buffer &&
target != sycl::access::target::local &&
target != sycl::access::target::host_buffer) {
/** check size() method
*/
auto accessorCount = accessor.size();
Expand Down
13 changes: 1 addition & 12 deletions tests/accessor_legacy/accessor_api_local_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,8 @@ class check_local_accessor_api_methods {
util::get_cts_object::range<data_dim<dims>::value>::get(1, 1, 1);
error_buffer_t errorBuffer(errors.get(), sycl::range<1>(2));

queue.submit([&](sycl::handler &h) {
queue.submit([&](sycl::handler& h) {
auto acc = make_local_accessor_generic<T, dims, mode>(range, h);
{
/** check size() method
*/
auto accessorCount = acc.size();
check_return_type<size_t>(log, accessorCount, "size()");
const auto expectedCount = ((dims == 0) ? 1 : count);
if (accessorCount != expectedCount) {
fail_for_accessor<T, dims, mode, target>(log, typeName,
"accessor does not return the correct count");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What is the problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests the legacy accessor<target::local> specialization, which does not have a size() member.

{
/** check get_count() method
*/
Expand Down
5 changes: 3 additions & 2 deletions tests/atomic_ref/atomic_ref_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ inline bool memory_order_is_supported(sycl::queue& q,
return it != memory_orders_supported.end();
}

inline bool memory_scope_is_suppoted(sycl::queue& q, sycl::memory_scope scope) {
inline bool memory_scope_is_supported(sycl::queue& q,
sycl::memory_scope scope) {
std::vector<sycl::memory_scope> memory_scopes_supported =
q.get_device()
.get_info<sycl::info::device::atomic_memory_scope_capabilities>();
Expand All @@ -266,7 +267,7 @@ inline bool memory_order_and_scope_are_supported(sycl::queue& q,
sycl::memory_order order,
sycl::memory_scope scope) {
return memory_order_is_supported(q, order) &&
memory_scope_is_suppoted(q, scope);
memory_scope_is_supported(q, scope);
}

inline bool memory_order_and_scope_are_not_supported(sycl::queue& q,
Expand Down
4 changes: 2 additions & 2 deletions tests/atomic_ref_stress/atomic_ref_stress_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,15 +172,15 @@ class aquire_release {
refA.store(0);
refB.store(0);
sycl::group_barrier(item.get_group());
if (item.get_local_id() == 0) {
if (item.get_local_id() == sycl::id(0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@AlexeySachkov AlexeySachkov Feb 23, 2024

Choose a reason for hiding this comment

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

And that is half of the problem, because id is also implicitly convertible to size_t. See godbolt. If you switch to gcc and member operation you will get a bit more detailed error message. But I was also surprised by this, apparently I'm not enough familiar with those overload resolution rules.

In our implementation we use some hackery to avoid ambiguity here

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So either the special-casing needs to be in the spec, or this explicit construction is correct.

x = refA.load();
refB.store(1);
} else {
y = refB.load();
refA.store(1);
}
sycl::group_barrier(item.get_group());
if (item.get_local_id() == 0)
if (item.get_local_id() == sycl::id(0))
Copy link
Member

Choose a reason for hiding this comment

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

Idem.

res_acc[item.get_group_linear_id()] = !(x == 1 && y == 1);
});
});
Expand Down
3 changes: 0 additions & 3 deletions tests/device/device_info.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ TEST_CASE("device info", "[device]") {
check_get_info_param<sycl::info::device::image3d_max_depth, size_t>(dev);
check_get_info_param<sycl::info::device::image_max_buffer_size, size_t>(
dev);
check_get_info_param<sycl::info::device::image_max_array_size, size_t>(dev);
check_get_info_param<sycl::info::device::max_samplers, uint32_t>(dev);
check_get_info_param<sycl::info::device::max_parameter_size, size_t>(dev);
check_get_info_param<sycl::info::device::mem_base_addr_align, uint32_t>(
Expand Down Expand Up @@ -223,8 +222,6 @@ TEST_CASE("device info", "[device]") {
check_get_info_param<sycl::info::device::driver_version, std::string>(dev);
check_get_info_param<sycl::info::device::profile, std::string>(dev);
check_get_info_param<sycl::info::device::version, std::string>(dev);
check_get_info_param<sycl::info::device::opencl_c_version, std::string>(
dev);
check_get_info_param<sycl::info::device::backend_version, std::string>(dev);

check_get_info_param<sycl::info::device::aspects,
Expand Down
12 changes: 6 additions & 6 deletions tests/device_selector/device_selector_predefined.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ TEST_CASE("predefined selectors", "[device_selector]") {
try {
auto cpuDevice = util::get_cts_object::device(cpuSelector);
FAIL("selected a CPU device when none are available");
} catch (const sycl::runtime_error &e) {
// sycl::runtime_error is expected
} catch (const sycl::exception& e) {
// sycl::exception is expected
} catch (...) {
FAIL("wrong error thrown when no CPU devices are available");
}
Expand All @@ -90,8 +90,8 @@ TEST_CASE("predefined selectors", "[device_selector]") {
try {
auto gpuDevice = util::get_cts_object::device(gpuSelector);
FAIL("selected a GPU device when none are available");
} catch (const sycl::runtime_error &e) {
// sycl::runtime_error is expected
} catch (const sycl::exception& e) {
// sycl::exception is expected
} catch (...) {
FAIL("wrong error thrown when no GPU devices are available");
}
Expand All @@ -108,8 +108,8 @@ TEST_CASE("predefined selectors", "[device_selector]") {
auto acceleratorDevice =
util::get_cts_object::device(acceleratorSelector);
FAIL("selected an accelerator device when none are available");
} catch (const sycl::runtime_error &e) {
// sycl::runtime_error is expected
} catch (const sycl::exception& e) {
// sycl::exception is expected
} catch (...) {
FAIL("wrong error thrown when no accelerator devices are available");
}
Expand Down
8 changes: 7 additions & 1 deletion tests/exceptions/exceptions_constructors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,14 @@ TEST_CASE("Constructors for sycl::exception with sycl::errc error codes",
check_exception(copy, errcode, sycl::sycl_category());
}
SECTION("operator=(const exception& other)") {
// sycl::exception is not default-constructible, so explicitly construct
Copy link
Member

Choose a reason for hiding this comment

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

Good point!

// `copy` with an error code that is different from `errcode`
std::error_code other_errc(errcode == sycl::errc::success
? sycl::errc::runtime
: sycl::errc::success);

sycl::exception e(std_errc);
sycl::exception copy;
sycl::exception copy(other_errc);
CHECK_NOTHROW((copy = e));
check_exception(copy, errcode, sycl::sycl_category());
}
Expand Down
3 changes: 2 additions & 1 deletion tests/id/id.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,8 @@ TEMPLATE_TEST_CASE_SIG("id can be implicitly conversion-constructed from item",
q.submit([r, &result_buf](sycl::handler& cgh) {
sycl::accessor result{result_buf, cgh, sycl::write_only};
cgh.parallel_for<kernel_id<D>>(r, [=](sycl::item<D> itm) {
if (itm.get_id() == id<D>{r} - 1) {
// `ul` suffix is necessary to resolve ambiguity for id<1>
if (itm.get_id() == id<D>{r} - 1ul) {
Copy link
Member

Choose a reason for hiding this comment

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

Why l?
Just u does not work?
The real type would be uz but it is C++23.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but size_t is at least unsigned long on all relevant architectures, no?

// Use assignment operator to trigger implicit conversion
result[0] = itm;
}
Expand Down
4 changes: 2 additions & 2 deletions tests/kernel/kernel_semantics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ TEST_CASE("kernel common reference semantics", "[kernel]") {

sycl::kernel kernel_0 =
sycl::get_kernel_bundle<sycl::bundle_state::executable>(context_0)
.template get_kernel(sycl::get_kernel_id<k_name>());
.get_kernel(sycl::get_kernel_id<k_name>());
sycl::kernel kernel_1 =
sycl::get_kernel_bundle<sycl::bundle_state::executable>(context_1)
.template get_kernel(sycl::get_kernel_id<k_name_other>());
.get_kernel(sycl::get_kernel_id<k_name_other>());
common_reference_semantics::check_host<storage>(kernel_0, kernel_1, "kernel");
sycl_cts::tests::kernel_bundle::define_kernel<k_name>(queue_0);
sycl_cts::tests::kernel_bundle::define_kernel<k_name_other>(queue_1);
Expand Down
30 changes: 20 additions & 10 deletions tests/marray/marray_operators.h
Original file line number Diff line number Diff line change
Expand Up @@ -822,11 +822,18 @@ class check_marray_pre_unary_operators_for_type {
INFO("prefix unary operators for type \"" << type_name << "\": ");

const auto num_elements = marray_common::get_num_elements();

static const auto unary_operators =
named_type_pack<op_upos, op_uneg, op_pre_inc, op_pre_dec, op_lnot,
op_bnot>::generate("unary +", "unary -", "pre ++",
"pre --", "!", "~");
static const auto unary_operators = [] {
if constexpr (std::is_same_v<DataT, bool>) {
// marray<bool> does not have operator++/--
return named_type_pack<op_upos, op_uneg, op_lnot, op_bnot>::generate(
"unary +", "unary -", "!", "~");
} else {
return named_type_pack<op_upos, op_uneg, op_pre_inc, op_pre_dec,
op_lnot, op_bnot>::generate("unary +", "unary -",
"pre ++", "pre --",
"!", "~");
}
}();
for_all_combinations<run_unary, DataT>(num_elements, unary_operators);
}
};
Expand All @@ -839,11 +846,14 @@ class check_marray_post_unary_operators_for_type {

const auto num_elements = marray_common::get_num_elements();

static const auto unary_post_operators =
named_type_pack<op_post_inc, op_post_dec>::generate("post ++",
"post --");
for_all_combinations<run_unary_post, DataT>(num_elements,
unary_post_operators);
// marray<bool> does not have operator++/--
if constexpr (!std::is_same_v<DataT, bool>) {
static const auto unary_post_operators =
named_type_pack<op_post_inc, op_post_dec>::generate("post ++",
"post --");
for_all_combinations<run_unary_post, DataT>(num_elements,
unary_post_operators);
}
}
};

Expand Down
11 changes: 8 additions & 3 deletions tests/namespace/namespace.cpp.in
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,14 @@

#include <iostream>

struct kernel_1;
struct kernel_2;
struct kernel_3;
// Multiple translation units are instantiated from this .in file and linked together, so we must
// ensure that kernel names are namespaced to disambiguate their definitions
namespace kernels CTS_NAMESPACE {
struct kernel_1;
struct kernel_2;
struct kernel_3;
}
using namespace kernels CTS_NAMESPACE;

/** Example program, adapted to use the CTS queue. */
static int test_namespace() {
Expand Down
2 changes: 1 addition & 1 deletion tests/scalars/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
file(GLOB test_cases_list *.cpp)

if(NOT SYCL_CTS_ENABLE_OPENCL_INTEROP_TESTS)
list(FILTER test_cases_list EXCLUDE REGEX scalars_interopability_types.cpp)
list(FILTER test_cases_list EXCLUDE REGEX scalars_interoperability_types.cpp)
endif()

add_cts_test(${test_cases_list})
3 changes: 2 additions & 1 deletion tests/scalars/scalars_fixed_width_types.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ TEST_CASE("Fixed width types size equality", "[scalars]") {
CHECK(results[i]);
}

std::iota(results.begin(), results.end(), false);
std::fill(results.begin(), results.end(), false);

{
sycl::buffer<bool, 1> res_buf(results.data(), {types_count});
queue.submit([&](sycl::handler& cgh) {
Expand Down
6 changes: 0 additions & 6 deletions tests/vector_load_store/generate_vector_load_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,25 +54,19 @@

cgh.single_task<class ${kernelName}>([=]() {
auto testVec${type_as_str}${size} = sycl::vec<${type}, ${size}>(${val});
testVec${type_as_str}${size}.load(0, inPtr${type_as_str}${size});
testVec${type_as_str}${size}.store(0, outPtr${type_as_str}${size});

auto multiPtrIn${type_as_str}${size} = inPtr${type_as_str}${size}.get_multi_ptr<${decorated}>();
sycl::global_ptr<const ${type}> constMultiPtrIn${type_as_str}${size} = multiPtrIn${type_as_str}${size};
auto multiPtrOut${type_as_str}${size} = outPtr${type_as_str}${size}.get_multi_ptr<${decorated}>();
testVec${type_as_str}${size}.load(0, multiPtrIn${type_as_str}${size});
testVec${type_as_str}${size}.load(0, constMultiPtrIn${type_as_str}${size});
testVec${type_as_str}${size}.store(0, multiPtrOut${type_as_str}${size});

auto cleanVec${type_as_str}${size} = sycl::vec<${type}, ${size}>(${val});
sycl::vec<${type}, ${size}> swizzledVec {cleanVec${type_as_str}${size}.template swizzle<${swizVals}>()};
swizzledVec.load(0, swizzleInPtr${type_as_str}${size});
swizzledVec.store(0, swizzleOutPtr${type_as_str}${size});

auto multiPtrInSwizzle${type_as_str}${size} = swizzleInPtr${type_as_str}${size}.get_multi_ptr<${decorated}>();
sycl::global_ptr<const ${type}> constMultiPtrInSwizzle${type_as_str}${size} = multiPtrInSwizzle${type_as_str}${size};
auto multiPtrOutSwizzle${type_as_str}${size} = swizzleOutPtr${type_as_str}${size}.get_multi_ptr<${decorated}>();
swizzledVec.load(0, multiPtrInSwizzle${type_as_str}${size});
swizzledVec.load(0, constMultiPtrInSwizzle${type_as_str}${size});
swizzledVec.store(0, multiPtrOutSwizzle${type_as_str}${size});
});
Expand Down
Loading
Loading