[SYCL] USM Buffer Location Properties #5634

sherry-yuan · 2022-02-22T20:46:05Z

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

sherry-yuan · 2022-02-24T17:07:59Z

SYCL / Linux / HIP AMDGPU LLVM Test Suite fails not due to this change, because it falled for a no-functional change in https://github.com/intel/llvm/pull/5656/files# as well

steffenlarsen · 2022-02-24T18:47:42Z

Note that this does not directly adhere to the cl_intel_mem_alloc_buffer_location proposal as this implementation uses SYCL 2020 properties instead of the properties introduced in the sycl_ext_oneapi_properties proposal (in development).

@gmlueck - Should we add a variant of this for SYCL 2020 property lists to prevent it from being blocked by the development of the compile-time property lists?

sherry-yuan · 2022-02-24T19:02:46Z

Note that this does not directly adhere to the cl_intel_mem_alloc_buffer_location proposal as this implementation uses SYCL 2020 properties instead of the properties introduced in the sycl_ext_oneapi_properties proposal (in development).

@steffenlarsen Yes this is more of a stopgap solution for passing properties into the runtime. so that memories can be allocated in the right place. Spec for full solution that uses sycl_ext_oneapi_properties is here; #5656

Once the new property is ready, this change will introduce an override malloc function that uses the new property.

…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

sycl/test/extensions/usm/usm_alloc_utility.cpp

sycl/include/CL/sycl/detail/property_helper.hpp

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

gmlueck

LGTM, but someone else should review.

cperkinsintel · 2022-03-03T21:05:18Z

Once the new property is ready, this change will introduce an override malloc function that uses the new property.

The new properties were merged at the beginning of this week. For both spec conformance and to reduce churn, should we not use them instead?
@steffenlarsen

sherry-yuan · 2022-03-04T01:20:13Z

The new properties were merged at the beginning of this week. For both spec conformance and to reduce churn, should we not use them instead?

@cperkinsintel Having the new property list alone is not sufficient, we will also need annotated pointer to be ready. See the full solution spec for details: #5656

I don't think the full solution will be ready by Mar 27 given the bandwidth, so this is a workaround that uses existing API and only introduce a simple property in this spec: #5665

We went with this solution because it falls more closely with the expected timeline, bandwidth.

sherry-yuan · 2022-03-04T16:33:09Z

There was no error that was caused by this change, it seems like some test need to be updated because the name space of property was being confused with the new property list.

steffenlarsen · 2022-03-07T10:07:32Z

As @sherry-yuan pointed out there is another extension for using the new properties for doing this exactly, but it requires a new return type. We could make this an experimental feature and change the return type, which would mean we would break experimental ABI when the new return type is introduced. By using sycl::property_list instead we can deprecate this temporary API and allow users some time to transition to the "final" API.

steffenlarsen

Some minor comments, otherwise LGTM. Failures do not seem related.

sycl/include/sycl/ext/intel/experimental/usm_properties.hpp

sycl/source/detail/usm/usm_impl.cpp

…tement check performance and cosmetic changes

cperkinsintel

LGTM

steffenlarsen

LGTM! Is it possible to write a test for this?

sherry-yuan · 2022-03-07T17:13:40Z

LGTM! Is it possible to write a test for this?

Thanks! I am not completely sure, it will have to be an e2e test and make sure that the icd loaded runtime has the correct version that contains accept buffer location property. Currently, an e2e test is developed internally and passes (and afaik it seems like intel ocl runtime is the only one that support this property for now). I will look more into how e2e test can be developed in sycl runtime and put that up in a separate PR.

I created an issue to track it here: #5750

sherry-yuan · 2022-03-07T17:26:20Z

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

The desired commit message body is the same as PR description & title.

Thanks,
Sherry

sherry-yuan · 2022-03-08T19:08:15Z

@intel/llvm-gatekeepers Note on precommit test failures: some of the tests (4) does not use explicit namespace reference which caused some confusion with the new property, the test are fixed in this PR: intel/llvm-test-suite#904

sherry-yuan · 2022-03-08T22:03:57Z

@intel/llvm-gatekeepers the tests are fixed, but I currently don't have permissions to re-run the workflows. The tests should pass.

sherry-yuan · 2022-03-09T14:47:49Z

Thank you all for helping to get this in 😊!

Resolves intel/llvm#5750 The test intended to test changes in PR: intel/llvm#5634

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

