[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Alexey Bader via Phabricator via llvm-commits]

bader added a subscriber: svenvh.
bader added a comment.

> Regarding SYCLDevice and SYCLAddrSpaceMap I am still not very convinced about the flow. Have you had any design discussion regarding this already that you could point to?

We discussed this with you in https://github.com/intel/llvm/pull/1039/.

> My main concern is that I am not sure it fits with the main design of Clang - producing IR that is to be consumed by multiple targets rather than only one specific target or even an external tool. Why do you add a triple component that is used only for one target SPIR:
>
> How would existing toolchains that consume LLVM-IR for SPIR cope with the fact that the Default address space is different for C++ and SYCL.

High level languages differences doesn't matter for toolchains consuming LLVM IR - it can python, D or SYCL. Toolchains are assuming that LLVM IR follows the sematic defined in SPIR-1.2 document - https://www.khronos.org/registry/SPIR/specs/spir_spec-1.2.pdf.

> Why is changing of `the address space map necessary for SPIR but not the other targets?

The main reason is the difference in handling Default address space in C++ for OpenCL mode and SYCL. C++ for OpenCL doesn't always deduce address space and there are cases when Default is used and SPIR target maps it AS 0 (equivalent of SPIR-V Function storage class). This lead to inconsistencies like reported here: https://bugs.llvm.org/show_bug.cgi?id=45472.

SYCL doesn't deduce language address space at all relies on mapping Default in LLVM AS equivalent of SPIR-V generic.

NOTE: other GPU targets like AMDGPU and NVPTX do the same i.e. maps Default language AS to LLVM IR AS equivalent of SPIR-V generic, so there is no need to update. I believe SPIR target must apply the same mapping.

We use alternative map to not impact OpenCL language modes.

I provided clang lit tests results for changing the default map below. Most of the failures are caused by valid changes in LLVM IR and can be fixed by updating FileCheck comments, but there are a few crashes in CodeGen for "C++ for OpenCL" mode (with the root cause similar t o https://bugs.llvm.org/show_bug.cgi?id=45472). AFAIK, it's the only blocker for using unified map for any language mode. @svenvh, do you think we can address https://bugs.llvm.org/show_bug.cgi?id=45472 and update SPIRAddrSpaceMap (i.e. SPIRAddrSpaceMap[0] <- 4)?

Failed Tests (22):

  Clang :: CodeGen/ext-int-cc.c
  Clang :: CodeGen/spir-half-type.cpp
  Clang :: CodeGenCXX/builtin-calling-conv.cpp
  Clang :: CodeGenOpenCL/addr-space-struct-arg.cl
  Clang :: CodeGenOpenCL/atomic-ops-libcall.cl
  Clang :: CodeGenOpenCL/blocks.cl
  Clang :: CodeGenOpenCL/intel-subgroups-avc-ext-types.cl
  Clang :: CodeGenOpenCL/opencl_types.cl
  Clang :: CodeGenOpenCL/partial_initializer.cl
  Clang :: CodeGenOpenCL/private-array-initialization.cl
  Clang :: CodeGenOpenCL/sampler.cl
  Clang :: CodeGenOpenCL/vectorLoadStore.cl
  Clang :: CodeGenOpenCLCXX/address-space-castoperators.cpp
  Clang :: CodeGenOpenCLCXX/address-space-deduction.cl
  Clang :: CodeGenOpenCLCXX/addrspace-derived-base.cl
  Clang :: CodeGenOpenCLCXX/addrspace-of-this.cl
  Clang :: CodeGenOpenCLCXX/addrspace-operators.cl
  Clang :: CodeGenOpenCLCXX/addrspace-references.cl
  Clang :: CodeGenOpenCLCXX/addrspace-with-class.cl
  Clang :: CodeGenOpenCLCXX/atexit.cl
  Clang :: CodeGenOpenCLCXX/template-address-spaces.cl
  Clang :: Index/pipe-size.cl



================
Comment at: clang/include/clang/AST/Type.h:499
+                                    other.getAddressSpace()) ||
+           (!hasAddressSpace() &&
+            (other.getAddressSpace() == LangAS::sycl_private ||
----------------
Anastasia wrote:
> This should be done in the method above.
Will do.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:624
 
+  // TBD: SYCL-specific ParsedAttr?
+  /// If this is an OpenCL address space attribute returns its SYCL
----------------
Anastasia wrote:
> Do you still plan to resolve this?
> Do you still plan to resolve this?

If re-using OpenCL ParsedAttr is okay, I can just remove this comment. Do you think we should add new ParsedAttr for SYCL?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89909/new/

https://reviews.llvm.org/D89909