[SYCL] Add device_ptr and host_ptr #1864

MrSidims · 2020-06-10T12:58:21Z

Currently a device backend can't trace from where a pointer allocated by USM comes: it can be either allocated on host or on device (it's just a pointer in OpenCL global address space). On FPGAs at least we can generate more efficient hardware code if the user tells us where the pointer can point. With this change users can create multi_ptr with specialized address space global_host or global_device that will proved to the compiler additional information to process load-store optimizations. Accessor pointers shall be also moved to global_device address spaces - otherwise backend would assume, that a pointer in global address space can access both host and device memory.

Previously there were added global_device in global_host address spaces
for OpenCL/SYCL in clang. With this patch device_space and host_space
were added in the SYCL headers the are mapped into the new address spaces
and aliases to multi_ptr instantiated with the space: device_ptr and
host_ptr.

Signed-off-by: Dmitry Sidorov dmitry.sidorov@intel.com

sycl/test/multi_ptr/multi_ptr.cpp

MrSidims · 2020-06-17T13:09:21Z

Please review only clang and SYCL headers part. The translator's one was added only for testing (otherwise LITs are expected to fail).

romanovvlad · 2020-06-17T16:01:07Z

@MrSidims
Could you please share more information why we need a new address space? Some docs?

MrSidims · 2020-06-18T09:53:24Z

@MrSidims
Could you please share more information why we need a new address space? Some docs?

I updated the description for now. Also I was promised, that the specification will be published today.

sycl/include/CL/sycl/access/access.hpp

sycl/include/CL/sycl/handler.hpp

Fznamznon

This may slow down our first upstreaming milestone https://github.com/intel/llvm/milestone/1 since global_device and global_host address space attributes are not upstreamed.

clang/test/CodeGenSYCL/Inputs/sycl.hpp

MrSidims · 2020-06-19T10:46:54Z

This may slow down our first upstreaming milestone https://github.com/intel/llvm/milestone/1 since global_device and global_host address space attributes are not upstreamed.

I have just created a PR to llvm.org: https://reviews.llvm.org/D82174

sycl/include/CL/sycl/access/access.hpp

Naghasan · 2020-06-19T11:11:07Z

sycl/include/CL/sycl/access/access.hpp

@@ -104,6 +115,16 @@ struct PtrValueType<ElementType, access::address_space::global_space> {
  using type = __OPENCL_GLOBAL_AS__ ElementType;
 };