…h malloc_shared (#6269) Use the same call to USMDeviceAlloc with an empty property list in all cases, to allow for straight-forward extension with future properties. Query buffer location extension only if buffer location property is passed. This amends #5634 See also #6220

…uite#955) Resolves intel#5750 The test intended to test changes in PR: intel#5634

sherry-yuan force-pushed the usm_buffer_loc branch 2 times, most recently from d674afb to 991fdd6 Compare February 24, 2022 15:31

sherry-yuan marked this pull request as ready for review February 24, 2022 18:29

sherry-yuan requested a review from a team as a code owner February 24, 2022 18:29

sherry-yuan requested a review from cperkinsintel February 24, 2022 18:29

sherry-yuan changed the title ~~[WIP] USM Buffer Location Properties~~ USM Buffer Location Properties Feb 24, 2022

sherry-yuan mentioned this pull request Feb 24, 2022

Adding compile time and runtime properties for usm allocations #5656

Closed

sherry-yuan mentioned this pull request Feb 24, 2022

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

Merged

gmlueck mentioned this pull request Feb 24, 2022

Support buffer location for usm allocations #5661

Closed

sherry-yuan force-pushed the usm_buffer_loc branch from 991fdd6 to 396ad46 Compare March 2, 2022 18:16

This comment was marked as duplicate.

Sign in to view

sherry-yuan changed the title ~~USM Buffer Location Properties~~ [SYCL] USM Buffer Location Properties Mar 2, 2022

sherry-yuan force-pushed the usm_buffer_loc branch from f7bdf16 to ca8b419 Compare March 2, 2022 19:48

gmlueck reviewed Mar 2, 2022

View reviewed changes

sycl/test/extensions/usm/usm_alloc_utility.cpp Outdated Show resolved Hide resolved

gmlueck reviewed Mar 2, 2022

View reviewed changes

sycl/include/CL/sycl/detail/property_helper.hpp Show resolved Hide resolved

sherry-yuan force-pushed the usm_buffer_loc branch from 612faab to 35f7459 Compare March 2, 2022 20:31

sherry-yuan requested review from gmlueck and steffenlarsen March 3, 2022 18:46

gmlueck previously approved these changes Mar 3, 2022

View reviewed changes

steffenlarsen reviewed Mar 7, 2022

View reviewed changes

sycl/include/sycl/ext/intel/experimental/usm_properties.hpp Outdated Show resolved Hide resolved

sycl/source/detail/usm/usm_impl.cpp Outdated Show resolved Hide resolved

sycl/source/detail/usm/usm_impl.cpp Outdated Show resolved Hide resolved

Pass buffer location property into usm allocations

0eb2959

sherry-yuan added 5 commits March 7, 2022 11:03

initialize ID within device usm alloc

9108fbf

Avoid muliplying by sizeof type when using templated malloc

c60f7b6

Add feature test macro

c5d9ae1

Check for extension support rather than whether the device is an FPGA

7af3b4c

Check for direct extension support, remove redundancy, improve if sta…

2c78174

…tement check performance and cosmetic changes

sherry-yuan dismissed gmlueck’s stale review via 2c78174 March 7, 2022 16:03

sherry-yuan force-pushed the usm_buffer_loc branch from e9058ee to 2c78174 Compare March 7, 2022 16:03

cperkinsintel approved these changes Mar 7, 2022

View reviewed changes

steffenlarsen approved these changes Mar 7, 2022

View reviewed changes

sherry-yuan mentioned this pull request Mar 7, 2022

Develop e2e test suite for testing usm buffer location #5750

Closed

sherry-yuan mentioned this pull request Mar 8, 2022

Explicit namespace reference intel/llvm-test-suite#904

Merged

vladimirlaz approved these changes Mar 9, 2022

View reviewed changes

bader merged commit 12c988a into intel:sycl Mar 9, 2022

sherry-yuan mentioned this pull request Mar 11, 2022

Pass new property list into USM malloc API #5666

Closed

sherry-yuan mentioned this pull request Mar 30, 2022

Add test for usm buffer location properties intel/llvm-test-suite#955

Merged

sherry-yuan added a commit to sherry-yuan/llvm-test-suite that referenced this pull request Apr 1, 2022

[SYCL] Add test for usm buffer location properties

f0549c0

Resolves intel/llvm#5750 The test intended to test changes in PR: intel/llvm#5634

vladimirlaz pushed a commit to intel/llvm-test-suite that referenced this pull request Apr 3, 2022

[SYCL] Add test for usm buffer location properties (#955)

e184d60

Resolves intel/llvm#5750 The test intended to test changes in PR: intel/llvm#5634

pcolberg mentioned this pull request May 31, 2022

[SYCL] Support USM buffer location property in malloc_shared #6218

Merged

pcolberg mentioned this pull request Jun 7, 2022

[SYCL] Align USM buffer location implementation for malloc_device with malloc_shared #6269

Merged

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

[SYCL] Add test for usm buffer location properties (intel/llvm-test-s…

6ba3d9a

…uite#955) Resolves intel#5750 The test intended to test changes in PR: intel#5634

[SYCL] USM Buffer Location Properties #5634

[SYCL] USM Buffer Location Properties #5634

Uh oh!

Conversation

sherry-yuan commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sherry-yuan commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffenlarsen commented Feb 24, 2022

Uh oh!

sherry-yuan commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Mar 3, 2022

Uh oh!

sherry-yuan commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sherry-yuan commented Mar 4, 2022

Uh oh!

steffenlarsen commented Mar 7, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

sherry-yuan commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sherry-yuan commented Mar 7, 2022

Uh oh!

sherry-yuan commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sherry-yuan commented Mar 8, 2022

Uh oh!

sherry-yuan commented Mar 9, 2022

Uh oh!

Uh oh!

sherry-yuan commented Feb 22, 2022 •

edited

Loading

sherry-yuan commented Feb 24, 2022 •

edited

Loading

sherry-yuan commented Feb 24, 2022 •

edited

Loading

sherry-yuan commented Mar 4, 2022 •

edited

Loading

sherry-yuan commented Mar 7, 2022 •

edited

Loading

sherry-yuan commented Mar 8, 2022 •

edited

Loading