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

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.


================
Comment at: clang/docs/SYCLSupport.md:821
+SYCL compiler toolchain. Section 3.8.2 of SYCL 2020 specification defines
+[memory model](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_sycl_device_memory_model),
+section 4.7.7 - [address space classes](https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_classes)
----------------
Anastasia wrote:
> > The memory model for SYCL devices is based on the OpenCL 1.2 memory model.
> 
> Is this possibly a spec bug? OpenCL didn't have generic address space in v1.2, it has only been added in v2.0.
> 
> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#the-generic-address-space
> 
> 
I filed https://github.com/KhronosGroup/SYCL-Docs/issues/131 to clarify.


================
Comment at: clang/docs/SYCLSupport.md:830
+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
+address space attributes. This design has two important features: keeps the type system consistent with C++ on one hand and enable tools for emitting device code aligned with SPIR memory model (and other GPU targets).
----------------
Anastasia wrote:
> Is this explained somewhere would you be able to add any reference?
I wasn't able to find documentation for this implementation detail, but we should be able to confirm that by printing AST for example.

Here is the documentation I found for CUDA in llvm project:
 - https://llvm.org/docs/CompileCudaWithLLVM.html
 - https://llvm.org/docs/NVPTXUsage.html - defines LLVM IR representation for NVPTX.

NVIDIA documentation - https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#variable-memory-space-specifiers.
It says that memory space is assigned using "variable specifiers" rather than type qualifiers.


================
Comment at: clang/docs/SYCLSupport.md:851
+
+Changing variable type has massive and destructive effect in C++. For instance
+this does not compile in C++ for OpenCL mode:
----------------
Anastasia wrote:
> aaron.ballman wrote:
> > 
> > This example demonstrates the problem with compiling C++ code when address space type qualifiers are inferred.
> > 
> >     The example compiles in accordance with OpenCL language semantic...
> > 
> > https://godbolt.org/z/9jzxK5xc4 - ToT clang doesn't compile this example.
> 
> I am still not clear what message you are trying to convey here? In OpenCL kernel languages any object is always in some address space so if you write the following `decltype(p)`, it will always have address space attribute in a type. OpenCL spec is very explicit about this:
> 
> https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-inference
> 
> So if you compare a type not attributed by an address space with an attributed one they will never compare as equal because according to C++ rules if the qualifiers differ the types will differ. You need to use a special type trait to remove an address space if you need to compare types not qualified by an address space. What is important to highlight however is that address space inference is where OpenCL differs to C or C++. But of course, neither C nor C++ have address spaces so it is hard to compare.
> 
> In relation to your documentation, it is not clear what you are trying to achieve with this paragraph?
>  
> In relation to your documentation, it is not clear what you are trying to achieve with this paragraph?

This paragraph provides clarification to the question why we can't apply OpenCL address space inference rules for SYCL mode.
I think it might be unnecessary because the SYCL specification defines address space deduction rules now.
Do you suggest removing this paragraph?


================
Comment at: clang/docs/SYCLSupport.md:909
+| `__attribute__((opencl_local))` | local_space |
+| `__attribute__((opencl_private))` | private_space |
+
----------------
Anastasia wrote:
> Since SYCL spec has constant AS you should explain whether it is going to be supported or not and if so then how.
The first raw of this table covers mapping between SYCL constant_space and address space attribute.
Could you clarify what else do we need?


================
Comment at: clang/docs/SYCLSupport.md:914-919
+Default address space represents "Generic-memory", which is a virtual address
+space which overlaps the global, local and private address spaces. SYCL mode
+enables conversion to/from default address space from/to address space
+attributed type.
+
+SPIR target allocates SYCL namespace scope variables in global address space.
----------------
Anastasia wrote:
> Naghasan wrote:
> > aaron.ballman wrote:
> > > 
> > I think this section should be extended.
> > 
> > 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> > 
> > This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).
> Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> 
> Do I understand it correctly that your implementation will use the first approach from the two described in:
> > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> 
> This should be added to the documentation btw.
> 
> 
> Btw does this statement in any way relate to the following statement:
> 
> > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> 
> from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> 
> Or if not where can I find the algorithm it refers to?
> I think this section should be extended.
> 
> 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> 
> This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).

I've added this text to the document.

> Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> 
> Do I understand it correctly that your implementation will use the first approach from the two described in:
> > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> 
> This should be added to the documentation btw.

