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 all 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
66 changes: 59 additions & 7 deletions tests/accessor/accessor_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,8 @@ struct tag_factory<accessor_type::host_accessor> {
* @param testing_acc Instance of TestingAccT that were constructed with default
* constructor
* @param res_acc Instance of result accessor
* @param check_iterator_methods Flag to avoid undefined behavior on access to
* uninitialized underlying buffer
*/
template <typename TestingAccT, typename ResultAccT>
void check_empty_accessor_constructor_post_conditions(
Expand Down Expand Up @@ -391,11 +393,57 @@ void check_def_constructor(GetAccFunctorT get_accessor_functor) {

auto acc = get_accessor_functor();
if constexpr (AccType != accessor_type::host_accessor) {
// Disable checking iteration methods with empty device accessor
// to avoid undefined behavior
bool check_iterator_methods = false;
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
false);
check_iterator_methods);
} else {
bool check_iterator_methods = true;
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
true);
check_iterator_methods);
}

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();
constexpr int buf_dims = (0 == Dimension) ? 1 : Dimension;
auto r = util::get_cts_object::range<buf_dims>::get(0, 0, 0);
sycl::buffer<DataT, buf_dims> 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.

// Disable checking iteration methods with empty device accessor
// to avoid undefined behavior
bool check_iterator_methods = false;
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
check_iterator_methods);
} else {
bool check_iterator_methods = true;
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
check_iterator_methods);
}

for (size_t i = 0; i < conditions_checks_size; i++) {
Expand Down Expand Up @@ -437,23 +485,27 @@ void check_zero_length_buffer_constructor(GetAccFunctorT get_accessor_functor) {
sycl::accessor<bool, 1, sycl::access_mode::read_write, Target>
res_acc(res_buf, cgh);
auto acc = get_accessor_functor(data_buf, cgh);
// Disable checking iteration methods with empty device accessor
// to avoid undefined behavior
bool check_iterator_methods = false;
if constexpr (Target == sycl::target::host_task) {
cgh.host_task([=] {
check_empty_accessor_constructor_post_conditions(acc, res_acc,
false);
check_empty_accessor_constructor_post_conditions(
acc, res_acc, check_iterator_methods);
});
} 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,
false);
check_empty_accessor_constructor_post_conditions(
acc, res_acc, check_iterator_methods);
});
}
})
.wait_and_throw();
} else {
auto acc = get_accessor_functor(data_buf);
bool check_iterator_methods = true;
check_empty_accessor_constructor_post_conditions(acc, conditions_check,
true);
check_iterator_methods);
}

for (size_t i = 0; i < conditions_checks_size; i++) {
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 @@ -43,12 +43,12 @@ void test_placeholder_zero_length_buffer_constructor(

SECTION(section_name) {
constexpr int dim_buf = (0 == Dimension) ? 1 : Dimension;
auto get_acc_functor = [](sycl::buffer<DataT, dim_buf>& data_buf,
sycl::handler& cgh) {
auto get_acc_functor = [](sycl::buffer<DataT, dim_buf>& 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
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
10 changes: 5 additions & 5 deletions tests/accessor/generic_accessor_placeholder_zero_length_buffer_range_offset_constructor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,14 @@ void test_placeholder_zero_length_buffser_range_offset_constructor(
"From zero-length buffer, range and offset placeholder constructor");

SECTION(section_name) {
auto get_acc_functor = [r_zero, offset](
sycl::buffer<DataT, Dimension>& data_buf,
sycl::handler& cgh) {
auto get_acc_functor = [r_zero,
offset](sycl::buffer<DataT, Dimension>& data_buf) {
return sycl::accessor<DataT, Dimension, AccessMode, Target>(
data_buf, r_zero, offset);
};
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