[SYCL] Refactor address space attributes to align with llorg. #3634

bader · 2021-04-27T16:19:25Z

This PR contains two patches:

[SYCL] Implement SYCL address space attributes handling - cherry-pick from llorg of llvm/llvm-project@7818906.
Adjust DPC++ to address space handling changes from llorg - changes on top of llorg commit to keep DPC++ working.

Today use re-use OpenCL attributes in SYCL mode, so second patch updates these uses with SYCL attributes (not all of them though).
Second patch includes content of https://reviews.llvm.org/D100396 + additional changes:

Enable opencl_constant address space in SYCL mode.
SYCL mode reuses a lot of OpenCL code, which rely on OpenCL language address spaces. Switching to new SYCL address spaces breaks re-use "as is". I updated uses of opencl_local address space, but there are a lot of uses for opencl_global (e.g. special types like images, pipes, etc. are annotated with OpenCL language address space). We will have to introduce SYCL address spaces in less invasive way possible.
Identified that llvm/llvm-project@7818906 drops address space attributes for global variables. I'll upload fix for this issue to llorg shortly.
I left a couple of FIXME comments, which should be addressed eventually.
clang/lib/Sema/SPIRVBuiltins.td updated to use SYCL address spaces. @Naghasan, please, take a look. I'm not sure if SPIRVBuiltins.td is supposed to be used for SYCL language mode only.
Reverted a whitespace few changes and one LIT test (clang/test/SemaOpenCLCXX/address-space-lambda.clcpp) to match llorg source code.
Reverted 761b2ea, which was a workaround for custom SYCL mangling scheme for NVPTX target.

Default address space (applies when no explicit address space was specified) maps to generic (4) address space. Added SYCL named address spaces `sycl_global`, `sycl_local` and `sycl_private` defined as sub-sets of the default address space. Static variables without address space now reside in global address space when compile for SPIR target, unless they have an explicit address space qualifier in source code. Differential Revision: https://reviews.llvm.org/D89909

AaronBallman · 2021-04-27T18:59:17Z

This PR is marked as a draft -- do you want me to hold off on reviewing it until you remove that, or would you like review now?

clang/lib/AST/ItaniumMangle.cpp

This reverts commit 761b2ea.

bader · 2021-04-29T13:44:11Z

/summary:run

bader · 2021-04-29T13:50:34Z

This PR is marked as a draft -- do you want me to hold off on reviewing it until you remove that, or would you like review now?

I think it should be ready for review now. Hopefully CI checks won't detect new issues with this patch.

premanandrao

@bader, I have tried to best review the changes and they mostly look good, but I do not understand it fully. I just have a few nits and some minor questions.

clang/include/clang/Sema/ParsedAttr.h

clang/lib/Basic/Targets/SPIR.h

clang/test/CodeGenSYCL/address-space-conversions.cpp

clang/test/CodeGenSYCL/address-space-deduction.cpp

clang/test/SemaSYCL/address-space-conversions.cpp

clang/include/clang/Sema/ParsedAttr.h

AaronBallman · 2021-05-03T11:35:27Z

clang/lib/Basic/Targets/SPIR.h

+    // FIXME: SYCL specification considers unannotated pointers and references
+    // to be pointing to the generic address space. See section 5.9.3 of
+    // SYCL 2020 specification.


Are the SYCL spec maintainers aware of this implementation issue?

This comment is super strange, AMDGPU target is doing this exact thing (default is generic or flat address space) with a view on killing the map with private as default (see AMDGPU's adjust comment). NVPTX doesn't because the generic and private (local in the CUDA terminology) maps to the same address space, however the semantic used for Default matches the semantic of a flat address space.

I would be happy to avoid these SYCL customizations, but I don't think we can do better w/o breaking SPIR ABI.
For instance, SPIR-V translator relies on mapping address space 0 to private address space (in OpenCL terms).
Let me know if you have other ideas.

Not sure to whom you are answering :) but

I would be happy to avoid these SYCL customizations, but I don't think we can do better w/o breaking SPIR ABI.

That was the underlying reason of my comment: I don't think you can avoid this but this is not unprecedented anyway. Because of that, this comment is strange.

This comment was suggested by Anastasia here: https://reviews.llvm.org/D89909#inline-946388, https://reviews.llvm.org/D89909#inline-948325.
I'm open to improvements. Do you suggest removing "FIXME:" word from the comment?

If I'm understanding properly, it sounds like the FIXME is wrong -- the code doesn't need to be fixed, it's basically behaving as the implementation requires it, and what needs to change is the SYCL spec based on implementation feedback. So perhaps change if it from a FIXME to a NOTE and file an issue with the SYCL spec authors?

