[SYCL] Implement SYCL 2020 multi_ptr #6893

steffenlarsen · 2022-09-28T13:53:09Z

This commit adds implementations of SYCL 2020 multi_ptr and address_space_cast. Likewise it adds the legacy decoration for SYCL 1.2.1 style multi_ptr as deprecated.
To prevent breaking user code the legacy decoration is made the default.

Test-suite changes: intel/llvm-test-suite#1293

This commit adds implementations of SYCL 2020 multi_ptr and address_space_cast. Likewise it adds the legacy decoration for SYCL 1.2.1 style multi_ptr as deprecated. To prevent breaking user code the legacy decoration is made the default. Signed-off-by: Larsen, Steffen <[email protected]>

Signed-off-by: Larsen, Steffen <[email protected]>

getPointerAdjusted would accidentally drop the address space of the qualified pointer, in turn causing an address-space cast when creating a multi_ptr from it. To avoid this, getPointerAdjusted is made auto to pick return type based on getQualifiedPtr rather than always address-space-less. Signed-off-by: Larsen, Steffen <[email protected]>

sycl/include/sycl/access/access.hpp

sycl/include/sycl/accessor.hpp

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

sycl/include/sycl/ext/oneapi/matrix/matrix-tensorcore.hpp

sycl/include/sycl/multi_ptr.hpp

Co-authored-by: Alexey Sachkov <[email protected]>

Signed-off-by: Larsen, Steffen <[email protected]>

Naghasan

Nice work which highlighted some spec issues.

You seem to pick by default the decorated interface regardless of whether you need the interface to return decorated pointers or not. I would advice to favour the decorated::no over decorated::yes. The reason is decorated::yes uses types that requires extension to C++ to be built therefore it requires the user to understand this for them to cope with this. The version with decorated::no doesn't require require any extension from a user perspective (unless they use get_decorated). While your patch doesn't trigger unintended exposure, it implicitly encourages it.

sycl/include/sycl/access/access.hpp

sycl/include/CL/__spirv/spirv_ops.hpp

sycl/include/sycl/accessor.hpp

sycl/include/sycl/ext/oneapi/group_local_memory.hpp

Naghasan · 2022-09-29T10:31:05Z

sycl/include/sycl/group.hpp

+    auto DestP = multi_ptr<VecT, DestS, DestIsDecorated>(
+        reinterpret_cast<VecT *>(Dest.get()));
+    auto SrcP = multi_ptr<VecT, SrcS, SrcIsDecorated>(
+        reinterpret_cast<VecT *>(Src.get()));


that's not using the spec interface, spec is missing a cast operator ...

spec compliant work around

Suggested change

auto DestP = multi_ptr<VecT, DestS, DestIsDecorated>(

reinterpret_cast<VecT *>(Dest.get()));

auto SrcP = multi_ptr<VecT, SrcS, SrcIsDecorated>(

reinterpret_cast<VecT *>(Src.get()));

auto DestP = address_space_cast<VecT, DestS, DestIsDecorated>(

reinterpret_cast<VecT *>(Dest.get()));

auto SrcP = address_space_cast<VecT, SrcS, SrcIsDecorated>(

reinterpret_cast<VecT *>(Src.get()));

(potentially uses DPC++ extension to strip away address space via the reinterpret cast)

Ideally you would have

Suggested change

auto DestP = multi_ptr<VecT, DestS, DestIsDecorated>(

reinterpret_cast<VecT *>(Dest.get()));

auto SrcP = multi_ptr<VecT, SrcS, SrcIsDecorated>(

reinterpret_cast<VecT *>(Src.get()));

auto DestP = address_space_cast<VecT, DestS, DestIsDecorated>(

reinterpret_cast<VecT *>(Dest.get_raw()));

auto SrcP = address_space_cast<VecT, SrcS, SrcIsDecorated>(

reinterpret_cast<VecT *>(Src.get_raw()));

but we would need to add get_raw to the legacy interface.

I have made the first changes, i.e. the work-around. Should this be brought to SYCL-Docs?

sycl/include/sycl/multi_ptr.hpp

sycl/include/sycl/pointers.hpp

sycl/include/sycl/reduction.hpp

Signed-off-by: Larsen, Steffen <[email protected]>

sycl/include/CL/__spirv/spirv_ops.hpp

Signed-off-by: Larsen, Steffen <[email protected]>

gmlueck

Changes to extension specifications LGTM.

smanna12

FE changes LGTM

dkhaldi

Changes in matrix LGTM

bader · 2022-10-07T13:35:12Z

@steffenlarsen, please, resolve merge conflicts. It looks easy to do, but I think I'll mess-up formatting.

…_ptr

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2022-10-10T09:47:05Z

@intel/llvm-reviewers-runtime | @KseniyaTikhomirova - Friendly ping.

Signed-off-by: Larsen, Steffen <[email protected]>

clang/lib/Sema/SPIRVBuiltins.td

Signed-off-by: Larsen, Steffen <[email protected]>

Naghasan

Thanks for the updates :)

steffenlarsen · 2022-10-12T12:58:08Z

/verify with intel/llvm-test-suite#1293

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2022-10-12T17:47:14Z

/verify with intel/llvm-test-suite#1293

steffenlarsen · 2022-10-13T09:14:33Z

/verify with intel/llvm-test-suite#1293

intel#6893 added support for SYCL 2020 multi_ptr as well as a fix to one of the operators. The fixed operator did however retain an unused parameter. This commit removes this unused parameter. Signed-off-by: Larsen, Steffen <[email protected]>

