Skip to content

[SYCL] Implement buffer constructor with iterators in accordance with spec #66

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

Merged
merged 1 commit into from
Apr 9, 2019
Merged

[SYCL] Implement buffer constructor with iterators in accordance with spec #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 43 additions & 14 deletions sycl/include/CL/sycl/detail/buffer_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,23 +99,52 @@ template <typename AllocatorT> class buffer_impl {
}
}

template <typename Iterator> struct is_const_iterator {
using pointer = typename std::iterator_traits<Iterator>::pointer;
static constexpr bool value =
std::is_const<typename std::remove_pointer<pointer>::type>::value;
};

template <typename Iterator>
using EnableIfConstIterator =
typename std::enable_if<is_const_iterator<Iterator>::value,
Iterator>::type;

template <typename Iterator>
using EnableIfNotConstIterator =
typename std::enable_if<!is_const_iterator<Iterator>::value,
Iterator>::type;

template <class InputIterator>
buffer_impl(InputIterator first, InputIterator last, const size_t sizeInBytes,
const property_list &propList,
buffer_impl(EnableIfNotConstIterator<InputIterator> first, InputIterator last,
Copy link
Contributor

Choose a reason for hiding this comment

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

EnableIfNotConstIterator<InputIterator> here makes the reading of the constructor unreadable.

What about something like

   template <class InputIterator, typename EnableIf = EnableIfNotConstIterator<InputIterator>>
   buffer_impl(InputIterator first, InputIterator last,

so you clearly separate the SFINAE mechanics from the argument declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, it will not work because I will get two function templates that differ only in their default template arguments. (c'tors with EnableIfNotConstIterator and EnableIfConstIterator). You can see note from here: https://en.cppreference.com/w/cpp/types/enable_if

Copy link
Contributor

@keryell keryell Apr 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, interesting, you found the exact explanation! Great.
By looking at our code base, we handled the same use case in a similar way as you https://github.com/triSYCL/triSYCL/blob/master/include/CL/sycl/buffer/detail/buffer.hpp#L361
where we put the SFINAE in a spare argument.
But it also makes the function globally more complex by moving apart the SFINAE mechanics....
So I am fine with your solution too.

const size_t sizeInBytes, const property_list &propList,
AllocatorT allocator = AllocatorT())
: SizeInBytes(sizeInBytes), Props(propList), MAllocator(allocator) {
if (Props.has_property<property::buffer::use_host_ptr>()) {
// TODO next line looks unsafe
BufPtr = &*first;
} else {
BufData.resize(get_size());
BufPtr = reinterpret_cast<void *>(BufData.data());
// We need cast BufPtr to pointer to the iteration type to get correct
// offset in std::copy when it will increment destination pointer.
auto *Ptr = reinterpret_cast<
typename std::iterator_traits<InputIterator>::pointer>(BufPtr);
std::copy(first, last, Ptr);
}
BufData.resize(get_size());
BufPtr = reinterpret_cast<void *>(BufData.data());
// We need cast BufPtr to pointer to the iteration type to get correct
// offset in std::copy when it will increment destination pointer.
auto *Ptr =
reinterpret_cast<typename std::iterator_traits<InputIterator>::pointer>(
BufPtr);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am lost here. Why the intermediate reinterpret_cast<void *> ?
Why not just using BufData.data() instead of BufPtr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BufPtr is not only intermediate reinterpret_cast<void *>. It also buffer_impl's field and currently we use it instead of BufData. BufData is needed only for memory allocation...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, I missed that. Good point.

std::copy(first, last, Ptr);
// Data is written back if InputIterator is not a const iterator.
set_final_data(first);
}

template <class InputIterator>
buffer_impl(EnableIfConstIterator<InputIterator> first, InputIterator last,
const size_t sizeInBytes, const property_list &propList,
AllocatorT allocator = AllocatorT())
: SizeInBytes(sizeInBytes), Props(propList), MAllocator(allocator) {
BufData.resize(get_size());
BufPtr = reinterpret_cast<void *>(BufData.data());
// We need cast BufPtr to pointer to the iteration type to get correct
// offset in std::copy when it will increment destination pointer.
typedef typename std::iterator_traits<InputIterator>::value_type value;
auto *Ptr = reinterpret_cast<typename std::add_pointer<
typename std::remove_const<value>::type>::type>(BufPtr);
std::copy(first, last, Ptr);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All this seems code duplication at first sight.
It is not possible to have a single version of these 2 functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, It is not possible to have a single version without using of some C++17 if constexpr because I can't add line with "set_final_data(first);" to c'tor with const iterators - compiler fails in instantiation of std::copy inside the set_final_data. I'm not sure that we are ready to C++17 in our codebase so I'm using SFINAE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, unfortunately that makes sense.


buffer_impl(cl_mem MemObject, const context &SyclContext,
Expand Down
36 changes: 28 additions & 8 deletions sycl/test/basic_tests/buffer/buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,29 @@ int main() {
range<1>{3}, [=](id<1> index) { B[index] = 20; });
});
}
// Data is not copied back in the desctruction of the buffer created from
// pair of iterators
// Data is copied back in the desctruction of the buffer created from
// pair of non-const iterators
for (int i = 0; i < 2; i++)
assert(data1[i] == -1);
for (int i = 2; i < 5; i++)
assert(data1[i] == 20);
for (int i = 5; i < 10; i++)
assert(data1[i] == -1);
}

// Check that data is not copied back in the desctruction of the buffer
// created from pair of const iterators
{
std::vector<int> data1(10, -1);
{
buffer<int, 1> b(data1.cbegin() + 2, data1.cbegin() + 5);
Copy link
Contributor

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.

Hmm, It's really big test and in all buffer constructions () is used, I think this change also should be done in separate patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right. It is cosmetic change.

queue myQueue;
myQueue.submit([&](handler &cgh) {
auto B = b.get_access<access::mode::read_write>(cgh);
cgh.parallel_for<class const_iter_constuctor>(
range<1>{3}, [=](id<1> index) { B[index] = 20; });
});
}
for (int i = 0; i < 10; i++)
assert(data1[i] == -1);
}
Expand Down Expand Up @@ -435,24 +456,23 @@ int main() {
// created from pair of iterators
{
std::vector<int> data1(10, -1);
std::vector<int> data2(10, -1);
{
buffer<int, 1> b(data1.begin() + 2, data1.begin() + 5);
b.set_final_data(data1.begin() + 2);
b.set_final_data(data2.begin() + 2);
queue myQueue;
myQueue.submit([&](handler &cgh) {
auto B = b.get_access<access::mode::read_write>(cgh);
cgh.parallel_for<class iter_constuctor_set_final_data>(
range<1>{3}, [=](id<1> index) { B[index] = 20; });
});
}
// Data is not copied back in the desctruction of the buffer created from
// pair of iterators
for (int i = 0; i < 2; i++)
assert(data1[i] == -1);
assert(data2[i] == -1);
for (int i = 2; i < 5; i++)
assert(data1[i] == 20);
assert(data2[i] == 20);
for (int i = 5; i < 10; i++)
assert(data1[i] == -1);
assert(data2[i] == -1);
}

// Check that data is copied back after forcing write-back using
Expand Down