Changed FIXME to NOTE.

I'm not sure if there is an issue with the SYCL spec. The problem we discussed https://reviews.llvm.org/D89909 is that we now have two different mappings from AST/language address space to LLVM address space. The only difference between these is how Default address space is mapped. I see two different points of view on how Default should be treated for GPU programming languages. Anastasia thinks Default should be mapped to OpenCL private address space, but this logic doesn't work in case of SYCL and other language mode, which do not infer address space attribute at AST level. SYCL spec authors are well aware of this issue and I think SYCL 2020 spec has clarifications for this.

@Naghasan, feel free to correct me.

the code doesn't need to be fixed, it's basically behaving as the implementation requires it

That's my interpretation as well. The change to NOTE make sense.

Anastasia thinks Default should be mapped to OpenCL private address space

I fail to see any rational for that. SPIR is a target, it is not bound to an input language.

I'm not sure if there is an issue with the SYCL spec.

Oh there is a lot of issues in the spec, but I don't think this is one of them. And I'm not fully convinced there is a clang design issue either: the underlying issue is we are trying to map the SYCL/CUDA/OpenCL memory model onto C/C++ which will have limitations (pointer escape for instances).

clang/lib/CodeGen/CodeGenModule.cpp

clang/lib/AST/ItaniumMangle.cpp

clang/include/clang/AST/Type.h

clang/include/clang/Sema/ParsedAttr.h

Naghasan · 2021-05-06T13:18:12Z

clang/lib/Basic/Targets/SPIR.h

+    // FIXME: SYCL specification considers unannotated pointers and references
+    // to be pointing to the generic address space. See section 5.9.3 of
+    // SYCL 2020 specification.


This comment is super strange, AMDGPU target is doing this exact thing (default is generic or flat address space) with a view on killing the map with private as default (see AMDGPU's adjust comment). NVPTX doesn't because the generic and private (local in the CUDA terminology) maps to the same address space, however the semantic used for Default matches the semantic of a flat address space.

clang/lib/Sema/SPIRVBuiltins.td

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

premanandrao

Thanks for addressing my review comments. I am okay with this change.

bader · 2021-05-13T14:33:25Z

Thanks!
@AaronBallman, @Naghasan, does it look good to you?

AaronBallman

LGTM aside from the commenting suggestion.

AaronBallman · 2021-05-13T15:48:00Z

clang/lib/Basic/Targets/SPIR.h

+    // FIXME: SYCL specification considers unannotated pointers and references
+    // to be pointing to the generic address space. See section 5.9.3 of
+    // SYCL 2020 specification.


If I'm understanding properly, it sounds like the FIXME is wrong -- the code doesn't need to be fixed, it's basically behaving as the implementation requires it, and what needs to change is the SYCL spec based on implementation feedback. So perhaps change if it from a FIXME to a NOTE and file an issue with the SYCL spec authors?

bader · 2021-05-14T06:17:15Z

@kbobrovs, @DenisBakhvalov, I see failures in ESIMD/private_memory/pm_access_2.cpp and ESIMD/private_memory/pm_access_3.cpp after comment update:

warning: GenXPromoteArray: _ZTSN2cl4sycl6detail19__pf_kernel_wrapperI8KernelIDILi1EEEE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTS8KernelIDILi1EE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTSN2cl4sycl6detail19__pf_kernel_wrapperI8KernelIDILi2EEEE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTS8KernelIDILi2EE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTSN2cl4sycl6detail19__pf_kernel_wrapperI8KernelIDILi3EEEE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTS8KernelIDILi3EE allocation size is too big: using TPM
LLVM ERROR: Cannot find pointer replacement

Are these are known issues?

bader · 2021-05-14T08:09:18Z

@kbobrovs, @DenisBakhvalov, I see failures in ESIMD/private_memory/pm_access_2.cpp and ESIMD/private_memory/pm_access_3.cpp after comment update:

warning: GenXPromoteArray: _ZTSN2cl4sycl6detail19__pf_kernel_wrapperI8KernelIDILi1EEEE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTS8KernelIDILi1EE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTSN2cl4sycl6detail19__pf_kernel_wrapperI8KernelIDILi2EEEE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTS8KernelIDILi2EE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTSN2cl4sycl6detail19__pf_kernel_wrapperI8KernelIDILi3EEEE allocation size is too big: using TPM
warning: GenXPromoteArray: _ZTS8KernelIDILi3EE allocation size is too big: using TPM
LLVM ERROR: Cannot find pointer replacement

Are these are known issues?

I see these tests also fail in other PRs - #3614, so these doesn't seem to be caused by changes from this PR.

Naghasan · 2021-05-14T15:11:24Z

Thanks!
@AaronBallman, @Naghasan, does it look good to you?

LGTM :)

