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

Remove deprecated target::host_buffer #835

Closed

Conversation

BenBrock
Copy link
Contributor

@BenBrock BenBrock commented Mar 8, 2023

Replacing a use of deprecated target::host_buffer with target::host_task. There is one other use of host_buffer here, but it is using the internal namespace, so not sure if it should be replaced as well.

@@ -45,7 +45,7 @@ struct extract_accessor<oneapi::dpl::__internal::sycl_iterator<Mode, T, Allocato
static constexpr sycl::access::mode mode = Mode;
static constexpr int dim = 1;
using buffer_type = sycl::buffer<T, dim, Allocator>;
using accessor_type = sycl::accessor<T, dim, mode, sycl::access::target::host_buffer>;
using accessor_type = sycl::accessor<T, dim, mode, sycl::access::target::host_task>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should instead use host_accessor here. @MikeDvorskiy please take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that sycl::host_accessor should be used as the return type.

Also, the sycl buffer member function used to get the return value, get_access<mode>(), is also deprecated in SYCL 2020 and get_host_access() should be used instead. These should change together.

@akukanov
Copy link
Contributor

akukanov commented Mar 16, 2023

Requested a few more developers to look at this.

@BenBrock BenBrock changed the title Replace deprecated target::host_buffer with target::host_task Remove deprecated target::host_buffer Mar 20, 2023
@BenBrock
Copy link
Contributor Author

I've updated the PR to use sycl::host_accessor instead. I quickly searched through the codebase for any usage of get_mode<..., target...>, but didn't find any.

@danhoeflinger
Copy link
Contributor

I've updated the PR to use sycl::host_accessor instead. I quickly searched through the codebase for any usage of get_mode<..., target...>, but didn't find any.

To be more specific because there are many get_access overloads...
From the SYCL spec for the buffer class:

template <access_mode Mode>
accessor<T, Dimensions, Mode, target::host_buffer> get_access()

"Deprecated in SYCL 2020. Use get_host_access() instead.
Returns a valid host accessor to the buffer with the specified access mode and target."


The instance I was referring to is right after your change, include/oneapi/dpl/internal/iterator_impl.h: line 53.

The function which makes use of this accessor_type should be changed.

    static accessor_type
    get(oneapi::dpl::__internal::sycl_iterator<Mode, T, Allocator>& iter)
    {
        return iter.get_buffer().template get_access<mode>();  // <-- here
    }

I think there are some instances elsewhere as well in include/oneapi/dpl/internal/function.h (a helper function which may not be used anymore) and in a test test/xpu_api/functional/test_functional.cpp.

@akukanov
Copy link
Contributor

The possible problem with trying to change get_access to get_host_access now is that the latter cannot be parameterized by the mode, instead it uses deduction tags as function arguments. So more changes will likely be needed in the class, and etc. And I am not even sure that this class is needed in the long term, as we now re-think the approach for buffer access modes in our pseudo-iterator class.

So I suggest to either accept the current one-line change in this PR, or postpone it completely until we figure out how we deal with access modes in general.

@danhoeflinger
Copy link
Contributor

So I suggest to either accept the current one-line change in this PR, or postpone it completely until we figure out how we deal with access modes in general.

Its a fair point, I hadn't fully considered the implications of how this will work in context. However, I believe these changes must be done together. To return a sycl::host_accessor<T,dim,mode> from this function, we need to use get_host_access(), as there is no conversion from the result of get_access<mode>() (the old return type) to a sycl::host_accessor<T,dim,mode>. Testing this briefly as a one-line change results in an error.

Therefore, I think its best to postpone this. As you mention, this struct may not be needed in general. As far as I can find, I don't believe it is used anywhere in the repository (and hasn't been in some time).

@BenBrock
Copy link
Contributor Author

Closing this PR.

@BenBrock BenBrock closed this Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants