[PATCH] D99488: [SYCL][Doc] Add address space handling section to SYCL documentation

[Alexey Bader via Phabricator via cfe-commits]

bader marked 4 inline comments as done.
bader added inline comments.


================
Comment at: clang/docs/SYCLSupport.rst:243
+Similar to other single-source C++-based GPU programming modes like
+OpenMP/CUDA/HIP, SYCL uses clang's "default" address space for types with no
+explicit address space attribute. This design has two important features: it
----------------
Anastasia wrote:
> This is ambiguous now because every language will use `clang's "default" address space` because at least one address space is always needed by every language but it has different semantics in languages. We should either attempt to describe it somehow or perhaps just point out that it is inherited from CUDA and currently undocumented.
Removed this paragraph as it's already covered by SYCL specification.


================
Comment at: clang/docs/SYCLSupport.rst:341
+that overlaps the global, local, and private address spaces. SYCL mode enables
+conversion to/from the default address space from/to the address
+space-attributed type.
----------------
Anastasia wrote:
> Do you mean both implicit and explicit conversions? Does it mean that in your AS model named ASes are subset of generic AS and generic AS is a subset of named ASes so they are equivalent sets? It is probably good to mention here that all named address spaces are disjoint.
> Do you mean both implicit and explicit conversions? Does it mean that in your AS model named ASes are subset of generic AS and generic AS is a subset of named ASes so they are equivalent sets? It is probably good to mention here that all named address spaces are disjoint.

Updated paragraph:

The default address space is "generic-memory", which is a virtual address space
that overlaps the global, local, and private address spaces. SYCL mode enables
both explicit and implicit conversion to/from the default address space from/to
the address space-attributed type. All named address spaces are disjoint and
sub-sets of default address space.


================
Comment at: clang/docs/SYCLSupport.rst:344
+
+The SPIR target allocates SYCL namespace scope variables in the global address
+space.
----------------
Anastasia wrote:
> Interesting, will this deduction always be target specific or can it be generalized since it is governed by the language semantic already?
> Interesting, will this deduction always be target specific or can it be generalized since it is governed by the language semantic already?

It's target specific deduction. CPU targets doesn't require such deduction.


================
Comment at: clang/docs/SYCLSupport.rst:347
+
+Pointers to Default address space should get lowered into a pointer to a generic
+address space (or flat to reuse more general terminology). But depending on the
----------------
Anastasia wrote:
> I think it is also relevant to highlight that you don't perform inference of the address space qualifiers and the memory segment binding is performed as a final phase of parsing. This is quite relevant since embedded C or C++ have no address space inference at all and OpenCL explicitly requires inference in the type qualifiers.
> I think it is also relevant to highlight that you don't perform inference of the address space qualifiers and the memory segment binding is performed as a final phase of parsing. This is quite relevant since embedded C or C++ have no address space inference at all and OpenCL explicitly requires inference in the type qualifiers.

I move this paragraph before the code example right after this section:

SYCL borrows its memory model from OpenCL however SYCL doesn't perform
the address space qualifier inference as detailed in
`OpenCL C v3.0 6.7.8 <https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-inference>`_.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99488