Naghasan added inline comments.
================ Comment at: clang/docs/SYCLSupport.md:909 +| `__attribute__((opencl_local))` | local_space | +| `__attribute__((opencl_private))` | private_space | + ---------------- bader wrote: > 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? To be more specific here, the OpenCL constant address space no longer have an equivalent in the SYCL core spec memory model https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_sycl_device_memory_model. The `multi_ptr` for that address space is now deprecated and implementors can map it the global address space (as described here). ================ 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: > > 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). > > > If I understand it correctly, another approach is supported by ComputeCPP. > @Naghasan, are you aware of any plans to upstream the second approach? We do support that other approach, but we do not have plan to upstream this. However an implementation doesn't have to support both, so you should just mention the implementation is solely based on the usage of generic. > from > https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_access_to_memory There is a spec bug here, this only applies to the deduced address space mode. > Or if not where can I find the algorithm it refers to? Section 5.9.4, however you will see a note stating rework is due. But that doesn't impact this design here. ================ Comment at: clang/docs/SYCLSupport.rst:222-224 +The main address space semantic difference between SYCL and OpenCL is that SYCL +doesn't perform the address space qualifier inference 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>`_. ---------------- Nitpicking things here, feel free to discard. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99488/new/ https://reviews.llvm.org/D99488 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