The implementation residing in https://github.com/intel/llvm targets devices supporting generic address space. If I understand it correctly, another approach is supported by ComputeCPP. @Naghasan, are you aware of any plans to upstream the second approach? If no, I can clarify that it's not supported.

> Btw does this statement in any way relate to the following statement:
> 
> > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> 
> from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> 
> Or if not where can I find the algorithm it refers to?

AFAIK, the pointer types mentioned in this section is not implemented yet, but their implementation can be done in the library using the attributes described in this section and C++ class templates. 

Just to demonstrate the idea, let's use the example implementation for `multi_ptr` template class provided above:
```
`multi_ptr` class implementation example:

``` C++
template <typename T, address_space AS> class multi_ptr {
  // DecoratedType applies corresponding address space attribute to the type T
  // DecoratedType<T, global_space>::type == "__attribute__((opencl_global)) T"
  // See sycl/include/CL/sycl/access/access.hpp for more details
  using pointer_t = typename DecoratedType<T, AS>::type *;

  pointer_t m_Pointer;
  public:
  pointer_t get() { return m_Pointer; }
  T& operator* () { return *reinterpret_cast<T*>(m_Pointer); }
}
```
"decorated" pointers will return `pointer_t`, where as "raw" pointers will return the type casted to "generic" address space.


================
Comment at: clang/docs/SYCLSupport.md:914-919
+Default address space represents "Generic-memory", which is a virtual address
+space which overlaps the global, local and private address spaces. SYCL mode
+enables conversion to/from default address space from/to address space
+attributed type.
+
+SPIR target allocates SYCL namespace scope variables in global address space.
----------------
bader wrote:
> Anastasia wrote:
> > Naghasan wrote:
> > > aaron.ballman wrote:
> > > > 
> > > I think this section should be extended.
> > > 
> > > 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> > > 
> > > This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).
> > Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> > 
> > Do I understand it correctly that your implementation will use the first approach from the two described in:
> > > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> > 
> > This should be added to the documentation btw.
> > 
> > 
> > Btw does this statement in any way relate to the following statement:
> > 
> > > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> > 
> > from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> > 
> > Or if not where can I find the algorithm it refers to?
> > I think this section should be extended.
> > 
> > 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> > 
> > This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).
> 
> I've added this text to the document.
> 
> > Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> > 
> > Do I understand it correctly that your implementation will use the first approach from the two described in:
> > > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> > 
> > This should be added to the documentation btw.
> 
> The implementation residing in https://github.com/intel/llvm targets devices supporting generic address space. If I understand it correctly, another approach is supported by ComputeCPP. @Naghasan, are you aware of any plans to upstream the second approach? If no, I can clarify that it's not supported.
> 
> > Btw does this statement in any way relate to the following statement:
> > 
> > > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> > 
> > from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> > 
> > Or if not where can I find the algorithm it refers to?
> 
> AFAIK, the pointer types mentioned in this section is not implemented yet, but their implementation can be done in the library using the attributes described in this section and C++ class templates. 
> 
> Just to demonstrate the idea, let's use the example implementation for `multi_ptr` template class provided above:
> ```
> `multi_ptr` class implementation example:
> 
> ``` C++
> template <typename T, address_space AS> class multi_ptr {
>   // DecoratedType applies corresponding address space attribute to the type T
>   // DecoratedType<T, global_space>::type == "__attribute__((opencl_global)) T"
>   // See sycl/include/CL/sycl/access/access.hpp for more details
>   using pointer_t = typename DecoratedType<T, AS>::type *;
> 
>   pointer_t m_Pointer;
>   public:
>   pointer_t get() { return m_Pointer; }
>   T& operator* () { return *reinterpret_cast<T*>(m_Pointer); }
> }
> ```
> "decorated" pointers will return `pointer_t`, where as "raw" pointers will return the type casted to "generic" address space.
> Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> 
> Do I understand it correctly that your implementation will use the first approach from the two described in:
> > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> 
> This should be added to the documentation btw.
> 
> 
> Btw does this statement in any way relate to the following statement:
> 
> > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> 
> from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> 
> Or if not where can I find the algorithm it refers to?




