Skip to content

[SYCL][Doc] Define buffer_location property for USM allocations #5665

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
Merged
merged 9 commits into from
Mar 3, 2022
Merged

[SYCL][Doc] Define buffer_location property for USM allocations #5665

merged 9 commits into from
Mar 3, 2022

Conversation

sherry-yuan
Copy link
Contributor

This serves as a temporary solution to supporting usm buffer location, and it is the easiest to implement.
Align with changes in #5634

A more complex workaround is to define a new malloc API that accept sycl::ext::oneapi::experimental::properties as its property argument of malloc.

The full solution is to define a malloc api that takes sycl::ext::oneapi::experimental::properties as property argument and returns annotated_ptr

@sherry-yuan
Define new buffer_location runtime property to be passed into malloc_…
f2e8240 
…device

-----------------------------------------------------------------------------------

This serves as a temporary solution to supporting usm buffer location, and it is the easiest to implement.
Align with changes in intel#5634

A more complex workaround is to define a new malloc API that accept sycl::ext::oneapi::experimental::properties as its property argument of malloc.

The full solution is to define a malloc api that takes sycl::ext::oneapi::experimental::properties as property argument and returns annotated_ptr
@sherry-yuan
rename extension to runtime_buffer_location
f2eab12
@sherry-yuan
fixup
9737bb4
gmlueck
gmlueck reviewed Feb 28, 2022
View reviewed changes
@sherry-yuan
Follow new spec template
bc2bbfa 
---------------------------------

1. remove ipmlemetation details
2. rename extension
3. fix feature test macro to have same name
4. fix name space to be in sycl::ext::intel::experimental::property::usm
@sherry-yuan sherry-yuan requested a review from gmlueck March 1, 2022 15:01
gmlueck
gmlueck reviewed Mar 1, 2022
View reviewed changes
@sherry-yuan
Add more details on motivation of the extension
8335ff5 
--------------------------------------------------------------

1. Remove notes
2. Specify which malloc api accept the property
3. Add more detaile ddescription, and expected behavior when used on non fpga device
@sherry-yuan
fix_typos
63ea912
@sherry-yuan sherry-yuan requested a review from gmlueck March 1, 2022 17:30
gmlueck
gmlueck reviewed Mar 1, 2022
View reviewed changes
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

This is looking a lot better. Thanks for all the changes. I just have a couple other comments.

@sherry-yuan sherry-yuan marked this pull request as ready for review March 1, 2022 20:00
@sherry-yuan sherry-yuan requested a review from a team as a code owner March 1, 2022 20:00
@sherry-yuan sherry-yuan requested a review from gmlueck March 1, 2022 20:00
gmlueck
gmlueck previously approved these changes Mar 1, 2022
View reviewed changes
@gmlueck
Copy link
Contributor

gmlueck commented Mar 1, 2022

Oh, I just noticed that this file is added to the "experimental" directory. Is this implemented yet? I think it is not?

If it is not implemented, the file should be added to "proposed". It can then be moved to "experimental" in the PR that adds the implementation. Or, if the implementation is simple, you can add the implementation to this PR and use this PR for both the specification and the implementation.

@sherry-yuan
Copy link
Contributor Author

sherry-yuan commented Mar 1, 2022 •
edited
Loading

Oh, I just noticed that this file is added to the "experimental" directory. Is this implemented yet? I think it is not?

Thanks, I have moved the spec to proposed folder. The implementation is already in another PR, I will keep it there for now.

#5634

@sherry-yuan
Copy link
Contributor Author

sherry-yuan commented Mar 2, 2022 •
edited
Loading

I currently do not have write access to the repo, it will be nice if someone could merge it for me once all the blocking steps are done (if any).

@gmlueck
Copy link
Contributor

gmlueck commented Mar 2, 2022

I currently do not have write access to the repo, it will be nice if someone could merge it for me once all the blocking steps are done (if any).

Most people do not have permission to merge PRs. @bader usually merges them after there is an approval and after testing completes.

@bader
Copy link
Contributor

bader commented Mar 2, 2022

I currently do not have write access to the repo, it will be nice if someone could merge it for me once all the blocking steps are done (if any).

Most people do not have permission to merge PRs. @bader usually merges them after there is an approval and after testing completes.

Please, notify @intel/llvm-gatekeepers team when PR is ready for merge.

@gmlueck gmlueck mentioned this pull request Mar 2, 2022
Closed
@sherry-yuan
Copy link
Contributor Author

@intel/llvm-gatekeepers The PR is ready to merge.

@sherry-yuan sherry-yuan changed the title Define new buffer_location runtime property to be passed into malloc_device [SYCL] Define new buffer_location runtime property to be passed into malloc_device Mar 2, 2022
@sherry-yuan sherry-yuan mentioned this pull request Mar 2, 2022
Merged
@bader bader changed the title [SYCL] Define new buffer_location runtime property to be passed into malloc_device [SYCL][Doc] Define buffer_location property for USM allocations Mar 3, 2022
@bader bader merged commit 962417d into intel:sycl Mar 3, 2022
bader pushed a commit that referenced this pull request Mar 9, 2022
@sherry-yuan
[SYCL] USM Buffer Location Properties (#5634)
12c988a 
Lower level runtime's usm allocation API now supports pass in of buffer location property [1] defined in OpenCL spec [2]

For this feature to be accessible to users, sycl runtime now passes in usm::buffer_location property defined in sycl spec [3] into the opencl runtime calls, the property is only passed if the extension name occurs in the platform extension string. If the lower level runtime does not support such property, then the property will yield no effect.

This is a temporary solution to allow user to specify which memory location the device usm allocation should be in. The full solution will require retuning an annotated_ptr that carries compile time properties for further optimization. The full solution spec is in [4]

[1] https://github.com/intel/fpga-runtime-for-opencl/pull/46/files
[2] https://github.com/KhronosGroup/OpenCL-Docs/blob/master/extensions/cl_intel_mem_alloc_buffer_location.asciidoc
[3] #5665
[4] #5656
This was referenced May 31, 2022
Merged
Merged
pvchupin pushed a commit that referenced this pull request Jun 7, 2022
@pcolberg
[SYCL] Support USM buffer location property in malloc_shared (#6218)
6e89821 
This ports commit 12c988a from
malloc_device to malloc_shared for use with the FPGA Runtime for OpenCL.

See malloc_device implementation in #5634

See extension specification in #5665
steffenlarsen pushed a commit that referenced this pull request Jun 8, 2022
@pcolberg
[SYCL] Support USM buffer location property in malloc_host (#6220)
2c7caab 
See malloc_shared implementation in #6218

See extension specification in #5665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlueck gmlueck gmlueck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sherry-yuan @gmlueck @bader