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

Jump to bottom

Fix placeholder accessor wrong checking #683

Merged
merged 7 commits into from
Jun 28, 2023
Merged

Fix placeholder accessor wrong checking #683

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9915ad
Cherry-picked PR #669 to avoid CI failures.
aleksmesh Jun 2, 2023
c081921
Fix placeholder accessor wrong checking
aleksmesh Jun 2, 2023
beff34a
Merge branch 'SYCL-2020' into fix_wrong_checking_for_placeholder_cons…
aleksmesh Jun 13, 2023
9e45d65
Apply comments
aleksmesh Jun 26, 2023
917baf6
Merge branch 'SYCL-2020' into fix_wrong_checking_for_placeholder_cons…
aleksmesh Jun 26, 2023
ccfcfae
Fix CI
aleksmesh Jun 27, 2023
3d739d5
Fix clang format
aleksmesh Jun 27, 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
110 changes: 75 additions & 35 deletions tests/accessor/accessor_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,18 @@ inline auto get_conformance_type_pack() {
#endif // SYCL_CTS_ENABLE_FULL_CONFORMANCE
}

template <template <typename, typename...> class action,
typename... actionArgsT>
void common_run_tests() {
const auto types = get_conformance_type_pack();
#if SYCL_CTS_ENABLE_FULL_CONFORMANCE
for_all_types_vectors_marray<action, actionArgsT...>(types);
#else
for_all_types<action, actionArgsT...>(types);
for_type_vectors_marray_reduced<action, int, actionArgsT...>("int");
#endif
}

/**
* @brief Factory function for getting type_pack with access modes values
*/
Expand Down Expand Up @@ -323,8 +335,8 @@ struct tag_factory<accessor_type::host_accessor> {
* @param res_acc Instance of result accessor
*/
template <typename TestingAccT, typename ResultAccT>
void check_empty_accessor_constructor_post_conditions(TestingAccT testing_acc,
ResultAccT res_acc) {
void check_empty_accessor_constructor_post_conditions(
TestingAccT testing_acc, ResultAccT res_acc, bool check_iterator_methods) {
size_t res_i = 0;
// (empty() == true)
res_acc[res_i++] = testing_acc.empty() == true;
Expand All @@ -334,11 +346,13 @@ void check_empty_accessor_constructor_post_conditions(TestingAccT testing_acc,
res_acc[res_i++] = testing_acc.size() == 0;
res_acc[res_i++] = testing_acc.max_size() == 0;

// The only iterator that can be obtained is nullptr
res_acc[res_i++] = testing_acc.begin() == testing_acc.end();
res_acc[res_i++] = testing_acc.cbegin() == testing_acc.cend();
res_acc[res_i++] = testing_acc.rbegin() == testing_acc.rend();
res_acc[res_i++] = testing_acc.crbegin() == testing_acc.crend();
if (check_iterator_methods) {
// The only iterator that can be obtained is nullptr
res_acc[res_i++] = testing_acc.begin() == testing_acc.end();
res_acc[res_i++] = testing_acc.cbegin() == testing_acc.cend();
res_acc[res_i++] = testing_acc.rbegin() == testing_acc.rend();
res_acc[res_i++] = testing_acc.crbegin() == testing_acc.crend();
}
}
// FIXME: re-enable when handler.host_task and sycl::errc is implemented in
// hipsycl and computcpp
Expand All @@ -362,32 +376,54 @@ void check_def_constructor(GetAccFunctorT get_accessor_functor) {
auto queue = once_per_unit::get_queue();
sycl::range<1> r(1);
const size_t conditions_checks_size = 8;
bool conditions_check[conditions_checks_size]{false};
bool conditions_check[conditions_checks_size];
std::fill(conditions_check, conditions_check + conditions_checks_size, true);

auto acc = get_accessor_functor();
if constexpr (AccType != accessor_type::host_accessor) {
sycl::buffer res_buf(conditions_check, sycl::range(conditions_checks_size));
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
false);
} else {
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
true);
}

queue
.submit([&](sycl::handler& cgh) {
sycl::accessor res_acc(res_buf, cgh);
auto acc = get_accessor_functor();
if (acc.is_placeholder()) {
cgh.require(acc);
}
if constexpr (Target == sycl::target::host_task) {
cgh.host_task([=] {
check_empty_accessor_constructor_post_conditions(acc, res_acc);
});
} else if constexpr (Target == sycl::target::device) {
cgh.parallel_for_work_group(r, [=](sycl::group<1>) {
check_empty_accessor_constructor_post_conditions(acc, res_acc);
});
}
})
.wait_and_throw();
for (size_t i = 0; i < conditions_checks_size; i++) {
CHECK(conditions_check[i]);
}
}

/**
* @brief Common function that constructs placeholder accessor with zero-length
* buffer and checks post-conditions
*
* @tparam AccType Type of the accessor
* @tparam DataT Type of underlying data
* @tparam Dimension Dimensions of the accessor
* @tparam AccessMode Access mode of the accessor
* @tparam Target Target of accessor
* @tparam GetAccFunctorT Type of functor for accessor creation
*/
template <accessor_type AccType, typename DataT, int Dimension,
sycl::access_mode AccessMode = sycl::access_mode::read_write,
sycl::target Target = sycl::target::device, typename GetAccFunctorT>
void check_zero_length_buffer_placeholder_constructor(
GetAccFunctorT get_accessor_functor) {
auto queue = once_per_unit::get_queue();
sycl::range<Dimension> r =
util::get_cts_object::range<Dimension>::get(0, 0, 0);
sycl::buffer<DataT, Dimension> data_buf(r);
const size_t conditions_checks_size = 8;
bool conditions_check[conditions_checks_size];
std::fill(conditions_check, conditions_check + conditions_checks_size, true);

auto acc = get_accessor_functor(data_buf);
if constexpr (AccType != accessor_type::host_accessor) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a difference for iterators between host and device?

Copy link
Contributor Author

@aleksmesh aleksmesh Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In paragraph 4.7.6.12. Common members for buffer and local accessors I found this:

iterator begin() const noexcept Returns an iterator to the first element of the memory this accessor may access.For a buffer accessor this is an iterator to the first element of the underlying buffer, unless this is a ranged accessor in which case it is an iterator to first element within the accessor’s range.For accessor and local_accessor, this function may only be called from within a command.

Also in paragraph 4.9.4.1. SYCL functions for adding requirements we have restriction for empty accessors:

template <typename DataT, int Dimensions, access_mode AccessMode, target AccessTarget, access::placeholder IsPlaceholder> void require( accessor<DataT, Dimensions, AccessMode, AccessTarget, IsPlaceholder> acc) Requires access to the memory object associated with the accessor.The command group now has a requirement to gain access to the given memory object before executing the kernel. If the accessor has already been registered with the command group, calling this function has no effect.Throws exception with the errc::invalid error code if (acc.empty() == true).

So we can't check iteration methods of empty placeholder sycl::accessor and we can check sycl::host_accessor iteration methods.

Copy link
Member

Choose a reason for hiding this comment

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

Could you summarize this in some comments around the code? We might forget about the rationale... :-)

Copy link
Contributor Author

@aleksmesh aleksmesh Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@keryell Comments added.

check_empty_accessor_constructor_post_conditions(acc, conditions_check,
false);
} else {
auto acc = get_accessor_functor();
check_empty_accessor_constructor_post_conditions(acc, conditions_check);
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
true);
}

