Closed by https://reviews.llvm.org/rG18e165f50d8c1ab3afe7098dc00557d5f1a43cfa.

By commit https://reviews.llvm.org/rGc2006f857d80f54b90ed7d911d3e7acf4f46001b.

Sorry for the delay.
LGTM. Thanks!

Added documentation to cover conversion rules and rebased on ToT.

Added explicit cast checks to Sema tests.

Apply code review suggestions and rebase on ToT.

Committed alternative version - https://reviews.llvm.org/D89909.

Thanks! I've uploaded this version to https://reviews.llvm.org/D101549.

In D99488#2725435, @Anastasia wrote:

If I build docs now I get the following output:

llvm-project/build-doc/tools/clang/docs/SYCLSupport.rst:102: WARNING: Error in "code-block" directive:
1 argument(s) required, 0 supplied.

.. code-block::

   TODO: add support for `__attribute__((opencl_global_host))` and
   `__attribute__((opencl_global_device))`.

Is this something already being looked at?

Rebased on ToT to resolve merge conflicts.

Rebase on ToT

Rebased patch to unblock commit.

@Anastasia, I've updated https://reviews.llvm.org/D99488 and refactored getStringLiteralAddressSpace to handle non-string constants as well. Please, take a look.

Incorporate https://reviews.llvm.org/D89909 review feedback.

Generalize getStringLiteralAddressSpace to GetGlobalConstantAddressSpace

Added SYCL address spaces mangling for targets without address space map

Applied more review comments.

Applied more comments.

Applied more code review suggestions.

Applied code review suggestions.

In D99488#2671906, @Anastasia wrote:

LGTM! Thanks for working on this. The expected sematic seems fairly clear now.

Add ReST marks to hyperlinks.

Address comments from @Anastasia.

Fixed a typo in the code example.

Added simplified accessor declaration to the example and TODO section.

Applied code review suggestions from @Naghasan.

Applied code review suggestions from @Naghasan.

Convert document to ReST format.

Convert document from Markdown to ReST format.

Applied code review suggestions

Apply code review comments from @Naghasan.

@Anastasia, I've addressed the comments for the address space section in https://reviews.llvm.org/D99488. Let's move discussion there.

Applied code review comments.

In D99488#2655326, @Anastasia wrote:

Thanks. I am guessing you will address relevant review comments from D99190 in here?

Move address space handling section to https://reviews.llvm.org/D99488 to address https://reviews.llvm.org/D89909#2653452.

Yesterday, we chatted offline and agreed that the main issue is missing documentation for Clang extensions being added for SYCL. To address this issue we are adding SYCL architecture design document, which we are going to update along with adding new features.

Add links to SYCL specification with address space behavior description.

In D89909#2624284, @aaron.ballman wrote:

Based on the discussion so far, would these be acceptable steps to take?

0) Complete review on this patch for any technical concerns related to it and commit when it's ready (this unblocks some downstream needs quickly, hopefully).

1. @bader (or someone else on the SYCL team) creates a bare-bones SYCL documentation page that is quickly accepted as a placeholder for us to put more documentation.
2. Each new SYCL related review that needs user-facing documentation outside of what the SYCL standard documents will update the Clang SYCL doc.
3. @bader (or someone else on the SYCL team) writes address-space mapping documentation and adds it to the Clang SYCL doc.
4. @bader (or someone else on the SYCL team) looks at other commits that have already gone into Clang to write missing documentation.

Some of these steps can be done in parallel, of course. I recognize we could have the order be #1 -> #3 -> #0 (so the docs for this change are written as part of this patch), but my concern with that approach is that this patch is blocking other SYCL efforts (@bader can correct me if I'm wrong about this) and no one has identified a definite issue with it yet beyond the lack of Clang documentation.

If those steps seem unreasonable or like I've totally missed the point on something, perhaps we could reach an understanding more quickly via a meeting (we could summarize the decisions from the meeting here so the community is aware of the end results)?

In D99190#2652054, @keryell wrote:

! In D99190#2650326, @bader wrote:

2. I'm looking for suggestions on "OpenCL extensions" clarification.

I said that "OpenCL extensions" are misleading because it can be understood as either extensions inside OpenCL (cl_khr_ext...) or extensions to C/C++ with specific OpenCL features (like kernel or global).

Resolve "OpenCL extensions" ambiguity.

@keryell, thanks for the feedback.

Apply most of the comments from Ronan.

Let's take a look at compilation output for the following program:

c++
void foo(int *p) {}

OpenCL compiler produces following LLVM IR for SPIR target:

LLVM
define dso_local spir_func void @_Z3fooPU3AS4i(i32 addrspace(4)* nocapture %0) local_unnamed_addr #0 {
  ret void
}

Here is what SYCL device compiler produces:

LLVM
define dso_local spir_func void @_Z3fooPi(i32* nocapture %0) local_unnamed_addr #0 {
  ret void
}

We would like to get equivalent code produced by both compiler for SPIR target.
Currently SYCL device compiler emits pointer to address space 0 instead of expected 4. This lowering is defined by the address space map.
Having multiple maps is not something new to the clang community. Similar approach AMDGPU target applies to customize address space mapping for OpenCL language (https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/AMDGPU.cpp#L354).

[NFC] Align address space map names with AMDGPU target.

LGTM. Thanks!

Thanks! LGTM.
I think we should consider deprecating spir_func calling convention in favor of default calling convention. I don't see much value in having it today.

In D89909#2606180, @Anastasia wrote:

In D89909#2600859, @aaron.ballman wrote:

Just a few minor nits from me, but I'm mostly wondering: where are we at with this and are there still substantive changes required? (I looked through the comments, but there's a lot of back-and-forth since Oct and I'm not certain what's holding the patch back currently.)

To make it short, from my side I am not very clear about the overall design. From the SYCL spec side, there is no indication of what compiler extensions are needed and if at all. As a result, some of the design choices are unclear to me - in particular why SPIR target would need a separate address space map for SYCL. This is not how it was intended originally and I am worried that this will create issues for the consumers of IR to handle two different formats. But in general, if the community is now to maintain this code we should at least have some deeper understanding of it.

I would suggest starting from some high-level documentation that provides the details of the compiler extension being implemented. Perhaps the documentation that @bader has linked earlier could be used as a starting point with some more details that would allow assessing and reviewing the changes.

@zahiraam, could you rebase the patch to the tip of the branch, please? It looks like there are merge conflicts.

Apply suggestions from Aaron.

@aaron.ballman, it looks like unittests should be updated as well. Please, take a look at failures in pre-merge checks.

LGTM.

LGTM. Thanks.

Anastasia added a comment.
In D89909#2536331 https://reviews.llvm.org/D89909#2536331, @bader wrote:

In D89909#2536157 https://reviews.llvm.org/D89909#2536157, @Anastasia wrote:

In D89909#2534790 https://reviews.llvm.org/D89909#2534790, @bader wrote:

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/.

I can't see.

The mapping has been discussed in this comment: https://github.com/intel/llvm/pull/1039/#discussion_r369667791.

The discussion you cite is in the separate Github project, but you should have a discussion using LLVM channels about the design you propose for llvm-project repo. Also I don't think citing resources with many comments and no conclusions is making the review process very productive.

Fixed a couple of typos in the comments; NFC.