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

[SYCLomatic] Removing data() from USM_NONE dpct::device_vector #667

Conversation

danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Feb 24, 2023

Removing the public member function data() of dpct::device_vector when DPCT_USM_LEVEL_NONE is defined.

dpct::device_vector has two different implementations, one where USM is available and another when USM is not available which that uses sycl::buffer internally for its device memory. This change only effects the implementation when USM is not available and sycl::buffer is used for the memory allocation.

This data() member function does not make a lot of sense to provide when we are using sycl::buffer as the memory backing for dpct::device_vector. Prior to this PR, it provided a pointer into the virtual memory space used to track and look up buffer allocations. Using this returned virtual pointer (when DPCT_USM_LEVEL_NONE is defined) to do things like memcpy() or as input to an algorithm would result in a segfault. It's availability in the API only encourages incorrect usage.

Getting a pointer to the actual device memory is not supported in any viable way by the SYCL specification when a sycl::buffer is the backing memory for a dpct::device_vector. The public member function get_buffer() is provided in this case, and can be used for a similar purpose, but without encouraging incorrect usage.

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
@danhoeflinger
Copy link
Contributor Author

For an example of the usage which can be problematic when DPCT_USM_LEVEL_NONE is defined, see the changes of onedpl_test_copy_if in oneapi-src/SYCLomatic-test#243.

@danhoeflinger
Copy link
Contributor Author

It looks like there is some work to be done in the migration tool here which will need to be done to handle .data() calls specifically for usmnone. One of the tests includes such a call, which used to compile and now does not for usmnone.
For usmnone, I expect it will be challenging to replace .data() calls automatically with something which can be used interchangeably. However, it may be best to point them to manual conversion with .get_buffer() as this will result in valid code.

@danhoeflinger danhoeflinger marked this pull request as draft May 22, 2023 19:58
ShengchenJ pushed a commit to ShengchenJ/SYCLomatic that referenced this pull request Sep 27, 2024
…to test more cases (oneapi-src#667)

Signed-off-by: Jiang, Zhiwei <zhiwei.jiang@intel.com>
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.

1 participant