+template <typename ElementType>
+struct PtrValueType<ElementType, access::address_space::device_space> {
+  using type = __OPENCL_GLOBAL_DEVICE_AS__ ElementType;


This is likely to not be portable and cause ICE rather than a clear error. As this overlaps with global, the address space should fallback to __OPENCL_GLOBAL_AS__ if the backend does not handles this address space.

As DPCPP compiler generated SPIR-V code - this mechanism is currently moved to the SPIR-V translator (basically during reversed translation from SPIR-V to LLVM IR there is an option added - without this option passed, the translator will generate global address space instead of global_device / global_host address space. So if someone would like to support these address spaces in their backend - it's needed to add this option in the backend's driver.

As DPCPP compiler generated SPIR-V code

or PTX via the NVPTX backend without going through SPIR-V

Got it, thanks!

Actually, not, I don't really get it. What target is used for NVPTX? I mean, that in clang part of the feature we have added definitions for these new address spaces like this:
`--- a/clang/lib/Basic/Targets/NVPTX.h
+++ b/clang/lib/Basic/Targets/NVPTX.h
@@ -30,6 +30,8 @@ static const unsigned NVPTXAddrSpaceMap[] = {
0, // opencl_private
// FIXME: generic has to be added to the target
0, // opencl_generic

1, // opencl_global_device

1, // opencl_global_host
1, // cuda_device
`

If for NVPTX we compile with spir-unknown-unknown triple, than the code above is indeed a problem. But if not - I don't see any issues.

I don't really get it. What target is used for NVPTX?

nvptx64-nvidia-cuda-sycldevice

I don't see any issues

The issue is in the mangler, given the current definition of the address space mapping

void foo(global_ptr<int>::pointer_t p) { [...] } void foo(device_ptr<int>::pointer_t p) { [...] }

This will cause the compiler to mangle the 2 foo overloads in the same way.

There is 2 solutions to it:

Having a new mangling scheme, but I'm not sure how it should be done (@bader ping for this)

a SYCL solution: make the address space available if and only if the target actually supports it.

Note: this is kind of a corner case for now, I pointing this out so you are aware of it. I'm more concerned about the naming here.

Thank you very much for your feedback. I'll think about these options.

I would like to leave this corner case unresolved for now. One of the possible solutions is to expand authority of sycl_enable_usm_address_spaces option added in #1986 , but in this case this option (which originally was considered as a temporary solution) will stay in the compiler.

@MrSidims, could you open a GitHub issue to track/discuss solution this problem, please?

MrSidims · 2020-06-23T15:23:29Z

Sorry for force-pushing. I'm updating patches on which SYCL header part is based on (clang driver, the translator).

mlychkov

LGTM.
The only doubt is extra spaces around arrow operator in https://github.com/intel/llvm/pull/1864/files#diff-57bb9a63b756ac60f068b8632adc20e7R155. I think that a bug should be created for clang-format tool if it doesn't accept a variant without spaces.

s-kanaev · 2020-07-01T14:33:04Z

On FPGAs at least we can generate more efficient hardware if the user tells us where the pointer can point.

There's a type, isn't it? I believe it should be "hardware code" or something like this. Am I correct?

sycl/CMakeLists.txt

Otherwise it breaks atomics. Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

MrSidims · 2020-07-01T14:48:37Z

On FPGAs at least we can generate more efficient hardware if the user tells us where the pointer can point.

There's a type, isn't it? I believe it should be "hardware code" or something like this. Am I correct?

Fixed

sycl/include/CL/sycl/access/access.hpp

MrSidims · 2020-07-02T10:23:00Z

@mlychkov @bader @s-kanaev @sergey-semenov ping

MrSidims · 2020-07-02T15:47:53Z

@bader could you please advise me how to pull this PR from the purgatory?

s-kanaev

Looks good to me.
FPGA experts should approve, I reckon.

bader · 2020-07-02T19:50:26Z

@bader could you please advise me how to pull this PR from the purgatory?

Fix clang-format check and get approvals from code owners.

MrSidims · 2020-07-02T21:18:15Z

@bader clang-format is firing incorrectly. Regarding code owners.. Well, for FPGA features it's me and Mikhail, who wrote LGTM. For SYCL RT, Sergey is replacing Vlad, while he's on vacation (and he approved as well).

mlychkov

@bader Regarding clang-format error: it suggests to format the code line auto x4 = ptr_4->x; into auto x4 = ptr_4 -> x;, which is not actually used in our code style. So, I think that exception should be made for current review and it should be merged with failed clang-format test. Additionally, an issue should be raised for clang-format tool to fix such false negative case.

bader · 2020-07-03T13:21:17Z

@bader Regarding clang-format error: it suggests to format the code line auto x4 = ptr_4->x; into auto x4 = ptr_4 -> x;, which is not actually used in our code style. So, I think that exception should be made for current review and it should be merged with failed clang-format test.

Okay.

Additionally, an issue should be raised for clang-format tool to fix such false negative case.

+1

bader · 2020-07-03T17:01:22Z

sycl/test/check_device_code/usm_pointers.cpp

+// ret i8 addrspace(4)* %[[DEVCAST]]
+//
+// CHECK-LABEL: define {{.*}} spir_func i8 addrspace(4)* @{{.*}}multi_ptr{{.*}}
+// CHECK: %m_Pointer = getelementptr inbounds %[[HOSTPTR_T]]


This test fails on the builds with disabled assertions. I suppose we should not check variable names - those are stripped.
https://github.com/intel/llvm/runs/834501651
Please, fix ASAP.

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com> Original commit: KhronosGroup/SPIRV-LLVM-Translator@32721e8

MrSidims requested review from bader, romanovvlad, sergey-semenov and mlychkov June 10, 2020 12:58

MrSidims commented Jun 10, 2020

View reviewed changes

sycl/test/multi_ptr/multi_ptr.cpp Outdated Show resolved Hide resolved

MrSidims force-pushed the private/MrSidims/DeviceHost branch from 072c149 to 4b4428c Compare June 17, 2020 13:05

MrSidims marked this pull request as ready for review June 17, 2020 13:08

MrSidims requested review from AlexeySachkov, AlexeySotkin, elizabethandrews, Fznamznon, premanandrao and a team as code owners June 17, 2020 13:08

romanovvlad reviewed Jun 18, 2020

View reviewed changes

sycl/include/CL/sycl/access/access.hpp Outdated Show resolved Hide resolved

sycl/include/CL/sycl/handler.hpp Outdated Show resolved Hide resolved

MrSidims force-pushed the private/MrSidims/DeviceHost branch from 7f82a3b to a1de059 Compare June 18, 2020 21:55

MrSidims requested review from AGindinson and mdtoguchi as code owners June 18, 2020 21:55

Fznamznon reviewed Jun 19, 2020

View reviewed changes

clang/test/CodeGenSYCL/Inputs/sycl.hpp Outdated Show resolved Hide resolved

Naghasan requested changes Jun 19, 2020

View reviewed changes

MrSidims force-pushed the private/MrSidims/DeviceHost branch 3 times, most recently from e087594 to 0d7b5a3 Compare June 23, 2020 15:22

MrSidims requested review from romanovvlad and Fznamznon June 23, 2020 15:24

MrSidims force-pushed the private/MrSidims/DeviceHost branch from 0d7b5a3 to ded22f2 Compare June 24, 2020 20:45

MrSidims removed request for AlexeySotkin, Fznamznon, premanandrao, mdtoguchi and elizabethandrews June 29, 2020 13:10

mlychkov reviewed Jun 29, 2020

View reviewed changes

MrSidims requested review from kbobrovs and pvchupin as code owners July 1, 2020 13:49

MrSidims requested review from s-kanaev and mlychkov July 1, 2020 13:50

s-kanaev reviewed Jul 1, 2020

View reviewed changes

sycl/CMakeLists.txt Outdated Show resolved Hide resolved

MrSidims added 2 commits July 1, 2020 17:46

Move accessor pointers back to global space

fc5aa11

Otherwise it breaks atomics. Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

Ignore clang-format concern

1a1237c

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

MrSidims force-pushed the private/MrSidims/DeviceHost branch from 428d2ff to 1a1237c Compare July 1, 2020 14:47

MrSidims requested a review from s-kanaev July 1, 2020 16:07

s-kanaev reviewed Jul 1, 2020

View reviewed changes

sycl/include/CL/sycl/access/access.hpp Show resolved Hide resolved

MrSidims requested a review from s-kanaev July 2, 2020 10:22

s-kanaev approved these changes Jul 2, 2020

View reviewed changes

mlychkov approved these changes Jul 3, 2020

View reviewed changes

bader merged commit 94b36ac into intel:sycl Jul 3, 2020

bader reviewed Jul 3, 2020

View reviewed changes

MrSidims mentioned this pull request Jul 5, 2020

[SYCL] Mangling issues for device_ptr/global_ptr for non-SPIR targets #2039

Closed

FreddyLeaf pushed a commit to FreddyLeaf/llvm that referenced this pull request Mar 22, 2023

Fix static analyzer issues (intel#1864)

4ce748e

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com> Original commit: KhronosGroup/SPIRV-LLVM-Translator@32721e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add device_ptr and host_ptr #1864

[SYCL] Add device_ptr and host_ptr #1864

MrSidims commented Jun 10, 2020 •

edited

Loading

MrSidims commented Jun 17, 2020

romanovvlad commented Jun 17, 2020

MrSidims commented Jun 18, 2020

Fznamznon left a comment

MrSidims commented Jun 19, 2020

Naghasan Jun 19, 2020

MrSidims Jun 19, 2020 •

edited

Loading

Naghasan Jun 19, 2020

MrSidims Jun 19, 2020

MrSidims Jun 19, 2020

Naghasan Jun 19, 2020

MrSidims Jun 19, 2020 •

edited

Loading

MrSidims Jun 29, 2020

bader Jul 3, 2020

MrSidims commented Jun 23, 2020

mlychkov left a comment

s-kanaev commented Jul 1, 2020

MrSidims commented Jul 1, 2020

MrSidims commented Jul 2, 2020 •

edited

Loading

MrSidims commented Jul 2, 2020

s-kanaev left a comment

bader commented Jul 2, 2020

MrSidims commented Jul 2, 2020 •

edited

Loading

mlychkov left a comment

bader commented Jul 3, 2020

bader Jul 3, 2020

MrSidims Jul 3, 2020

[SYCL] Add device_ptr and host_ptr #1864

[SYCL] Add device_ptr and host_ptr #1864

Conversation

MrSidims commented Jun 10, 2020 • edited Loading

MrSidims commented Jun 17, 2020

romanovvlad commented Jun 17, 2020

MrSidims commented Jun 18, 2020

Fznamznon left a comment

Choose a reason for hiding this comment

MrSidims commented Jun 19, 2020

Choose a reason for hiding this comment

MrSidims Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims commented Jun 23, 2020

mlychkov left a comment

Choose a reason for hiding this comment

s-kanaev commented Jul 1, 2020

MrSidims commented Jul 1, 2020

MrSidims commented Jul 2, 2020 • edited Loading

MrSidims commented Jul 2, 2020

s-kanaev left a comment

Choose a reason for hiding this comment

bader commented Jul 2, 2020

MrSidims commented Jul 2, 2020 • edited Loading

mlychkov left a comment

Choose a reason for hiding this comment

bader commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims commented Jun 10, 2020 •

edited

Loading

MrSidims Jun 19, 2020 •

edited

Loading

MrSidims Jun 19, 2020 •

edited

Loading

MrSidims commented Jul 2, 2020 •

edited

Loading

MrSidims commented Jul 2, 2020 •

edited

Loading