bader · 2021-05-14T15:16:44Z

Thanks!
@AaronBallman, @Naghasan, does it look good to you?

LGTM :)

Thanks a lot for review! I'll try to address your comments and remove the difference between llvm/llvm-project@7818906 and what we landed to sycl branch in follow-up patches.

This patch fixes the `core/GroupAsyncCopy.cl` and `ocl/prefetch.cl` lit tests in `libclc`. These started failing with the introduction of the SYCL address spaces in intel#3634. The issue is that the builtins are declared with the SYCL address spaces in the tablegen so when used in an OpenCL setting it ends up with address space mismatches between things like `opencl_global` and `sycl_global`. These two were the only tests affected because they're also the only tested builtins that do not have generic address space variants in tablegen, the other tests would just fall back on the generic address space variants. With this patch the builtins should automatically get the appropriate version of the address space depending on the context.

This patch fixes the `core/GroupAsyncCopy.cl` and `ocl/prefetch.cl` lit tests in `libclc`. These started failing with the introduction of the SYCL address spaces in #3634. The issue is that the builtins are declared with the SYCL address spaces in the tablegen so when used in an OpenCL setting it ends up with address space mismatches between things like `opencl_global` and `sycl_global`. These two were the only tests affected because they're also the only tested builtins that do not have generic address space variants in tablegen, the other tests would just fall back on the generic address space variants. With this patch the builtins should automatically get the appropriate version of the address space depending on the context.

bader added 3 commits April 27, 2021 14:44

Adjust DPC++ to address space handling changes from llorg.

cd2999b

Remove SPIR target redundant overloads.

07eaf09

bader requested a review from AaronBallman April 27, 2021 16:19

bader commented Apr 29, 2021

View reviewed changes

clang/lib/AST/ItaniumMangle.cpp Show resolved Hide resolved

Revert "[SYCL-PTX] Libclc binding for SYCL device (intel#1914)"

20b6d34

This reverts commit 761b2ea.

bader marked this pull request as ready for review April 29, 2021 13:49

bader requested review from elizabethandrews and premanandrao as code owners April 29, 2021 13:49

Revert unnecessary changes.

3b50946

premanandrao reviewed Apr 30, 2021

View reviewed changes

AaronBallman reviewed May 3, 2021

View reviewed changes

Naghasan reviewed May 6, 2021

View reviewed changes

bader linked an issue May 11, 2021 that may be closed by this pull request

Crash on unreachable that is reachable #3622

Closed

Merge remote-tracking branch 'intel/sycl' into sycl-address-spaces

3127da6

bader mentioned this pull request May 11, 2021

Update isAddressSpaceSupersetOf function comment. #3723

Open

bader and others added 2 commits May 11, 2021 17:47

Apply suggestions from code review

114b1dd

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

Apply code review comments.

665ea95

bader requested review from premanandrao and AaronBallman May 11, 2021 16:54

Replace FIXME with NOTE in the comments.

f57217b

premanandrao previously approved these changes May 13, 2021

View reviewed changes

AaronBallman previously approved these changes May 13, 2021

View reviewed changes

bader dismissed stale reviews from AaronBallman and premanandrao via f57217b May 13, 2021 16:28

bader mentioned this pull request May 14, 2021

[ESIMD] private_memory tests sporadically fail #3758

Closed

bader merged commit 870b840 into intel:sycl May 14, 2021

bader deleted the sycl-address-spaces branch May 14, 2021 15:10

npmiller mentioned this pull request Aug 6, 2021

[SYCL] Fix builtins address space type #4275

Merged

[SYCL] Refactor address space attributes to align with llorg. #3634

[SYCL] Refactor address space attributes to align with llorg. #3634

Uh oh!

Conversation

bader commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Apr 27, 2021

Uh oh!

Uh oh!

bader commented Apr 29, 2021

Uh oh!

bader commented Apr 29, 2021

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented May 13, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented May 14, 2021

Uh oh!

bader commented May 14, 2021

Uh oh!

Naghasan commented May 14, 2021

Uh oh!

bader commented May 14, 2021

Uh oh!

Uh oh!

bader commented Apr 27, 2021 •

edited

Loading