================
Comment at: clang/docs/SYCLSupport.md:914-919
+Default address space represents "Generic-memory", which is a virtual address
+space which overlaps the global, local and private address spaces. SYCL mode
+enables conversion to/from default address space from/to address space
+attributed type.
+
+SPIR target allocates SYCL namespace scope variables in global address space.
----------------
bader wrote:
> bader wrote:
> > Anastasia wrote:
> > > Naghasan wrote:
> > > > aaron.ballman wrote:
> > > > > 
> > > > I think this section should be extended.
> > > > 
> > > > 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> > > > 
> > > > This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).
> > > Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> > > 
> > > Do I understand it correctly that your implementation will use the first approach from the two described in:
> > > > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> > > 
> > > This should be added to the documentation btw.
> > > 
> > > 
> > > Btw does this statement in any way relate to the following statement:
> > > 
> > > > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> > > 
> > > from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> > > 
> > > Or if not where can I find the algorithm it refers to?
> > > I think this section should be extended.
> > > 
> > > 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> > > 
> > > This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).
> > 
> > I've added this text to the document.
> > 
> > > Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> > > 
> > > Do I understand it correctly that your implementation will use the first approach from the two described in:
> > > > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> > > 
> > > This should be added to the documentation btw.
> > 
> > The implementation residing in https://github.com/intel/llvm targets devices supporting generic address space. If I understand it correctly, another approach is supported by ComputeCPP. @Naghasan, are you aware of any plans to upstream the second approach? If no, I can clarify that it's not supported.
> > 
> > > Btw does this statement in any way relate to the following statement:
> > > 
> > > > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> > > 
> > > from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> > > 
> > > Or if not where can I find the algorithm it refers to?
> > 
> > AFAIK, the pointer types mentioned in this section is not implemented yet, but their implementation can be done in the library using the attributes described in this section and C++ class templates. 
> > 
> > Just to demonstrate the idea, let's use the example implementation for `multi_ptr` template class provided above:
> > ```
> > `multi_ptr` class implementation example:
> > 
> > ``` C++
> > template <typename T, address_space AS> class multi_ptr {
> >   // DecoratedType applies corresponding address space attribute to the type T
> >   // DecoratedType<T, global_space>::type == "__attribute__((opencl_global)) T"
> >   // See sycl/include/CL/sycl/access/access.hpp for more details
> >   using pointer_t = typename DecoratedType<T, AS>::type *;
> > 
> >   pointer_t m_Pointer;
> >   public:
> >   pointer_t get() { return m_Pointer; }
> >   T& operator* () { return *reinterpret_cast<T*>(m_Pointer); }
> > }
> > ```
> > "decorated" pointers will return `pointer_t`, where as "raw" pointers will return the type casted to "generic" address space.
> > Ok, if the implementation plans to follow the spec precisely then just adding a reference should be sufficient. 
> > 
> > Do I understand it correctly that your implementation will use the first approach from the two described in:
> > > If the target of the SYCL backend can represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "generic as default address space rules" in Section 5.9.3 apply. If the target of the SYCL backend cannot represent the generic address space, then the "common address space deduction rules" in Section 5.9.2 and the "inferred address space rules" in Section 5.9.4 apply.
> > 
> > This should be added to the documentation btw.
> > 
> > 
> > Btw does this statement in any way relate to the following statement:
> > 
> > > Within kernels, the underlying C++ pointer types can be obtained from an accessor. The pointer types will contain a compile-time deduced address space. So, for example, if a C++ pointer is obtained from an accessor to global memory, the C++ pointer type will have a global address space attribute attached to it. The address space attribute will be compile-time propagated to other pointer values when one pointer is initialized to another pointer value using a defined algorithm.
> > 
> > from https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory
> > 
> > Or if not where can I find the algorithm it refers to?
> 
> 
> I think this section should be extended.
> 
> 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 allocation context, the `default` address space of a non-pointer type is assigned to a specific address space. This is described in https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace.
> 
> This is also in line with the behaviour of CUDA (small example https://godbolt.org/z/veqTfo9PK).




================
Comment at: clang/docs/SYCLSupport.md:915
+Default address space represents "Generic-memory", which is a virtual address
+space which overlaps the global, local and private address spaces. SYCL mode
+enables conversion to/from default address space from/to address space
----------------
Anastasia wrote:
> aaron.ballman wrote:
> > 
> You should also explain what address spaces are super/sub-sets because this impacts implicit and explicit conversion behavior in an embedded C-like models. In relation to that, you should highlight that the private, local and global ASes are disjoint.
It's already covered in the SYCL device memory model section of the specification: https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_sycl_device_memory_model.
What additional clarification do we need in this document?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99488