Adding public utility for users to check if usm_allocators are supported #1476

danhoeflinger · 2024-04-04T12:59:28Z

Support for USM allocators depends on the implementation details of the C++ standard library used after #1438, and after doc changes of #1477.

This PR adds a public utility (usm_allocated_vector_iterators_supported_v<ValueType>) which users can use at compile time to check if usm_allocators are available to them as efficient input to oneDPL algorithms.

If usm_allocated_vector_iterators_supported_v<ValueType> evaluates to true, std::vector<ValueType, usm_allocator>::iterator input types are passed directly to sycl kernels, and are an efficient way of using oneDPL.
If usm_allocated_vector_iterators_supported_v<ValueType> evaluates to false, std::vector<ValueType, usm_allocator>::iterator input types are treated as if they were host-side iterators, and wrapped in sycl::buffer before use in sycl kernels. This includes all that comes with that, an extra host-side copy in and out. Also these cannot then be used as the source iterator for permutation_iterator.

MikeDvorskiy · 2024-04-05T16:05:16Z

include/oneapi/dpl/pstl/hetero/dpcpp/sycl_iterator.h

@@ -233,6 +233,11 @@ __internal::sycl_iterator<access_mode::discard_read_write, T, Allocator> end(syc
    return __internal::sycl_iterator<access_mode::discard_read_write, T, Allocator>{buf,
                                                                                    __dpl_sycl::__get_buffer_size(buf)};
 }
+
+template <typename ValueType>
+constexpr static bool usm_allocated_vector_iterators_supported_v =


I guess the traits doesn't depend on ValueType.

As an idea - to make such public trait more general, for example,

template <typename Container> constexpr static bool is_usm_container_supported = ...

Sadly, I believe it can depend on the ValueType, for example std::vector<bool>. GNU c++ standard library implementation normally includes allocator type in the iterator, but not for std::vector<bool> with the bitfield "optimization".

(This is the only instance I know of, but it shows that we cant guarantee that the implementation details are consistent across value type)

MikeDvorskiy · 2024-04-05T16:09:44Z

include/oneapi/dpl/pstl/hetero/dpcpp/sycl_iterator.h

+
+template <typename ValueType>
+constexpr static bool usm_allocated_vector_iterators_supported_v =
+    oneapi::dpl::__internal::__vector_iter_distinguishes_by_allocator<ValueType*>::value;


BTW, what if ValueType is a pointer to the host data?
It is rather a kind of general question connected to "Pass Data to algorithms" topic.

If I understand correctly, you suggest the following type:

std::vector<int*, usm_allocator<int*, ...>>

In theory, a vector of host pointers will be very similar to a vector of integers I think.

In principle, as long as they are not dereferencing the pointers on the device, there is nothing wrong with this.

SergeyKopienko · 2024-04-05T16:14:27Z

include/oneapi/dpl/pstl/hetero/dpcpp/sycl_iterator.h

+};
+template <typename Iter>
+struct __vector_iter_distinguishes_by_allocator<
+    Iter, std::enable_if_t<!std::is_same_v<__default_alloc_vec_iter<Iter>, __usm_shared_alloc_vec_iter<Iter>> &&


Should we apply here std::decay_t for Iter template param?

Actually, this is already merged code from #1438 . Working to rebase, to make this focus on just what is added by this PR.
If we need to go back and fix something here, it will needs its own PR I think.

SergeyKopienko · 2024-04-05T16:16:09Z

include/oneapi/dpl/pstl/hetero/dpcpp/utils_ranges_sycl.h

