[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

Tue Mar 30 10:59:03 PDT 2021

bader added a subscriber: ABataev.
bader added inline comments.

================
Comment at: clang/docs/SYCLSupport.md:73
+the integration header is used (included) by the SYCL runtime implementation, so
+the header must be available before the host compilation starts.*
+
----------------
Naghasan wrote:
> > First, it must be possible to use any host compiler
> 
> I don't understand the link with the integration header. SYCL being implementable as a library is a design principle of the specs but it doesn't means the clang host compiler has to remain a vanilla C++ compiler.
> 
> > information provided in the integration header is used (included) by the SYCL runtime implementation, so the header must be available before the host compilation starts
> 
> Another approach to the integration header would be for clang as the host compiler to generate the used type traits.
> > First, it must be possible to use any host compiler
> 
> I don't understand the link with the integration header. SYCL being implementable as a library is a design principle of the specs but it doesn't means the clang host compiler has to remain a vanilla C++ compiler.
> 
> > information provided in the integration header is used (included) by the SYCL runtime implementation, so the header must be available before the host compilation starts
> 
> Another approach to the integration header would be for clang as the host compiler to generate the used type traits.

If there are no objections from @keryell, I'd like to prototype this approach for SYCL first to make sure there are no blocking issues.
This option seems to be worth to explore considering integration header approach disadvantages.

================
Comment at: clang/docs/SYCLSupport.md:123
+traverse all symbols accessible from kernel functions and add them to the
+"device part" of the code marking them with the new SYCL device attribute.
+
----------------
Naghasan wrote:
> OpenMP offload uses a similar approach isn't it? Might be worth to describe how the 2 relates to each other and where they diverge. 
Do you mean the approach OpenMP compiler uses to outline single-source code parts to offload?
To be honest, I'm not sure... @ABataev, is there any description how OpenMP compiler outlines device code?
https://clang.llvm.org/docs/OpenMPSupport.html doesn't provide much details unfortunately.

================
Comment at: clang/docs/SYCLSupport.md:130
+`accessor` classes. Raw pointers map to kernel parameters one-to-one without
+additional transformations. `accessor` classes require additional processing as
+The "device" implementation of this class contains pointers to the device memory
----------------
Naghasan wrote:
> > Raw pointers map to kernel parameters one-to-one without additional transformations
> 
> Is this true ? I thought they should be passed as "pointer to global memory" (under the OpenCL or CUDA model).
Good catch. I'll adjust the wording.

================
Comment at: clang/docs/SYCLSupport.md:132
+The "device" implementation of this class contains pointers to the device memory
+as a class member. OpenCL doesn't allow passing structures with pointer type
+members as kernel parameters. All memory objects shared between host and device
----------------
Naghasan wrote:
> Won't be better to refer to SPIR or SPIR-V kernels rather than OpenCL ?
> 
> Or even SPIR-like or SPIR-defacto to be even less normative.
Is it okay if I refer to OpenCL SPIR-V environment specification:
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Env.html#_kernels?

================
Comment at: clang/docs/SYCLSupport.md:134
+members as kernel parameters. All memory objects shared between host and device
+must be passed to the kernel as raw pointers.
+
----------------
Naghasan wrote:
> This is a bit ambiguous and leaves the impression you can't objects pass by value.
Agree. Removed.

================
Comment at: clang/docs/SYCLSupport.md:161
+// Generated kernel function (expressed in OpenCL-like pseudo-code)
+__kernel KernelName(global int* a) {
+  KernelType KernelFuncObj; // Actually kernel function object declaration
----------------
Naghasan wrote:
> This is missing  the template instantiation  that will eventually lead to that lowering.
> 
> I would also suggest to split code block in 2 as to mark what is in header and source file (SYCL code) and what is compiler generated (that pseudo OpenCL).
> 
> Might be good to also mention the glue generated in the integration header as this is what allows arguments to be set by the runtime (bridge between the structure in C++ and the SPIR-like kernel arguments).
> This is missing  the template instantiation  that will eventually lead to that lowering.
> I would also suggest to split code block in 2 as to mark what is in header and source file (SYCL code) and what is compiler generated (that pseudo OpenCL).

Do you suggest to sketch an SYCL kernel invocation API usage example with the complete implementation of these methods to demonstrate the full stack? I was trying to avoid runtime/header part of implementation in this section and describe only API compiler relies on or provides for the runtime library. I think these details were in this document before, but moved to https://github.com/intel/llvm/blob/sycl/sycl/doc/KernelParameterPassing.md referenced below. 

> Might be good to also mention the glue generated in the integration header as this is what allows arguments to be set by the runtime (bridge between the structure in C++ and the SPIR-like kernel arguments).

I've mentioned that, but I hope KernelParameterPassing.md document provides the rest of the details.
Is that okay?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99190