for (size_t i = 0; i < conditions_checks_size; i++) {
Expand Down Expand Up @@ -415,29 +451,33 @@ void check_zero_length_buffer_constructor(GetAccFunctorT get_accessor_functor) {
util::get_cts_object::range<Dimension>::get(0, 0, 0);
sycl::buffer<DataT, Dimension> data_buf(r);
const size_t conditions_checks_size = 8;
bool conditions_check[conditions_checks_size]{false};
bool conditions_check[conditions_checks_size];
std::fill(conditions_check, conditions_check + conditions_checks_size, true);

if constexpr (AccType != accessor_type::host_accessor) {
sycl::buffer res_buf(conditions_check, sycl::range(conditions_checks_size));

queue
.submit([&](sycl::handler& cgh) {
sycl::accessor res_acc(res_buf);
sycl::accessor res_acc(res_buf, cgh);
auto acc = get_accessor_functor(data_buf, cgh);
if constexpr (Target == sycl::target::host_task) {
cgh.host_task([=] {
check_empty_accessor_constructor_post_conditions(acc, res_acc);
check_empty_accessor_constructor_post_conditions(acc, res_acc,
false);
});
} else if constexpr (Target == sycl::target::device) {
cgh.parallel_for_work_group(r, [=](sycl::group<Dimension>) {
check_empty_accessor_constructor_post_conditions(acc, res_acc);
check_empty_accessor_constructor_post_conditions(acc, res_acc,
false);
});
}
})
.wait_and_throw();
} else {
auto acc = get_accessor_functor(data_buf);
check_empty_accessor_constructor_post_conditions(acc, conditions_check);
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
true);
}

for (size_t i = 0; i < conditions_checks_size; i++) {
Expand Down Expand Up @@ -643,7 +683,7 @@ void check_common_constructor(const sycl::range<Dimension>& r,

queue
.submit([&](sycl::handler& cgh) {
sycl::accessor res_acc(res_buf);
sycl::accessor res_acc(res_buf, cgh);
auto acc = get_accessor_functor(data_buf, cgh);

if constexpr (AccType == accessor_type::generic_accessor) {
Expand Down
6 changes: 2 additions & 4 deletions tests/accessor/accessor_default_values_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ namespace accessor_default_values_test_core {
using namespace sycl_cts;

DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Accessors constructor default values test core types.", "[accessor]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_tests>(types);
});
("Accessors constructor default values test core types.",
"[accessor]")({ common_run_tests<run_tests>(); });

} // namespace accessor_default_values_test_core
16 changes: 4 additions & 12 deletions tests/accessor/accessor_exceptions_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,14 @@ using namespace sycl_cts;

DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp, DPCPP)
("Generic sycl::accessor constructor exceptions test. Core types.",
"[accessor]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_tests_with_types, generic_accessor>(types);
});
"[accessor]")({ common_run_tests<run_tests, generic_accessor>(); });

DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp, DPCPP)
("sycl::local_accessor constructor exceptions test. Core types.",
"[accessor]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_tests_with_types, local_accessor>(types);
});
"[accessor]")({ common_run_tests<run_tests, local_accessor>(); });

DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp, DPCPP)
("sycl::host_accessor constructor exceptions test. Core types.", "[accessor]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_tests_with_types, host_accessor>(types);
});
("sycl::host_accessor constructor exceptions test. Core types.",
"[accessor]")({ common_run_tests<run_tests, host_accessor>(); });

} // namespace accessor_exceptions_test_core
9 changes: 3 additions & 6 deletions tests/accessor/accessor_implicit_conversions_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,19 @@ namespace accessor_implicit_conversions_core {
DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Generic sycl::accessor implicit conversion. core types",
"[accessor][generic_accessor][conversion][core]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_test_generic>(types);
common_run_tests<run_test_generic>();
});

DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("The sycl::local_accessor implicit conversion. core types",
"[accessor][local_accessor][conversion][core]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_test_local>(types);
common_run_tests<run_test_local>();
});

DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("The sycl::host_accessor implicit conversion. core types",
"[accessor][host_accessor][conversion][core]")({
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_test_host>(types);
common_run_tests<run_test_host>();
});

} // namespace accessor_implicit_conversions_core
3 changes: 1 addition & 2 deletions tests/accessor/generic_accessor_api_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ namespace generic_accessor_api_core {
DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp, DPCPP)
("Generic sycl::accessor api. core types", "[accessor]")({
using namespace generic_accessor_api_common;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_generic_api_for_type>(types);
common_run_tests<run_generic_api_for_type>();
});

} // namespace generic_accessor_api_core
4 changes: 1 addition & 3 deletions tests/accessor/generic_accessor_common_buffer_constructors_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ namespace generic_accessor_common_buffer_constructors_core {
DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp, DPCPP)
("Generic sycl::accessor buffer constructors. core types", "[accessor]")({
using namespace generic_accessor_common_buffer_constructors;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_generic_common_buffer_constructors_test>(
types);
common_run_tests<run_generic_common_buffer_constructors_test>();
});

} // namespace generic_accessor_common_buffer_constructors_core
3 changes: 1 addition & 2 deletions tests/accessor/generic_accessor_def_constructor_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ namespace generic_accessor_def_constructor_core {
DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Generic sycl::accessor def constructors. core types", "[accessor]")({
using namespace generic_accessor_def_constructor;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_generic_def_constructor_test>(types);
common_run_tests<run_generic_def_constructor_test>();
});

} // namespace generic_accessor_def_constructor_core
3 changes: 1 addition & 2 deletions tests/accessor/generic_accessor_linearization_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ namespace generic_accessor_linearization_core {
DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Generic sycl::accessor linearization test. core types", "[accessor]")({
using namespace generic_accessor_linearization;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_generic_linearization_for_type>(types);
common_run_tests<run_generic_linearization_for_type>();
});

} // namespace generic_accessor_linearization_core
4 changes: 1 addition & 3 deletions tests/accessor/generic_accessor_placeholder_buffer_constructor_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ namespace generic_accessor_placeholder_buffer_constructor_core {
DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Generic sycl::accessor placeholder constructors. core types", "[accessor]")({
using namespace generic_accessor_placeholder_buffer_constructor;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<run_generic_placeholder_buffer_constructor_test>(
types);
common_run_tests<run_generic_placeholder_buffer_constructor_test>();
});

} // namespace generic_accessor_placeholder_buffer_constructor_core
5 changes: 2 additions & 3 deletions tests/accessor/generic_accessor_placeholder_buffer_constructor_exceptions_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Generic sycl::accessor placeholder buffer constructor exceptions. core types",
"[accessor]")({
using namespace generic_accessor_placeholder_buffer_constructor_exceptions;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<
run_generic_placeholder_buffer_constructor_exceptions_test>(types);
common_run_tests<
run_generic_placeholder_buffer_constructor_exceptions_test>();
});

} // namespace generic_accessor_placeholder_buffer_constructor_exceptions_core
4 changes: 1 addition & 3 deletions tests/accessor/generic_accessor_placeholder_buffer_range_constructor_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
("Generic sycl::accessor placeholder buffer range constructor. core types",
"[accessor]")({
using namespace generic_accessor_placeholder_buffer_range_constructor;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<
run_generic_placeholder_buffer_range_constructor_test>(types);
common_run_tests<run_generic_placeholder_buffer_range_constructor_test>();
});

} // namespace generic_accessor_placeholder_buffer_range_constructor_core
5 changes: 2 additions & 3 deletions tests/accessor/generic_accessor_placeholder_buffer_range_constructor_exceptions_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
"types",
"[accessor]")({
using namespace generic_accessor_placeholder_buffer_range_constructor_exceptions;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<
run_generic_placeholder_buffer_range_constructor_exceptions_test>(types);
common_run_tests<
run_generic_placeholder_buffer_range_constructor_exceptions_test>();
});

} // namespace
Expand Down
5 changes: 2 additions & 3 deletions tests/accessor/generic_accessor_placeholder_buffer_range_offset_constructor_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
"types",
"[accessor]")({
using namespace generic_accessor_placeholder_buffer_range_offset_constructor;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<
run_generic_placeholder_buffer_range_offset_constructor_test>(types);
common_run_tests<
run_generic_placeholder_buffer_range_offset_constructor_test>();
});

} // namespace
Expand Down
6 changes: 2 additions & 4 deletions ...accessor/generic_accessor_placeholder_buffer_range_offset_constructor_exceptions_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,8 @@ DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
"exceptions. core types",
"[accessor]")({
using namespace generic_accessor_placeholder_buffer_range_offset_constructor_exceptions;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<
run_generic_placeholder_buffer_range_offset_constructor_exceptions_test>(
types);
common_run_tests<
run_generic_placeholder_buffer_range_offset_constructor_exceptions_test>();
});

} // namespace
Expand Down
8 changes: 4 additions & 4 deletions tests/accessor/generic_accessor_placeholder_zero_length_buffer_constructor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ void test_placeholder_zero_length_buffer_constructor(
"From zero-length buffer placeholder constructor");

