[SYCL] Pass buffer_location property to buffer #5604

maximdimakov · 2022-02-17T14:06:29Z

There is the accessor property "buffer_location" that allows to allocate buffer in definite location (spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_intel_buffer_location.asciidoc)
Current implementation doesn't implement allocating a buffer at the passed location and the buffer will be re-sided when a kernel is enqueued. It leads to problems of various kinds.
The proposed solution implies adding new buffer property in order to store it in a buffer and use it when the buffer is allocated.
If the property is not supported by device it will be ignored.
Signed-off-by: mdimakov [email protected]

This reverts commit 34a4689.

This reverts commit a68ee9cf2bd421fb67fdf6e6e9be0a18b0d43f6b.

s-kanaev · 2022-02-17T14:42:59Z

sycl/test/abi/vtable.cpp

+// CHECK-NEXT:  11 | void sycl::detail::SYCLMemObjI::addOrReplaceAccessorProperties(const sycl::property_list &) [pure]
+// CHECK-NEXT:  12 | void sycl::detail::SYCLMemObjI::deleteAccessorProperty(const sycl::detail::PropWithDataKind &) [pure]


Isn't this an ABI break?

Remove virtual functions

maximdimakov · 2022-02-18T16:18:34Z

/summary:run

maximdimakov · 2022-02-21T13:23:53Z

The test fpga_tests/buffer_location.cpp failed because the device the test ran on has no support for the buffer_location feature. There are 2 ways to resolve this problem:

Disable this test until CI devices get support for the buffer_location feature
Add sycl::aspect for this feature and check it. The difficulty is that spec has no mention for such aspect. It can be added there, but this action may require indefinite amount of time

bader · 2022-02-21T13:34:55Z

Add sycl::aspect for this feature and check it. The difficulty is that spec has no mention for such aspect. It can be added there, but this action may require indefinite amount of time

I suggest filing a bug report on the spec and ask for fix ETA.

Please, update the link to the spec and fix punctuation in the description.

bader · 2022-02-21T13:36:25Z

Add sycl::aspect for this feature and check it. The difficulty is that spec has no mention for such aspect. It can be added there, but this action may require indefinite amount of time

I suggest filing a bug report on the spec and ask for fix ETA.

Please, update the link to the spec and fix punctuation in the description.

Just confirming that this the issue in the spec and it's going to be fixed by adding a new device aspect might be enough to implement a proper test fix.

s-kanaev · 2022-03-02T12:20:53Z

sycl/unittests/buffer/BufferLocation.cpp