+//support std::vector::iterator with usm host / shared allocator as passed directly
+template <typename Iter>
+struct is_passed_directly<
+    Iter, std::enable_if_t<(std::is_same_v<Iter, oneapi::dpl::__internal::__usm_shared_alloc_vec_iter<Iter>> ||


Should we apply here std::decay_t for Iter template param?

Same as above, this is merged code from #1438. The duplicated conflict code has been fixed via rebase. Any changes here will need a new PR.

documentation/library_guide/parallel_api/pass_data_algorithms.rst

dcbenito · 2024-04-05T18:27:22Z

documentation/library_guide/parallel_api/pass_data_algorithms.rst

+allocator is included in the ``std::vector::iterator`` type definition. You can check
+if your C++ standard library implementation provides enough information by checking the
+value of ``oneapi::dpl::usm_allocated_vector_iterators_supported_v<ValueType>``. This will
+evaluate to ``true`` for C++ standard library implementations which allow oneDPL to support


Please add a comma between "implementations" and "which" and add an "s" to "allow". The final sentence should read:

This will evaluate to true for C++ standard library implementations, which allows oneDPL to support USM allocators with this ValueType and false otherwise.

I think there was some confusion by my wording. I've expanded this some to hopefully make things more clear.

Thanks for the update. My suggestions on the new content (ignore spacing; I broke things up for this review).

This evaluates to true when using a C++ standard library implementation, which allows oneDPL to detect and
support USM allocators for ValueType and false.

When oneapi::dpl::usm_allocated_vector_iterators_supported_v<ValueType> evaluates to false,
you should rely on USM pointers directly instead of using iterators to vectors with USM allocators.

documentation/library_guide/parallel_api/pass_data_algorithms.rst

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger · 2024-04-26T21:27:10Z

I think that with the direction we are going with #1477, this does not make sense to continue with such a public API.
I prefer our decision to recommend against using USM allocated vector iterators in favor of USM pointers to data within them.
Exposing a public API like this is probably too "in the weeds" for the user about the details of the C++ standard library implementations.
Therefore, I'm closing this PR.

danhoeflinger changed the base branch from main to dev/dhoeflin/improve_usm_shared_alloc_vectors April 4, 2024 12:59

danhoeflinger requested review from akukanov, MikeDvorskiy, dcbenito and SergeyKopienko April 4, 2024 13:00

danhoeflinger mentioned this pull request Apr 4, 2024

Adding std::vector<T, usm_allocator>::iterator to is_passed_directly types #1438

Merged

danhoeflinger force-pushed the dev/dhoeflin/add_public_util_usm_alloc branch from d994552 to 2e10abf Compare April 4, 2024 14:33

danhoeflinger changed the base branch from dev/dhoeflin/improve_usm_shared_alloc_vectors to dev/dhoeflin/usm_allocator_doc_changes April 4, 2024 14:34

MikeDvorskiy reviewed Apr 5, 2024

View reviewed changes

danhoeflinger force-pushed the dev/dhoeflin/usm_allocator_doc_changes branch from b4022c0 to 07dffec Compare April 5, 2024 16:11

SergeyKopienko reviewed Apr 5, 2024

View reviewed changes

danhoeflinger force-pushed the dev/dhoeflin/add_public_util_usm_alloc branch from 2e10abf to 411ad2f Compare April 5, 2024 16:21

dcbenito requested changes Apr 5, 2024

View reviewed changes

danhoeflinger force-pushed the dev/dhoeflin/add_public_util_usm_alloc branch from 47b9858 to 8a67a8f Compare April 5, 2024 19:18

danhoeflinger force-pushed the dev/dhoeflin/usm_allocator_doc_changes branch from c748730 to 0d69123 Compare April 15, 2024 18:08

danhoeflinger added 6 commits April 15, 2024 14:13

[Doc] Additional details for USM allocators with std::vector

4a44b47

Signed-off-by: Dan Hoeflinger <[email protected]>

address reviewer comments

1e98dee

Signed-off-by: Dan Hoeflinger <[email protected]>

more explicit language

604cf9f

Signed-off-by: Dan Hoeflinger <[email protected]>

Adding public utility for users to check if usm_allocators are supported

2e64284

Signed-off-by: Dan Hoeflinger <[email protected]>

restoring removed space

6d53ee5

Signed-off-by: Dan Hoeflinger <[email protected]>

Added language for clarity

45ebb8d

Signed-off-by: Dan Hoeflinger <[email protected]>

danhoeflinger force-pushed the dev/dhoeflin/add_public_util_usm_alloc branch from 8a67a8f to 45ebb8d Compare April 15, 2024 18:13

danhoeflinger closed this Apr 26, 2024

danhoeflinger deleted the dev/dhoeflin/add_public_util_usm_alloc branch May 6, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding public utility for users to check if usm_allocators are supported #1476

Adding public utility for users to check if usm_allocators are supported #1476

danhoeflinger commented Apr 4, 2024 •

edited

Loading

MikeDvorskiy Apr 5, 2024

danhoeflinger Apr 5, 2024 •

edited

Loading

MikeDvorskiy Apr 5, 2024

danhoeflinger Apr 5, 2024 •

edited

Loading

SergeyKopienko Apr 5, 2024

danhoeflinger Apr 5, 2024

SergeyKopienko Apr 5, 2024

danhoeflinger Apr 5, 2024

dcbenito Apr 5, 2024

danhoeflinger Apr 5, 2024

dcbenito Apr 5, 2024

danhoeflinger commented Apr 26, 2024

Adding public utility for users to check if usm_allocators are supported #1476

Adding public utility for users to check if usm_allocators are supported #1476

Conversation

danhoeflinger commented Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

danhoeflinger Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhoeflinger Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhoeflinger commented Apr 26, 2024

danhoeflinger commented Apr 4, 2024 •

edited

Loading

danhoeflinger Apr 5, 2024 •

edited

Loading

danhoeflinger Apr 5, 2024 •

edited

Loading