SECTION(section_name) {
auto get_acc_functor = [](sycl::buffer<DataT, Dimension>& data_buf,
sycl::handler& cgh) {
auto get_acc_functor = [](sycl::buffer<DataT, Dimension>& data_buf) {
return sycl::accessor<DataT, Dimension, AccessMode, Target>(data_buf);
};
check_zero_length_buffer_constructor<AccType, DataT, Dimension, AccessMode,
Target>(get_acc_functor);
check_zero_length_buffer_placeholder_constructor<AccType, DataT, Dimension,
AccessMode, Target>(
get_acc_functor);
}
}

Expand Down
4 changes: 1 addition & 3 deletions tests/accessor/generic_accessor_placeholder_zero_length_buffer_constructor_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ DISABLED_FOR_TEST_CASE(hipSYCL, ComputeCpp)
"types",
"[accessor]")({
using namespace generic_accessor_placeholder_zero_length_buffer_constructor;
const auto types = get_conformance_type_pack();
for_all_types_vectors_marray<
run_generic_placeholder_zero_length_buffer_constructor>(types);
common_run_tests<run_generic_placeholder_zero_length_buffer_constructor>();
});

} // namespace generic_accessor_placeholder_zero_length_buffer_constructor_core
8 changes: 4 additions & 4 deletions tests/accessor/generic_accessor_placeholder_zero_length_buffer_range_constructor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ void test_placeholder_zero_length_buffer_range_constructor(
"From zero-length buffer and range placeholder constructor");

SECTION(section_name) {
auto get_acc_functor = [r_zero](sycl::buffer<DataT, Dimension>& data_buf,
sycl::handler& cgh) {
auto get_acc_functor = [r_zero](sycl::buffer<DataT, Dimension>& data_buf) {
return sycl::accessor<DataT, Dimension, AccessMode, Target>(data_buf,
r_zero);
};
check_zero_length_buffer_constructor<AccType, DataT, Dimension, AccessMode,
Target>(get_acc_functor);
check_zero_length_buffer_placeholder_constructor<AccType, DataT, Dimension,
AccessMode, Target>(
get_acc_functor);
}
}

Expand Down
Loading