#6893 added support for SYCL 2020 multi_ptr as well as a fix to one of the operators. The fixed operator did however retain an unused parameter. This commit removes this unused parameter. Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen requested a review from a team as a code owner September 28, 2022 13:53

steffenlarsen requested a review from KseniyaTikhomirova September 28, 2022 13:53

steffenlarsen force-pushed the steffen/SYCL2020_multi_ptr branch from e0acfbe to 86cb94a Compare September 28, 2022 13:53

steffenlarsen mentioned this pull request Sep 28, 2022

[SYCL] Add and adjust tests for SYCL 2020 multi_ptr intel/llvm-test-suite#1293

Merged

steffenlarsen added 5 commits September 28, 2022 07:38

Make address space case work-around for NVPTX and AMDGCN

a91e39a

Signed-off-by: Larsen, Steffen <[email protected]>

Fix formatting

1a6ebb4

Signed-off-by: Larsen, Steffen <[email protected]>

Correctly propagate decoration in matrix-tensorcore

b0a0a0e

Signed-off-by: Larsen, Steffen <[email protected]>

Remember joint_matrix_store_impl

81697c8

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen requested review from AlexeySachkov and KornevNikita September 28, 2022 19:21

AlexeySachkov reviewed Sep 29, 2022

View reviewed changes

steffenlarsen and others added 2 commits September 29, 2022 11:00

Update sycl/include/sycl/ext/intel/experimental/fpga_lsu.hpp

f467339

Co-authored-by: Alexey Sachkov <[email protected]>

Fix naming in fpga_lsu

90689a3

Signed-off-by: Larsen, Steffen <[email protected]>

Naghasan requested changes Sep 29, 2022

View reviewed changes

steffenlarsen added 2 commits September 30, 2022 06:08

Address review comments

06ec7e8

Signed-off-by: Larsen, Steffen <[email protected]>

Move SPIR-V casts to SPIRVBuiltins.td and make const

ffc89ae

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen requested a review from a team as a code owner September 30, 2022 14:21

steffenlarsen added 3 commits September 30, 2022 08:26

Fix getting decorated ptrs in atomic

ef595db

Signed-off-by: Larsen, Steffen <[email protected]>

Remove pointer, ref, and quals instead of propagating in deduce_AS

46b2677

Signed-off-by: Larsen, Steffen <[email protected]>

Correct use of decorated pointers for atomics

9d5c063

Signed-off-by: Larsen, Steffen <[email protected]>

Fznamznon reviewed Oct 4, 2022

View reviewed changes

sycl/include/CL/__spirv/spirv_ops.hpp Outdated Show resolved Hide resolved

Add decoration to matrix-jit-use.cpp and adjust extensions

bf30cfa

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen requested a review from a team as a code owner October 5, 2022 09:29

gmlueck approved these changes Oct 5, 2022

View reviewed changes

Fznamznon approved these changes Oct 5, 2022

View reviewed changes

smanna12 approved these changes Oct 5, 2022

View reviewed changes

elizabethandrews approved these changes Oct 5, 2022

View reviewed changes

dkhaldi approved these changes Oct 5, 2022

View reviewed changes

steffenlarsen requested a review from Naghasan October 7, 2022 10:01

steffenlarsen added 4 commits October 7, 2022 07:07

Merge remote-tracking branch 'intel/sycl' into steffen/SYCL2020_multi…

309bea3

…_ptr

Fix formatting

0a0cd43

Signed-off-by: Larsen, Steffen <[email protected]>

Fix merge mistake

bae047d

Signed-off-by: Larsen, Steffen <[email protected]>

Fix another merge mistake

2a7b384

Signed-off-by: Larsen, Steffen <[email protected]>

KseniyaTikhomirova approved these changes Oct 10, 2022

View reviewed changes

Remove deprecation of legacy enum and add notes to deprecate legacy

427777e

Signed-off-by: Larsen, Steffen <[email protected]>

Naghasan requested changes Oct 12, 2022

View reviewed changes

clang/lib/Sema/SPIRVBuiltins.td Outdated Show resolved Hide resolved

steffenlarsen added 2 commits October 12, 2022 03:44

Fix generic cast SPIR-V builtin signatures

ced5db7

Signed-off-by: Larsen, Steffen <[email protected]>

Fix formatting

eec663f

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen requested a review from Naghasan October 12, 2022 11:10

Naghasan approved these changes Oct 12, 2022

View reviewed changes

Move operator= sfinae into template args to appease MSVC

2a80b38

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen merged commit 8700b76 into intel:sycl Oct 13, 2022

steffenlarsen mentioned this pull request Oct 13, 2022

[SYCL][NFC] Remove unused parameter from multi_ptr operator #7047

Merged

Pennycook mentioned this pull request Jun 9, 2023

Implement SYCL 2020 address_space_cast #5526

Closed

AlexeySachkov mentioned this pull request Apr 3, 2024

[SYCL][SubBlockNDRange] Extend spec to allow dealing with USM pointers #849

Closed

[SYCL] Implement SYCL 2020 multi_ptr #6893

[SYCL] Implement SYCL 2020 multi_ptr #6893

Uh oh!

Conversation

steffenlarsen commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Naghasan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Naghasan Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

dkhaldi left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Oct 7, 2022

Uh oh!

steffenlarsen commented Oct 10, 2022

Uh oh!

Uh oh!

Naghasan left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Oct 12, 2022

Uh oh!

steffenlarsen commented Oct 12, 2022

Uh oh!

steffenlarsen commented Oct 13, 2022

Uh oh!

Uh oh!

steffenlarsen commented Sep 28, 2022 •

edited

Loading