+      .wait();
+  std::shared_ptr<sycl::detail::buffer_impl> BufImpl =
+      sycl::detail::getSyclObjImpl(Buf);
+  EXPECT_EQ(


Why buffer impl is used here instead of PassedLocation?
If the property shouldn't be there in call to piMemBufferCreate then it should be checked.

There is no kernel in the command group above, so piMemBufferCreate will not be called. This check verifies that new buffer_location is passed to the buffer properties. Check that this value is passed correctly is placed above
https://github.com/intel/llvm/pull/5604/files/4d548846221b21799da61a62f7755bc33b59f829#diff-8e8ed34037520ee5405a46f25d39b96ff066f684a71560371fd3258ac46be9f8R146

s-kanaev · 2022-03-02T12:21:38Z

sycl/unittests/buffer/BufferLocation.cpp

+      .wait();
+
+  EXPECT_EQ(
+      BufImpl->has_property<sycl::property::buffer::detail::buffer_location>(),


sycl/source/detail/context_impl.hpp

sycl/source/detail/context_impl.cpp

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

Co-authored-by: sergei <[email protected]>

maximdimakov · 2022-03-02T15:05:42Z

@bader I have opened PR with adding the aspect #5660. It was decided to ignore this property if it is not supported on device. So, fpga_tests/buffer_location.cpp works as before.

sherry-yuan · 2022-03-02T16:43:30Z

sycl/source/detail/context_impl.cpp

+  auto Plugin = getPlugin();
+  for (auto &Device : MDevices) {
+    const RT::PiDevice PiDevice = getSyclObjImpl(Device)->getHandleRef();
+    if (Plugin.call_nocheck<detail::PiApiKind::piDeviceGetInfo>(
+            PiDevice, (pi_device_info)PI_MEM_PROPERTIES_ALLOC_BUFFER_LOCATION,
+            sizeof(pi_device_info), &device_info, &return_size) != PI_SUCCESS) {
+      SupportBufferLocationByDevices = NotSupported;
+      break;
+    }
+  }


I think this specific way of calling get device info will always return false since the lower level runtime currently does not support passing PI_MEM_PROPERTIES_ALLOC_BUFFER_LOCATION into the get device info API. Is there no other way of determining whether the target is an FPGA that does not rely on lower level runtime?

If we do this, then the OpenCL Spec has to be changed.

P.S refer to the opencl spec for what can be done with the property
https://github.com/KhronosGroup/OpenCL-Docs/blob/main/extensions/cl_intel_mem_alloc_buffer_location.asciidoc

Although technically the opencl spec is written in a way that enforces all vendor's runtime to accept this property, so technically it should be fine to directly pass it into other runtime (although they may do nothing with the property).

Is there no other way of determining whether the target is an FPGA that does not rely on lower level runtime?
We can determine if the device is FPGA or not in runtime. I tried to do this in ccf73a6
Do you think it would be better to just check if a device is FPGA or not?

Yes I do think it is better to check if a device is FPGA directly, because even if it error out on FPGA device, it will prompt the other vendor to support passing in of buffer location feature in their runtime API. (Technically all vendor should support this property anyways)

------------------------------------------------------ Currently sycl runtime is trying to distinguish whether buffer location is supported on the target device using lower level runtime getDeviceInfo call to determine it. However current runtime get device info does not yet support accepting the buffer location property yet, this change addes it. The referenced sycl runtime PR: intel/llvm#5604

maximdimakov · 2022-03-02T19:03:13Z

/summary:run

sherry-yuan · 2022-03-02T20:29:23Z

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

-  PropWithDataKindSize = 5
+  AccPropBufferLocation = 5,
+  PropWithDataKindSize = 6,


Is there any reason why new enum is not added at the end?

Feel free to ignore, I found the reason: there is a comparison somewhere where all other enum of the same type cannot exceed this last enum.

llvm/sycl/include/CL/sycl/detail/property_list_base.hpp

Line 94 in 54ede40

if (PropKind >= PropWithDataKind::PropWithDataKindSize)

Is this an ABI breaking change given that PropWithDataKindSize changed from 5 -> 6? If there are existing object file that assumes PropWithDataKindSize is 5?

It is not an ABI breaking change, because it is not a property and it is used only in property_list_base.hpp

maximdimakov · 2022-03-05T08:50:55Z

@s-kanaev could you, please, review?

sherry-yuan

LGTM, The latest change passes internal test and has the expected behavior.
Wait for others' approval though.

Update spec accordingly to the changes from #5604. buffer_location property will be ignored if the device it passed to doesn't support it. Signed-off-by: mdimakov <[email protected]>

s-kanaev

LGTM overall

s-kanaev · 2022-03-11T15:08:20Z

sycl/source/detail/context_impl.hpp

@@ -177,6 +182,7 @@ class context_impl {
  std::map<std::pair<DeviceLibExt, RT::PiDevice>, RT::PiProgram>
      MCachedLibPrograms;
  mutable KernelProgramCache MKernelProgramCache;
+  mutable PropertySupport SupportBufferLocationByDevices;


Suggested change

mutable PropertySupport SupportBufferLocationByDevices;

mutable PropertySupport MSupportBufferLocationByDevices;

s-kanaev · 2022-03-15T16:24:38Z

sycl/unittests/CMakeLists.txt

@@ -33,4 +33,5 @@ add_subdirectory(program_manager)
 add_subdirectory(assert)
 add_subdirectory(Extensions)
 add_subdirectory(windows)
+add_subdirectory(event)


That's an interesting change in this late commit...

I tried to resolve merge conflict. This line has already added to CMakeLists.txt

s-kanaev

LGTM

bader · 2022-03-16T16:54:18Z

/summary:run

maximdimakov added 9 commits February 3, 2022 14:31

Return cleanupdep

9d6a1bf

This reverts commit 34a4689.

Return old option

a3c2249

Merge remote-tracking branch 'origin/sycl' into sycl

872e72d

Merge branch 'sycl' of https://github.com/intel/llvm into sycl

73268ab

Revert "Revert "working draft""

879aefe

This reverts commit a68ee9cf2bd421fb67fdf6e6e9be0a18b0d43f6b.

Delete debugging cout

7396044

Working draft

7b414f9

Clang-format fix

e340604

Update files

7e1922e

maximdimakov requested a review from s-kanaev February 17, 2022 14:07

s-kanaev reviewed Feb 17, 2022

View reviewed changes

maximdimakov added 6 commits February 18, 2022 10:34

No longer use of sycl_mem_obj_i

531d38e

Add test for buffer_location

30b9d01

Add check for opencl backend

9f84f3b

Fix unittest problems for windows

9f5be75

Add windows symbols

5914d97

fix types

cbcc0d3

maximdimakov added 2 commits February 21, 2022 10:47

Remove extra parameter from adjustAccPropsInBu

6f560f6

Remove comment line

c1fd1ea

maximdimakov marked this pull request as ready for review February 21, 2022 07:54

maximdimakov requested a review from a team as a code owner February 21, 2022 07:54

maximdimakov requested a review from s-kanaev February 21, 2022 09:32

sherry-yuan mentioned this pull request Feb 24, 2022

[SYCL] USM Buffer Location Properties #5634

Merged

maximdimakov added 3 commits February 24, 2022 21:07

Move pi property

794953d

Adjust test for accelerator

841d59f

Check for support buffer location

7af9aec

s-kanaev suggested changes Mar 2, 2022

View reviewed changes

maximdimakov and others added 2 commits March 2, 2022 17:19

Address to review comments

6de4bc7

Update sycl/source/detail/context_impl.cpp

82394cb

Co-authored-by: sergei <[email protected]>

maximdimakov mentioned this pull request Mar 2, 2022

[SYCL] Add aspect for buffer_location property #5660

Closed

maximdimakov added 2 commits March 2, 2022 17:32

Address to review comments

e5e4fb3

Clang-format fix

ddefba5

sherry-yuan reviewed Mar 2, 2022

View reviewed changes

sherry-yuan mentioned this pull request Mar 2, 2022

Check if device support heterogeneous memory intel/fpga-runtime-for-opencl#88

Merged

sherry-yuan reviewed Mar 2, 2022

View reviewed changes

maximdimakov added 2 commits March 3, 2022 16:11

Add check for extension

d2701b6

Clang-format fix

3db4cfc

maximdimakov mentioned this pull request Mar 5, 2022

[SYCL][DOC] Update buffer_location property spec #5739

Merged

sherry-yuan previously approved these changes Mar 7, 2022

View reviewed changes

Merge branch 'sycl' into buffer_location_prop

7b65c22

maximdimakov requested a review from s-kanaev March 11, 2022 09:01

s-kanaev suggested changes Mar 11, 2022

View reviewed changes

Address review comment

13e14d4

maximdimakov dismissed sherry-yuan’s stale review via 13e14d4 March 15, 2022 16:01

Merge branch 'sycl' into buffer_location_prop

bc62ba7

maximdimakov requested a review from s-kanaev March 15, 2022 16:03

s-kanaev reviewed Mar 15, 2022

View reviewed changes

s-kanaev approved these changes Mar 15, 2022

View reviewed changes

bader merged commit 9808525 into intel:sycl Mar 17, 2022

		// CHECK-NEXT: 11 \| void sycl::detail::SYCLMemObjI::addOrReplaceAccessorProperties(const sycl::property_list &) [pure]
		// CHECK-NEXT: 12 \| void sycl::detail::SYCLMemObjI::deleteAccessorProperty(const sycl::detail::PropWithDataKind &) [pure]

	mutable PropertySupport SupportBufferLocationByDevices;
	mutable PropertySupport MSupportBufferLocationByDevices;

[SYCL] Pass buffer_location property to buffer #5604

[SYCL] Pass buffer_location property to buffer #5604

Uh oh!

Conversation

maximdimakov commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximdimakov commented Feb 18, 2022

Uh oh!

maximdimakov commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Feb 21, 2022

Uh oh!

bader commented Feb 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maximdimakov commented Mar 2, 2022

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximdimakov commented Mar 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximdimakov commented Mar 5, 2022

Uh oh!

sherry-yuan left a comment

Choose a reason for hiding this comment

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Mar 16, 2022

Uh oh!

Uh oh!

maximdimakov commented Feb 17, 2022 •

edited

Loading

maximdimakov commented Feb 21, 2022 •

edited

Loading

sherry-yuan Mar 2, 2022 •

edited

Loading

sherry-yuan Mar 2, 2022 •

edited

Loading

sherry-yuan Mar 2, 2022 •

edited

Loading